Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Fix client-side sort (ArrayList approach) ......................................................................
Patch Set 1: (9 comments) http://gerrit.ovirt.org/#/c/28392/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java: Line 16: * Line 17: * @param <E> Line 18: * The type of element maintained by this collection. Line 19: */ Line 20: public class SortedCollection<E> implements Collection<E>, Serializable { There should be a test class to verify that this collection works as expected. I think it would have exposed the issues I raised. Line 21: Line 22: public interface PositionCallback<T> { Line 23: int getPosition(T item); Line 24: } Line 75: } Line 76: Line 77: @Override Line 78: public boolean contains(Object o) { Line 79: return items.contains(o); This may fail, as TreeSet implements contains() using only the compare() method (and thus actually breaks the Set contract, look it up), which we're tailoring to be different than equals(). It's better to check on the map. Line 80: } Line 81: Line 82: @Override Line 83: public Iterator<E> iterator() { Line 88: Line 89: @Override Line 90: public boolean hasNext() { Line 91: boolean hasNext = i.hasNext(); Line 92: if (!hasNext) { hasNext() shouldn't affect lastRet, especially not if it breaks the remove() contract. See comment below. Line 93: // Reference cleanup Line 94: lastRet = null; Line 95: } Line 96: return hasNext; Line 106: public void remove() { Line 107: i.remove(); Line 108: // In the edge case when user called next() and hasNext() returned false, Line 109: // lastRet was cleaned up so we can't remove the corresponding mapping from Line 110: // positions. This isn't very likely to happen, we prefer to avoid keeping This should work as specified in the Iterator.remove() contract, i.e. it should work independently of whether hasNext() was called. I'm not sure how you judge that this isn't likely to happen; it could happen whenever somebody tries to remove the last item (you have no knowledge, nor should you depend on, whether they called hasNext() prior to calling remove()). And if it does happen, nobody will understand why it isn't working. Line 111: // references to items within the iterator after hasNext() returned false. Line 112: if (lastRet != null) { Line 113: SortedCollection.this.positions.remove(lastRet); Line 114: lastRet = null; Line 144: } Line 145: Line 146: @Override Line 147: public boolean containsAll(Collection<?> c) { Line 148: return items.containsAll(c); Again, I would check this against the map. Line 149: } Line 150: Line 151: /** Line 152: * Retain original AbstractCollection.addAll method Line 165: return ret; Line 166: } Line 167: Line 168: @Override Line 169: public boolean removeAll(Collection<?> c) { The map should also be updated, probably via loop and calls to remove(). Line 170: // TreeSet delegates to AbstractCollection.removeAll method Line 171: // which removes elements one by one via collection's iterator Line 172: return items.removeAll(c); Line 173: } Line 172: return items.removeAll(c); Line 173: } Line 174: Line 175: @Override Line 176: public boolean retainAll(Collection<?> c) { Same here. Maybe it'd be best to subtract the collection from the map (I suspect subtraction won't work as expected with TreeSet due to its strange equality condition), then clear the TreeSet and add whichever items are still in the map. Line 177: // TreeSet delegates to AbstractCollection.retainAll method Line 178: // which removes elements one by one via collection's iterator Line 179: return items.retainAll(c); Line 180: } http://gerrit.ovirt.org/#/c/28392/1/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 85: Line 86: public final void setComparator(Comparator<? super T> comparator) { Line 87: setComparator(comparator, true); Line 88: } Line 89: When updating the comparator sortItems() should be called to make sure the existing collection is sorted according to new comparator. It should be moved from SearchableListModel here. This is important to maintain stability across refreshes, you may verify for yourself. Line 90: public void setComparator(Comparator<? super T> comparator, boolean sortAscending) { Line 91: this.comparator = (comparator != null) ? new SortSensitiveComparator<T>(comparator, sortAscending) : null; Line 92: } Line 93: http://gerrit.ovirt.org/#/c/28392/1/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 14: import org.ovirt.engine.ui.uicommonweb.Configurator; Line 15: import org.ovirt.engine.ui.uicommonweb.ILogger; Line 16: import org.ovirt.engine.ui.uicommonweb.models.SortedListModel.SortSensitiveComparator; Line 17: Line 18: public class SortedListModelTest { I am missing at least the following tests: 1. Preserved order across calls to setItems(), where the items are the same. 2. Stable ordering when setting a new comparator. Line 19: Line 20: static class TestItem { Line 21: Line 22: 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: 1 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
