Lior Vernia has posted comments on this change. Change subject: webadmin: Render client-side sorting more robust ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/28268/2/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 25: Line 26: @Override Line 27: public int compare(T a, T b) { Line 28: int res = sortAscending ? comparator.compare(a, b) : comparator.compare(b, a); Line 29: return res != 0 ? res : Integer.signum(positions.get(b) - positions.get(a)); > I'd rather be more defensive concerning "positions" map, for example: You are correct on both accounts. However, current fix should make it impossible for the Map to always hold a corresponding Integer. Second issue was a lapse in attention :) Line 30: } Line 31: Line 32: @Override Line 33: public boolean equals(Object obj) { Line 58: Line 59: } Line 60: Line 61: protected Comparator<? super T> comparator; Line 62: private final Map<T, Integer> positions = new HashMap<T, Integer>(); > Why are we using a map with integer indexes? Wouldn't a regular list work j The Map provides reverse mapping, quick access from member to index. Line 63: Line 64: public SortedListModel(Comparator<? super T> comparator) { Line 65: super(); Line 66: setComparator(comparator); Line 88: if (items == null || comparator == null) { Line 89: return items; Line 90: } Line 91: Line 92: initPositions(items); > So how is doing getItems().add(item) going to work in this scenario? it obv Very good comment, fixed to maintain map and make sure new items are added following existing "equal" items. Used the fact that it isn't in fact necessary to hold exact positions, just a number that increases monotonously as items are inserted. Note that we could have put new items anywhere we wished among "equal" items, because if the comparator doesn't discern between them it means that it doesn't matter. Moreover, since they hadn't previously existed, the desired behavior is also undefined concerning the "stable" sorting argument, so again we still could do whatever we wanted. The code would have been simpler that way, as we could have fallen back to sorting by hashcode for example. However, adding items in the end is probably the most predictable behavior. Line 93: SortedSet<T> sortedItems = new TreeSet<T>(comparator); Line 94: sortedItems.addAll(items); Line 95: return sortedItems; Line 96: } -- 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: 2 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
