Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 9: (8 inline comments)
Hi Tomas, I rethought things and I can accept everything you said. Please
disregard my comments from patchset 8 and focus on these ones in patchset 9.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/BaseListModelSuggestBox.java
Line 19:
Line 20: /**
Line 21: * Base SuggestBox widget that adapts to UiCommon list model items.
Line 22: */
Line 23: public abstract class BaseListModelSuggestBox<T> extends Composite
implements EditorWidget<T, TakesConstrainedValueEditor<T>>,
HasConstrainedValue<T> {
If we change the ListModelSuggestBox's name, we could just call this
ListModelSuggestBox.
Line 24:
Line 25: private TakesConstrainedValueEditor<T> editor;
Line 26:
Line 27: private T value;
Line 112: public void setEnabled(boolean enabled) {
Line 113: asTextBox().setEnabled(enabled);
Line 114: }
Line 115:
Line 116: public void setValue(T value) {
As I had already commented, I would just implement this as setValue(value,
false). Code reuse.
Line 117: this.value = value;
Line 118: render(value, false);
Line 119:
Line 120: }
Line 123: this.value = value;
Line 124: render(value, fireEvents);
Line 125: }
Line 126:
Line 127: protected abstract void render(T value, boolean fireEvents);
I still think it's weird for render to fire events. Why is that needed?
Line 128:
Line 129: protected abstract T wrap(String value);
Line 130:
Line 131: @Override
Line 125: }
Line 126:
Line 127: protected abstract void render(T value, boolean fireEvents);
Line 128:
Line 129: protected abstract T wrap(String value);
Maybe we could find a better name than wrap()? Because we're not just wrapping
a String with an arbitrary class T, we're inferring the T from the String.
Maybe interpret() or infer()?
Line 130:
Line 131: @Override
Line 132: public T getValue() {
Line 133: return value;
Line 137: public HandlerRegistration addValueChangeHandler(final
ValueChangeHandler<T> handler) {
Line 138: return asSuggestBox().addValueChangeHandler(new
ValueChangeHandler<String>() {
Line 139: @Override
Line 140: public void onValueChange(ValueChangeEvent<String> event)
{
Line 141: T value = wrap(event.getValue());
You know what, never mind, it is a matter of personal taste, I don't really
mind.
Line 142: if (value != null) {
Line 143: handler.onValueChange(new
ValueChangeEvent<T>(wrap(event.getValue())) {
Line 144: });
Line 145: }
Line 138: return asSuggestBox().addValueChangeHandler(new
ValueChangeHandler<String>() {
Line 139: @Override
Line 140: public void onValueChange(ValueChangeEvent<String> event)
{
Line 141: T value = wrap(event.getValue());
Line 142: if (value != null) {
Why notify of value change only if value != null?
Line 143: handler.onValueChange(new
ValueChangeEvent<T>(wrap(event.getValue())) {
Line 144: });
Line 145: }
Line 146: }
Line 139: @Override
Line 140: public void onValueChange(ValueChangeEvent<String> event)
{
Line 141: T value = wrap(event.getValue());
Line 142: if (value != null) {
Line 143: handler.onValueChange(new
ValueChangeEvent<T>(wrap(event.getValue())) {
Why wrap again? Use the value variable.
Line 144: });
Line 145: }
Line 146: }
Line 147: });
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelSuggestBox.java
Line 10:
Line 11: /**
Line 12: * SuggestBox widget that adapts to UiCommon list model items. Expects
all of it's items to be non null Strings
Line 13: */
Line 14: public class ListModelSuggestBox extends
BaseListModelSuggestBox<Object> {
Evidently the name of this class is confusing, maybe change it to
StringSuggestBox or something like that?
Line 15:
Line 16: public ListModelSuggestBox() {
Line 17: super(new MultiWordSuggestOracle());
Line 18: initWidget(asSuggestBox());
--
To view, visit http://gerrit.ovirt.org/14936
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I956af3c675894c850a1a104a81cec49f4bd62011
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
