Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Fix client-side sort (ArrayList approach) ......................................................................
Patch Set 1: (11 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 expect Originally there was SortedCollectionTest but later I've decided to have SortedListModelTest - same unit test for both ArrayList vs. TreeSet approach - demonstrating that both approaches work as expected. Aside from above, I agree that SortedCollection deserves its own unit test. > I think it would have exposed the issues I raised. Since SortedCollection is utilized by SortedListModel, you can modify SortedListModelTest to demonstrate the issues you're concerned about. Line 21: Line 22: public interface PositionCallback<T> { Line 23: int getPosition(T item); Line 24: } Line 38: Collection<? extends E> initialValues) { Line 39: this.items = new TreeSet<E>(comparator); Line 40: Line 41: initComparator(comparator); Line 42: initPositions(initialValues); > Actually, by initiating the positions Map each time you create a new Collec > Actually, by initiating the positions Map each time you create a new > Collection, I think stability across refreshes because the state of ordering > isn't preserved across calls to SortedListModel.sortItems(). Sorry, I don't understand.. on refresh, SortedListModel.sortItems() receives ArrayList deserialized from GWT RPC response and creates SortedCollection initialized with all of its items. This happens on each refresh, so assuming TreeSet's iterator yields same item order, this should be stable. SortedListModel.sortItems() is protected API and should be called once on each refresh if and only if the model has comparator assigned. With each refresh we get back "fresh" data, is it really meaningful or needed to remember item positions across multiple refreshes? > For this to be preserved, I think the positions map has to not be part of the > collection itself, as it needs to persist its state across creations of new > collections. Right, however as I mentioned in my comment: " custom SortedCollection implementation ensures that "items" and "positions" live in the same scope, this is important because we don't want "positions" to keep referencing individual items that are not needed anymore (preventing memory leaks) " We shouldn't be keeping references to "old" items, in this case for the purpose of remembering item positions across refreshes. This can lead to many problems including memory leaks. > In my original implementation, note that the positions map was NOT > initialized each time sortItems() was called. Correct, in your implementation, positions were initialized only if comparator changed. However, this can lead to situations like this one: * user sorts a column, comparator is set, positions are initialized * data refresh, positions are updated due to TreeSet.addAll() * data refresh again, same as above, repeat X times * positions prevents "old" items from being garbage collected (unless user sorts a column again..) > This same problem exists in the other alternative as well. As I mentioned above - is it really meaningful or needed to remember item positions across multiple refreshes? Doesn't each refresh mean "new" data? Why should we maintain item-specific state across multiple refreshes? Line 43: addAll(initialValues); Line 44: } Line 45: Line 46: SortedCollection(ComparatorWithPositionCallback<E> comparator) { 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() me > TreeSet implements contains() using only the compare() method (and thus > actually breaks the Set contract, look it up) TreeSet works with explicit comparator -or- comparable items, either of which define natural ordering of items within the set. Comparator/Comparable interfaces define implementation of uniqueness of items within the set. I don't understand your concern.. Anyway, yes, we could change this to: return positions.contains(o); because positions map is always in sync with TreeSet's items. BTW, your implementation doesn't override TreeSet.contains() so it also defaults to above mentioned behavior. 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( Please see my reply 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 sh > This should work as specified in the Iterator.remove() contract, i.e. it > should work independently of whether hasNext() was called. Conceptually, yes. Practically, we need to clear "lastRet" to avoid keeping reference to item from within the iterator. This is because a set doesn't have any notion of explicit item order, which is very important. The fact that TreeSet's iterator returns items in some order doesn't mean this is the order of the set. Conceptually, set has no item order, it's just a collection of unique items. > I'm not sure how you judge that this isn't likely to happen First of all, the typical way to work with list models (refresh logic) is to call setItems(collection) and NOT getItems().add(). The latter is not a typical way to work with list model. Second, if we didn't clear "lastRet" then we would be keeping reference to "last" item from within the iterator, which could mean memory leaks. If you don't agree with current implementation, what's your suggestion here? As you know, set has no notion of item order in theory, so we can't have something like "lastRetIndex" because set doesn't provide indexOf(element) anyway. 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. Yes, I am not against it. Both items and positions should be in sync anyway. 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(). As I wrote in comment below, TreeSet.removeAll() delegates to AbstractCollection.removeAll() which uses iterator to remove items. So the positions map IS updated via iterator.remove() calls. 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 su Again, as I wrote in comment below, TreeSet.retainAll() delegates to AbstractCollection.removeAll() which uses iterator to remove items. So the positions map IS updated via iterator.remove() calls. What's the advantage of providing custom behavior vs. just calling super? 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 > 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. The behavior you described existed before in SearchableListModel.setComparator(comparator,boolean) override. It did NOT exist in SortedListModel before, so if we did it like you describe, we would be changing existing behavior. This patch is not intended to change existing behavior, it's intended to fix the known issue. We can do the change you described in a separate patch. What do you think? 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 { > Refinement to article 1: same items but different ordering (which is what c I don't agree on preserving ordering across multiple setItems() calls as we get "new" items on each refresh. Line 19: Line 20: static class TestItem { Line 21: Line 22: private final int value; 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. I don't think we should keep item-specific state across multiple refreshes (represented by multiple setItems() calls) because on each refresh we get "new" data. > 2. Stable ordering when setting a new comparator. This would mean we need to track positions map on SortedListModel level instead of SortedCollection level. This implies that positions would have to be initialized in setItems() to avoid memory leaks. What is your opinion here? 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: 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
