Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 8: (3 inline comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/BaseListModelSuggestBox.java
Line 23: public abstract class BaseListModelSuggestBox<T> extends Composite
implements EditorWidget<T, TakesConstrainedValueEditor<T>>,
HasConstrainedValue<T> {
Line 24:
Line 25: private TakesConstrainedValueEditor<T> editor;
Line 26:
Line 27: private T value;
I'm sorry Tomas, but I have to disagree. The fact that ListModelSuggestBox
holds String is no coincidence, it is so because the user may input text freely
and not just out of a set of constrained value. That is a very general
property. Many other widget using a SuggestBox might have this property in the
future.
I further disagree about this solution being more generic. At the moment you
have one base class, and two derived classes. Half of them don't need this,
half of them do. You have no way of knowing if more widgets using SuggestBox in
the future will be of one type or another. So a solution that only fits have
the derived classes should not reside in the base class.
Line 28:
Line 29: private SuggestBox suggestBox;
Line 30:
Line 31: private ListModelSuggestionDisplay suggestionDisplay = new
ListModelSuggestionDisplay();
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) {
Accepted the argument.
Line 37: @Override
Line 38: public void setText(String text) {
Line 39: super.setText(text);
Line 40:
Line 152: return value;
Line 153: }
Line 154:
Line 155: @Override
Line 156: public HandlerRegistration addValueChangeHandler(final
ValueChangeHandler<T> handler) {
My general argument is that currently half of the existing subclasses don't
need it, and they don't need it for a good reason. They're a type of widget
that shouldn't hold anything but String. So for that entire half of the
inheritance tree, you'll just have no-op methods, which would just baffle
anybody looking at those classes.
I think we have been able to define two distinct types of behaviour here - one
that chooses between a set of constrained value and can be generic, and another
that allows also free input that will always have to hold String. I think these
two behaviours are distinct enough to have two sets of behaviours upon value
change. I don't see a strong argument for them to share methods that will do
nothing for one of them.
Line 157: this.handler = handler;
Line 158: return asSuggestBox().addValueChangeHandler(new
ValueChangeHandler<String>() {
Line 159: @Override
Line 160: public void onValueChange(ValueChangeEvent<String> event)
{
--
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]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
