Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 9: (5 inline comments)
....................................................
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> {
No problem.
Line 24:
Line 25: private TakesConstrainedValueEditor<T> editor;
Line 26:
Line 27: private T value;
Line 120: }
Line 121:
Line 122: public void setValue(T value, boolean fireEvents) {
Line 123: this.value = value;
Line 124: render(value, fireEvents);
I've come to think again that this should be
asSuggestBox().setValue(renderer.render(value), fireEvents) instead, see
comment below.
Line 125: }
Line 126:
Line 127: protected abstract void render(T value, boolean fireEvents);
Line 128:
Line 123: this.value = value;
Line 124: render(value, fireEvents);
Line 125: }
Line 126:
Line 127: protected abstract void render(T value, boolean fireEvents);
So I think it would be better to have a Renderer<T> passed to the constructor.
It's confusing that a method called render fires events, rendering is simply
returning a String representing an entity.
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);
Yeah, sounds good.
Line 130:
Line 131: @Override
Line 132: public T getValue() {
Line 133: return value;
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) {
Sounds to me like this argument doesn't fit an abstract base class. Generally,
an event should be fired on any value change, even if it is null. Then the
client listening to that event will decide what to do with null.
Line 143: handler.onValueChange(new
ValueChangeEvent<T>(wrap(event.getValue())) {
Line 144: });
Line 145: }
Line 146: }
--
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
