Tomas Jelinek has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 8: Verified
(3 inline comments)
> It doesn't make sense for a ListModelSuggestBox to be
> generic
plz see my comment
> and the implementation of BaseListModelSuggestBox looks
> fishy to me as well.
Could you be more specific? It is hard to fix my code according to this kind of
comment.
> Maybe TypeAheadListBox should just be a separate widget
> and not share a class ancestry with ListModelSuggestBox.
They are doing nearly the same thing - copy pasting the common code would not
be a good way to go.
> Also, has this been verified?
yes, putting verified flag to make this explicit
> It looks to me like it might have broken functionality in
> HostBondPopupView that was using ListModelSuggestBox.
Why do you think? How exactly? Because I was trying hard to not change the
existing functionality -
that is why did I let the non generic implementation just for HostBondPopupView
and let it's behavior exactly how it was.
....................................................
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>
{
> but it definitely doesn't make sense for ListModelSuggestBox
well, this is the original legacy code I was used but did not want to change
too much. It indeed models only strings (see javadoc). I could change it to ...
extends BaseListModelSuggestBox<String>, but it would break the editor driver.
So I can do ...extends BaseListModelSuggestBox<Object> which would not be too
much of help. But if you really want, I can use "...extends
BaseListModelSuggestBox<Object>", but in the future it should be changed anyway
to work with entities.
> I still think that this does not make sense for BaseListModelSuggestBox
If you are still not convinced by this widget, please have a look at some other
in oVirt (e.g. ListModelListBox is similar but has simpler task because the
user can not type free text. The principle is the same however).
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);
sure, documented in javadoc
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);
same
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
