Lior Vernia 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 actually checked, and the implementation of ArrayList and TreeSet do NOT Alex, before we talk about technical details we should clarify what is and what isn't the objective of this patch. If I understand correctly, you think that it is primarily about preventing duplicate values, and that the alternative is to substitute the SortedSet by a List. That is not true. If this were the primary objective, then it would have been enough to squeeze in derez's hashcode fallback into the compare() override above. The primary goal of this patch is to make sorting "stable". I have described in length what that means and why I think it would benefit UX; in comments here, in the commit message of the patch and in the mailing list. You can agree or disagree on its importance, but so far you haven't argued against it, you've just ignored it. For this purpose I'm using a Map. As mentioned, otherwise it wouldn't have been required. Replacing SortedSet with a List would do nothing to maintain this stability. A list would indeed maintain consistency across refreshes but not while maintaining consistency with previous sorting preferences of a user. You would need to somehow take care of that to achieve the same goals. And I suspect it would involve a Map as well. However, as a secondary benefit, this patch should also eliminates the chance of duplicates and therefore obviate the need to replace the SortedSet with a List. Now for technicalities. I don't think the Map has to be kept in perfect sync with the contents of the TreeSet for things to work, I only overrode remove() to keep memory usage minimal. If I'm not mistaken, the only thing required is an increasing counter and some updates on add(). There was indeed an issue with addAll(), thanks for turning my attention to that, it should now be fixed. And you do have a point about the iterators as well, so I think I have let go of my attempts to keep track of deletions (as they are not extremely important). 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
