Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Consolidate SortedListModel vs. SearchableListModel ......................................................................
Patch Set 1: (5 comments) Haven't gone over test class, as we have some disagreement on the semantics of the class itself. http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java: Line 837: lastSelectedItems.add(item); Line 838: } Line 839: } Line 840: Line 841: if (useComparator(value)) { > Patch http://gerrit.ovirt.org/#/c/28392/ changed SortedListModel.sortItems Well, I can still call setItems() with a SortedSet argument, in which case I would know that a collection is already sorted. But I agree that this is an optimization that can be discarded. Line 842: Collection<T> sortedItems = sortItems(value); Line 843: itemsChanging(sortedItems, items); Line 844: items = sortedItems; Line 845: } else { http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java: Line 82: // Sort items after updating comparator Line 83: Collection<T> items = getItems(); Line 84: if (items != null) { Line 85: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : new ArrayList<T>(items); Line 86: super.setItems(maybeSortedItems); > > Can't you just call setItems(items) here, as what you are doing here is e I don't think there's any real good reason to wrap the items in a new ArrayList here. This is related to my comment below - I would not write code in SortedListModel just for the sake of SearchableListModel. And SearchableListModel already makes its own decision whether to call sortItems(), in its implementation of setItems(). So I think here you only need to update the comparator and call setItems(). Line 87: } Line 88: } Line 89: Line 90: @Override Line 88: } Line 89: Line 90: @Override Line 91: public void setItems(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; > Can you be more specific? What's wrong with useComparator() check? I think you should sort the items even if comparator is null, that's why I wouldn't call useComparator() here. Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { > I disagree, please see my comment above. Please see my comment below :) In my opinion, if this is needed anywhere, then it's only in SearchableListModel. Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 101: if (comparator == null) { > Yes, when comparator is null, Collections.sort will rely on natural orderin In my opinion, this would be a valid argument if this code were in SearchableListModel (which it basically is, since you only call sortItems() there if there's a comparator). Unlike SearchableListModel which is directly related to backend search queries, I don't think SortedListModel should care about the possibility of items already being sorted by the backend. Line 102: throw new IllegalStateException("comparator cannot be null"); //$NON-NLS-1$ Line 103: } Line 104: Line 105: List<T> sortedList = new ArrayList<T>(items); -- To view, visit http://gerrit.ovirt.org/31101 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id10b84c2fb476ed2240c33d72f6432335650df33 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
