Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Consolidate SortedListModel vs. SearchableListModel ......................................................................
Patch Set 1: (2 comments) 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); > > So I think here you only need to update the comparator and call setItems( What I meant originally was as Alex suggested, to just call setItems(getItems()) after updating the comparator. However, that wouldn't produce the desired result in the case of SearchableListModel. Calling sortItems() and then setItems() is no good as well, because items will be sorted twice for no good reason. So what should probably be done is: this.comparator = ... Collection<T> newItems = new ArrayList<T>(getItems()); setItems(newItems); And I would add a comment that the middle line is only required because of this artifact in SearchableListModel. 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; > My understanding is that if the comparator is null, items should not be sor Sure, easily. Not all ListModel instances hold collections of backend entities. HostBondInterfaceModel has a SortedListModel<String> member. If we were happy with the natural order of strings we wouldn't need to pass our own comparator, yet we would still want the SortedListModel to maintain sorting order. It's also quite legitimate to implement Comparable and store sorting logic within the class itself - one may even argue this is better from an OOD point of view. We usually don't make this choice because in many cases sorting depends on context, but NetworkItemModel for example chose to implement Comparable (and evidently it's not a backend entity). Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<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
