Tomas Jelinek has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 8: (10 inline comments)
> I still think it might be worth considering to inherit from ListModelListBox,
> it would make about as much sense.
not really - have a look at all the asListBox().addABC calls. It is bound to
ListBox not to suggest box. It would be theoretically possible to
hack it somehow to use the underlying list box while hiding it, but anyway all
the logic would have to be in the suggest box implementation (remember,
the indexes in the list box are constant while in suggest box changing as you
type). A little bit less problematic would be to inherit the ValueListBox
but I would face the same issues but in smaller range as the class is smaller.
....................................................
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;
removed
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) {
- it does affect, but in a good way - the suggestions will never hide under the
rest of the elements (see suggestionDisplay z-index)
- I don't see the problem having this common. It shares the common
functionality (z-index setting)
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) {
removed - changed the ValueChangeHandler to find the corresponding entity to
the String from the event.
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) {
- They also have to remember the new value.
- You are right, they don't need to fire the event. Plz see the new
implementation of the value change handler
- The render - I like more the abstract render method here - I find it more
readable for the implementators than to pass the renderer (especially an
artificial one for the ListModelSuggestBox
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);
But the ListModelSuggestBox does not use a renderer and I find it more readable
to have a simple abstract method which renders the given value somehow than to
create a one line class for it. I'm not strictly against, but I like more the
method here :)
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) {
removed
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) {
well, the problem here is that if it was implemented with the map the values
which are not part of the acceptable ones would not be handled at all (just
returned null). But, ListModelTypeAheadListBox expects to return to the
previous selected (or empty if none was) and ListModelSuggestBox just returns
the value which is not part of the accepted ones and validates it later in the
model. The solution with the map would support neither of this. And especially
would be unable to distinguish the two different expected behaviors.
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
{
actually it is shared - at least the
getPopupPanel().getElement().getStyle().setZIndex(1); is relevant for both. And
to have a base micro class and extend it would be overengineered according to
me.
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>>
ok, I can do that
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>
{
Actually only String but we have to use Object or T which has anyway to be
Object because of editor widget and not-generic model... But OK, I can change
it to BaseListModelSuggestBox<Object> even I don't think it will bring much
clarity to the fact that only Strings are supported ;)
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: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
