Alexander Wels has posted comments on this change. Change subject: webadmin: Render client-side sorting more robust ......................................................................
Patch Set 3: (1 comment) http://gerrit.ovirt.org/#/c/28268/3/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 90: } Line 91: Line 92: insertionIndex.clear(); Line 93: SortedSet<T> sortedItems = new TreeSet<T>(comparator) { Line 94: private int counter = 0; > I think all implementations of addAll() call add(), and same with removeAll I actually checked, and the implementation of ArrayList and TreeSet do NOT call add in addAll. If I call remove on an iterator, does it call remove on the class or something else to remove the data from the set. I don't think there is any way we can guarantee what the iterator calls our overriden method. Those are implementation details we should not be concerned about. If our comparator in the end always defaults to the natural sort by doing for instance a Collection.sort(items, comparator). the result will be stable across refreshes, as the refresh will call setItems which will re-apply the sort, the same way your code will do it. When I say at this point, we have added a new Map, we have added some computations to keep the Map in sync with the TreeSet. One of the 'benefits' of the TreeSet was to keep costs for sorting it low, but with all the extra overhead is that cost still low? If you don't keep the Map in perfect sync with the TreeSet the sort is not stable at all as it is dependent on the state of Map. And I am questioning if the overhead of synchronizing the map with the Set is worth all the effort. Line 95: Line 96: @Override Line 97: public boolean add(T e) { Line 98: insertionIndex.put(e, counter++); // item will be placed following all existing "equal" items -- To view, visit http://gerrit.ovirt.org/28268 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iffd1ce7763e516ad109fdae5d584b183ee44117c Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Lior Vernia <[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
