Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Consolidate SortedListModel vs. SearchableListModel ......................................................................
Patch Set 1: (5 comments) Lior, sorry for replying so late, please see my comments. 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)) { > Well, I can still call setItems() with a SortedSet argument, in which case Yes, you can still call setItems() and pass SortedSet argument, but why would you do that? :-) My understanding is that using SortedListModel API (passing comparator, which is then wrapped inside SortSensitiveComparator) is the recommended and consistent way to do client-side sorting. In other words, SortedListModel should be the one containing comparator to use for client-side sorting. 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); > I don't think there's any real good reason to wrap the items in a new Array > So I think here you only need to update the comparator and call setItems(). I assume you mean something like: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; setItems(maybeSortedItems); If so, I can try to do that. If not, please correct me. 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; > I think you should sort the items even if comparator is null, that's why I My understanding is that if the comparator is null, items should not be sorted. Sorting items with null comparator: Collections.sort(items, null); means that items will still be sorted according to their natural ordering (defined by Comparable interface). If items don't implement Comparable, no sorting will occur. This implicit reliance on Comparable interface is something I really don't like. Today, we use backend business entities which expose their internal details - implementing Comparable by an entity is one such internal detail. In future, we won't use backend business entities, we'll use objects representing RESTful resources (essentially data holders) originating from JS context, so these won't implement Comparable and therefore the natural ordering has no use in such case. Can you give some example when would relying on natural ordering of items be useful? 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) { > Please see my comment below :) In my opinion, if this is needed anywhere, t If (and only if) we allow "comparator is null" for Collections.sort() then yes, this method could be removed and we could just check if "items is null". But, as you might already know, I'm not really a big fan on relying on natural ordering of items - please see my latest comment for line 92. 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) { > In my opinion, this would be a valid argument if this code were in Searchab Well, just like I wrote in my latest comment for line 92 - what's the actual benefit of supporting ordering based on item's natural ordering? Please don't get me wrong, in theory I agree, but in practice I really see no benefit. 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
