Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 6: (4 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> {
I don't see a situation where T could be anything that isn't a superclass of
String, because in general T objects will have to be built from user-input
Strings.
I didn't delve deeply into your implementation of TypeAheadListBox, but it
seems like you're only allowing to choose between existing values and not input
new values, in which case it could be possible to match the input String to its
implicit value. So it would make sense to make TypeAheadListBox generic, but I
don't think there's good reason to make BaseListModelSuggestBox generic.
Line 24:
Line 25: private TakesConstrainedValueEditor<T> editor;
Line 26:
Line 27: private T value;
Line 29: private SuggestBox suggestBox;
Line 30:
Line 31: private ListModelSuggestionDisplay suggestionDisplay = new
ListModelSuggestionDisplay();
Line 32:
Line 33: private ValueChangeHandler<T> handler;
I don't understand the need for this member.
Line 34:
Line 35: public BaseListModelSuggestBox(MultiWordSuggestOracle
suggestOracle) {
Line 36: suggestBox = new SuggestBox(suggestOracle, new TextBox(),
suggestionDisplay) {
Line 37: @Override
Line 121: public void setEnabled(boolean enabled) {
Line 122: asTextBox().setEnabled(enabled);
Line 123: }
Line 124:
Line 125: public void setValue(T value) {
No reason to rewrite code, this method should only perform setValue(value,
false).
Line 126: if (value != null && value != getValue()) {
Line 127: handler.onValueChange(new ValueChangeEvent<T>(value) {
Line 128: });
Line 129: }
Line 157: this.handler = handler;
Line 158: return asSuggestBox().addValueChangeHandler(new
ValueChangeHandler<String>() {
Line 159: @Override
Line 160: public void onValueChange(ValueChangeEvent<String> event)
{
Line 161: handler.onValueChange(new
ValueChangeEvent<T>(getValue()) {
1. I don't remember whether the value is actually set before or after the
ValueChangeEvent is fired, but getValue() here would be wrong in the latter
case. I think the right thing to do would be to use event.getValue().
2. This of course would be troublesome in case T is anything that isn't a
superclass of String, see my comment above.
Line 162: });
Line 163: }
Line 164: });
Line 165: }
--
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: 6
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: Lior Vernia <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
