Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Fix client-side sort (ArrayList approach) ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/28392/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 68: Line 69: public final void setComparator(Comparator<? super T> comparator) { Line 70: setComparator(comparator, true); Line 71: } Line 72: See my comment concerning this method in the other patchset. At the moment the sorting will not remain stable across refreshes in tabs. Line 73: public void setComparator(Comparator<? super T> comparator, boolean sortAscending) { Line 74: this.comparator = (comparator != null) ? new SortSensitiveComparator<T>(comparator, sortAscending) : null; Line 75: } Line 76: Line 84: if (items == null || comparator == null) { Line 85: return items; Line 86: } Line 87: Line 88: List<T> sortedList = new ArrayList<T>() { The contract of List isn't compatible with what SortedListModel is supposed to do, due to methods like add(item, pos). SortedListModel maintains data that's constantly and consistently sorted according to the comparator, while add(item, pos) will allow a developer to mess with the ordering and break the contract and cause inconsistent behavior. The right thing to do would be to suggest this as an alternative internal implementation for SortedCollection from the other patch, but SortedListModel should still use something like SortedCollection that is consistent with its requirements. Line 89: Line 90: private static final long serialVersionUID = 499355412389856325L; Line 91: Line 92: @Override http://gerrit.ovirt.org/#/c/28392/2/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java: Line 16: import org.ovirt.engine.ui.uicommonweb.Configurator; Line 17: import org.ovirt.engine.ui.uicommonweb.ILogger; Line 18: import org.ovirt.engine.ui.uicommonweb.models.SortedListModel.SortSensitiveComparator; Line 19: Line 20: public class SortedListModelTest { See my comments in other patchset. You would discover that sorting isn't stable across calls to setItems(), etc. Line 21: Line 22: static class TestItem { Line 23: Line 24: private final int value; -- To view, visit http://gerrit.ovirt.org/28392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437e9dd7ff2afdb2d87fb0ba6c17dcb081cd965d Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[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
