Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 8: (12 inline comments)
Please accept my sincere apology, I've come to terms with the base class, just
not in its current form. Please see inline comments.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/BaseListModelSuggestBox.java
Line 29: private SuggestBox suggestBox;
Line 30:
Line 31: private ListModelSuggestionDisplay suggestionDisplay = new
ListModelSuggestionDisplay();
Line 32:
Line 33: private ValueChangeHandler<T> handler;
This won't be needed, see below. Anyway, it would have been problematic to only
allow one handler.
Line 34:
Line 35: public BaseListModelSuggestBox(MultiWordSuggestOracle
suggestOracle) {
Line 36: suggestBox = new SuggestBox(suggestOracle, new TextBox(),
suggestionDisplay) {
Line 37: @Override
Line 32:
Line 33: private ValueChangeHandler<T> handler;
Line 34:
Line 35: public BaseListModelSuggestBox(MultiWordSuggestOracle
suggestOracle) {
Line 36: suggestBox = new SuggestBox(suggestOracle, new TextBox(),
suggestionDisplay) {
I suspect this also affects the display of ListModelSuggestBox. Perhaps pass it
along to the base class constructor, and then have ListModelSuggestBox pass the
default and ListModelTypeAheadListBox pass the custom one?
Line 37: @Override
Line 38: public void setText(String text) {
Line 39: super.setText(text);
Line 40:
Line 34:
Line 35: public BaseListModelSuggestBox(MultiWordSuggestOracle
suggestOracle) {
Line 36: suggestBox = new SuggestBox(suggestOracle, new TextBox(),
suggestionDisplay) {
Line 37: @Override
Line 38: public void setText(String text) {
I don't think this is needed by both subclasses. You could add a listener in
your ListModelTypeAheadListBox to the actual SuggestBox/TextBox
ValueChangeEvent<String>, and avoid the need to define the postTextSet() method.
Line 39: super.setText(text);
Line 40:
Line 41: postTextSet(text);
Line 42: }
Line 121: public void setEnabled(boolean enabled) {
Line 122: asTextBox().setEnabled(enabled);
Line 123: }
Line 124:
Line 125: public void setValue(T value) {
I think these methods should just perform
asSuggestBox().setValue(renderer.render(value)). See long comment below about
how to handle events, which won't require this.
Line 126: if (value != null && value != getValue()) {
Line 127: handler.onValueChange(new ValueChangeEvent<T>(value) {
Line 128: });
Line 129: }
Line 140: this.value = value;
Line 141: render(value, fireEvents);
Line 142: }
Line 143:
Line 144: protected abstract void render(T value, boolean fireEvents);
It's customary to pass a renderer to the constructor rather than have a method
for it. Also, strange for rendering to fire events, it's simply choosing a
String to represent an object.
Line 145:
Line 146: protected void postTextSet(String text) {
Line 147: // by default do nothing
Line 148: }
Line 142: }
Line 143:
Line 144: protected abstract void render(T value, boolean fireEvents);
Line 145:
Line 146: protected void postTextSet(String text) {
As I said, don't think this is needed.
Line 147: // by default do nothing
Line 148: }
Line 149:
Line 150: @Override
Line 152: return value;
Line 153: }
Line 154:
Line 155: @Override
Line 156: public HandlerRegistration addValueChangeHandler(final
ValueChangeHandler<T> handler) {
Upon ValueChangeEvent<String> you would want to fire a ValueChangeEvent<T> to
represent the change in the T value, so you have to know how to translate from
String to T. ListModelListBox does it through GWT's ValueListBox, which
translates according to index. You don't have indices, so you would need some
sort of Map<String,T>.
I would do that by implementing setAcceptableValues() in this base class, and
in its implementation construct the map using renderer.render() for the map
keys. The renderer for ListModelSuggestBox should just cast.
Then when a ValueChangeHandler<T> is passed here, I would add a
ValueChangeHandler<String> to the SuggestBox that calls handler(map.get(value)).
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 163: }
Line 164: });
Line 165: }
Line 166:
Line 167: class ListModelSuggestionDisplay extends DefaultSuggestionDisplay
{
This should reside in ListModelTypeAheadListBox and passed as argument to the
constructor, as it's not shared by both widgets.
Line 168:
Line 169: public ListModelSuggestionDisplay() {
Line 170: // not not be hidden under the panel
Line 171: getPopupPanel().getElement().getStyle().setZIndex(1);
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelSuggestBoxEditor.java
Line 9: *
Line 10: * @param <T>
Line 11: * SuggestBox item type.
Line 12: */
Line 13: public class ListModelSuggestBoxEditor<T> extends
AbstractValidatedWidgetWithLabel<T, ListModelSuggestBox<T>>
Please revert to non-generic as in ListModelSuggestBox.
Line 14: implements IsEditor<WidgetWithLabelEditor<T,
ListModelSuggestBoxEditor<T>>> {
Line 15:
Line 16: private final WidgetWithLabelEditor<T,
ListModelSuggestBoxEditor<T>> editor;
Line 17:
....................................................
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<T> extends BaseListModelSuggestBox<T>
{
So now I agree that the base class could be generic, but this shouldn't be, as
free text can be input here - it shouldn't hold anything but String or Object,
so no need to confuse future developers. Please extend Base...<Object>.
Line 15:
Line 16: public ListModelSuggestBox() {
Line 17: super(new MultiWordSuggestOracle());
Line 18: initWidget(asSuggestBox());
Line 27: }
Line 28:
Line 29: @Override
Line 30: public void setAcceptableValues(Collection<T> values) {
Line 31: Collection<String> stringValues = Linq.cast(values);
If you implement setAcceptableValues() in base class as suggested, this would
be unnecessary.
Line 32: MultiWordSuggestOracle suggestOracle =
(MultiWordSuggestOracle) asSuggestBox().getSuggestOracle();
Line 33: suggestOracle.clear();
Line 34: suggestOracle.addAll(stringValues);
Line 35: suggestOracle.setDefaultSuggestionsFromText(stringValues);
Line 36: }
Line 37:
Line 38: @Override
Line 39: protected void render(T value, boolean fireEvents) {
Line 40: asSuggestBox().setValue((String) value, fireEvents);
This would be unnecessary too, just pass renderer to constructor.
Line 41: }
Line 42:
--
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: 8
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]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
