Vojtech Szocs has posted comments on this change.
Change subject: webadmin: Overload ListModel.setItems() with selectedItem
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Guys, I'm giving +2 on this patch, I've tried to answer Gilad's question
(inline), to me it looks safe.
@Lior, small note - do you expect that "itemsChanged(T selectedItem)" method
can be overriden? If yes, overriding code must always call
"super.itemsChanged(selectedItem)" and there's no way to enforce that.
I suggest following for your consideration:
protected void itemsChanged(T selectedItem) {
List<T> selectedItems = new ArrayList<T>();
...
}
public void setItems(Collection<T> value, T selectedItem) {
...
if (selectedItem == null) {
itemsChanged();
} else {
itemsChanged(selectedItem);
}
...
}
This way, overriding "itemsChanged(T selectedItem)" without calling super won't
break original behavior, i.e. call to "itemsChanged()" if selectedItem is null
is always ensured.
http://gerrit.ovirt.org/#/c/25651/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ListModel.java
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ListModel.java:
Line 254: } else {
Line 255: List<T> selectedItems = new ArrayList<T>();
Line 256: selectedItems.add(selectedItem);
Line 257: setSelectedItem(selectedItem);
Line 258: setSelectedItems(selectedItems);
> Even before this patch, the user was already setting the selected item befo
If I understand Gilad's question correctly, he's asking why is this working -
we raise "SelectedItemChanged" event first, before raising "ItemsChanged"
event. The answer to this question lies in GWT ValueListBox implementation.
Here's an example:
* assume we're on the login screen, before domain combo is filled with values
* ListModel.setItems(newItems) is called from within GWT RPC result callback
* ListModel.itemsChanged() is called, which triggers "SelectedItemChanged" event
* UiCommonEditorVisitor's eventMap fires listener for "SelectedItemChanged"
event, which in turn calls ValueListBox.setValue(selectedItem), where
selectedItem = model.getSelectedItem()
* ValueListBox.setValue(newValue) calls ValueListBox.updateListBox() which ***
adds value if not present in <select> element ***
* after this, ListModel triggers "ItemsChanged" event
* UiCommonEditorVisitor's eventMap fires listener for "ItemsChanged" event,
which does following: (1) iterate model's items to find currently selected item
or fallback to first item, calls ValueListBox.setValue(selectedItem) -- (2)
call ValueListBox.setAcceptableValues(items) which *** clears <select> items
first *** and adds given values
So as you can see, processing both "SelectedItemChanged" + "ItemsChanged"
events yields:
* ValueListBox.setValue(selectedItem) -> due to "SelectedItemChanged"
* (again) ValueListBox.setValue(selectedItem) -> due to "ItemsChanged"
* ValueListBox.setAcceptableValues(items) -> due to "ItemsChanged"
Long story short, it should work OK.
Line 259: }
Line 260: }
Line 261:
Line 262: protected void itemsChanged()
--
To view, visit http://gerrit.ovirt.org/25651
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6de644297cdd5eb965720fc64aea1a5a735ea24
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
