Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 8: (2 inline comments)
Yes, you have neglected to respond to my comments in patchset 6 which continued
our discussion. My main concern about the design is still the same - it doesn't
make sense for a widget to create business entities based on a String. This is
not the view's job, but the model's. This is a significant design flaw.
In BaseListModelSuggestBox you have included code which not only isn't shared
by both children, but actually goes against the logic of the
ListModelSuggestBox. In my opinion, you are forcing the inheritance.
Your animals argument is misapplied. If both widgets use a SuggestBox, then
they should definitely both delegate to the GWT SuggestBox class. That's code
reuse just the same. However, they don't have to inherit from a common
BaseListModelSuggestBox as you had implemented it.
....................................................
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>
{
No, the principle is not the same, because there the values are constrained.
Here the value can be input freely by the user, and you would have to create a
business entity in the view according to a String. Which is not the view's job,
it's the model's job.
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);
I don't understand. This will not work if T can't be cast to String. Could you
point me to the javadoc?
Line 32: MultiWordSuggestOracle suggestOracle =
(MultiWordSuggestOracle) asSuggestBox().getSuggestOracle();
Line 33: suggestOracle.clear();
Line 34: suggestOracle.addAll(stringValues);
Line 35: suggestOracle.setDefaultSuggestionsFromText(stringValues);
--
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
