Vojtech Szocs 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 Well, as I mentioned before, for me each refresh yields "new" items and therefore it doesn't make sense to maintain positions across multiple refreshes. 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>() { > Also, similarly to the comment I added in the other patchset, this data str > 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. IF the developer uses methods like add(item, pos) he is doing that on purpose and must expect that this breaks the item ordering. > 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. First of all, list models work with Collection interface and NOT with List interface. Sure, there are some hidden casts like (List) getItems() but these are just wrong. I see your point, but what should happen if developer calls add(item, pos)? Should we just ignore "pos" and call add(item)? Anyway, this is irrelevant from Collection interface perspective. > Also, similarly to the comment I added in the other patchset, this data > structure can't maintain stable ordering across refreshes as it doesn't refer > to the previous collection of items each time new items are passed. I don't think that keeping positions across multiple refreshes is a good idea in general (as I mentioned multiple times). Each refresh = "new" items. 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 st Yes, but.. why is keeping item-specific state (positions) across refreshes needed in the first place? :-) 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: Daniel Erez <[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
