Tomas Jelinek has posted comments on this change.

Change subject: userportal,webadmin: type ahead list box
......................................................................


Patch Set 8: (2 inline comments)

> 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.

No, I do not create business entities in the widget - it would be really a 
design flaw. The widget only gets a list of possible business entities and you 
can select one of them according to their name using type ahead (the same way 
as in classic list box, but this one supports also the type ahead).

> 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.

well, you are right - it would be better to extend directly SuggestBox. 
Unfortunately it is not possible (see my comment in BaseListModelSuggestBox - 
Daniel asked the same).

But I guess your main concern was that you have misunderstood how the 
ListModelTypeAheadListBox works. Now as I have clarified you that it does not 
create business entities but uses exactly the same principle as the e.g. 
ListModelListBox just visualizes it differently are you ok with it?

When we will have an agreement on this we can discuss the specific details 
about event firing.

....................................................
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> 
{
not true. I'm not creating the business entity from String. Please have a 
closer look at the ListModelTypeAheadListBox. It is exactly the same principle 
as the ListModelSuggestBox just the way how you select from the possible values 
is different (typing with type ahead or selecting from the list).
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);
the javadoc in this class. Yes, you are right, this is the contract of this 
class - it edits only Strings. It is the original code I did not change to not 
touch original code too much.
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

Reply via email to