Lior Vernia has posted comments on this change.
Change subject: userportal,webadmin: type ahead list box
......................................................................
Patch Set 6: (3 inline comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/BaseListModelSuggestBox.java
Line 121: public void setEnabled(boolean enabled) {
Line 122: asTextBox().setEnabled(enabled);
Line 123: }
Line 124:
Line 125: public void setValue(T value) {
The meaning of not firing events is usually that you don't want listening
clients to be notified. If you're notifying a model that the value has changed
when a developer asked not to fire the event, then you're abusing the mechanism.
Line 126: if (value != null && value != getValue()) {
Line 127: handler.onValueChange(new ValueChangeEvent<T>(value) {
Line 128: });
Line 129: }
Line 133: }
Line 134:
Line 135: public void setValue(T value, boolean fireEvents) {
Line 136: if (value != null && value != getValue()) {
Line 137: handler.onValueChange(new ValueChangeEvent<T>(value) {
Same as above. You shouldn't be firing events without even checking whether
fireEvents is true.
Line 138: });
Line 139: }
Line 140: this.value = value;
Line 141: render(value, fireEvents);
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 161: handler.onValueChange(new
ValueChangeEvent<T>(getValue()) {
1. There's good reason why the event.getValue() method returns a String.
Because that's the event - the String has changed, not the object T. And you're
completely ignoring the new String value here. Maybe it works for your widget
if you implemented postTextSet(), but you did nothing for ListModelSuggestBox
so it must be broken under these conditions.
2. This should be another hint that it doesn't make sense to have T as
non-String. Again, it looks like abuse of the events mechanism. What is
triggered when a user inputs text into the widget is onValueChange. Then
listeners to this change can check the new value and decide what to do with it.
Instead of using this mechanism, you're ignoring it, and perhaps bypassing it
using postTextSet().
Line 162: });
Line 163: }
Line 164: });
Line 165: }
--
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: 6
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
