From 78e5cb1a9606402ff087196cc15cbcf7f17263ac Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 9 Feb 2015 16:25:24 +0200 Subject: [PATCH] Fix RpcDataProviderExtension value change listener setup (#16550) This patch changes value change listener mapping from itemid based to index based mapping. This makes removing rows much less error prone Change-Id: I77e9078de4ae61ce5d752cc394aa47bccd505e70 --- .../vaadin/data/RpcDataProviderExtension.java | 86 ++++++++++++------- .../server/GridStructureTest.java | 12 +++ 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java index 354bfe336c..3acd12d863 100644 --- a/server/src/com/vaadin/data/RpcDataProviderExtension.java +++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java @@ -325,10 +325,10 @@ public class RpcDataProviderExtension extends AbstractExtension { */ private class ActiveRowHandler implements Serializable { /** - * A map from itemId to the value change listener used for all of its + * A map from index to the value change listener used for all of column * properties */ - private final Map valueChangeListeners = new HashMap(); + private final Map valueChangeListeners = new HashMap(); /** * The currently active range. Practically, it's the range of row @@ -388,36 +388,27 @@ public class RpcDataProviderExtension extends AbstractExtension { } private void addValueChangeListeners(Range range) { - for (int i = range.getStart(); i < range.getEnd(); i++) { + for (Integer i = range.getStart(); i < range.getEnd(); i++) { final Object itemId = container.getIdByIndex(i); final Item item = container.getItem(itemId); - if (valueChangeListeners.containsKey(itemId)) { - /* - * This might occur when items are removed from above the - * viewport, the escalator scrolls up to compensate, but the - * same items remain in the view: It looks as if one row was - * scrolled, when in fact the whole viewport was shifted up. - */ - continue; - } + assert valueChangeListeners.get(i) == null : "Overwriting existing listener"; GridValueChangeListener listener = new GridValueChangeListener( itemId, item); - valueChangeListeners.put(itemId, listener); + valueChangeListeners.put(i, listener); } } private void removeValueChangeListeners(Range range) { - for (int i = range.getStart(); i < range.getEnd(); i++) { - final Object itemId = container.getIdByIndex(i); + for (Integer i = range.getStart(); i < range.getEnd(); i++) { final GridValueChangeListener listener = valueChangeListeners - .remove(itemId); + .remove(i); - if (listener != null) { - listener.removeListener(); - } + assert listener != null : "Trying to remove nonexisting listener"; + + listener.removeListener(); } } @@ -478,12 +469,16 @@ public class RpcDataProviderExtension extends AbstractExtension { */ public void insertRows(int firstIndex, int count) { if (firstIndex < activeRange.getStart()) { + moveListeners(activeRange, count); activeRange = activeRange.offsetBy(count); } else if (firstIndex < activeRange.getEnd()) { - final Range deprecatedRange = Range.withLength( - activeRange.getEnd(), count); - removeValueChangeListeners(deprecatedRange); - + int end = activeRange.getEnd(); + // Move rows from first added index by count + Range movedRange = Range.between(firstIndex, end); + moveListeners(movedRange, count); + // Remove excess listeners from extra rows + removeValueChangeListeners(Range.withLength(end, count)); + // Add listeners for new rows final Range freshRange = Range.withLength(firstIndex, count); addValueChangeListeners(freshRange); } else { @@ -509,23 +504,52 @@ public class RpcDataProviderExtension extends AbstractExtension { public void removeRows(int firstIndex, int count) { Range removed = Range.withLength(firstIndex, count); if (removed.intersects(activeRange)) { - final Range deprecated = removed.restrictTo(activeRange); - for (int i = deprecated.getStart(); i < deprecated.getEnd(); ++i) { - Object itemId = keyMapper.itemIdAtIndex(i); - // Item doesn't exist anymore. - valueChangeListeners.remove(itemId); - } + final Range[] deprecated = activeRange.partitionWith(removed); + // Remove the listeners that are no longer existing + removeValueChangeListeners(deprecated[1]); + // Move remaining listeners to fill the listener map correctly + moveListeners(deprecated[2], -deprecated[1].length()); activeRange = Range.withLength(activeRange.getStart(), - activeRange.length() - deprecated.length()); + activeRange.length() - deprecated[1].length()); + } else { if (removed.getEnd() < activeRange.getStart()) { /* firstIndex < lastIndex < start */ + moveListeners(activeRange, -count); activeRange = activeRange.offsetBy(-count); } /* else: end <= firstIndex, no need to do anything */ } } + + /** + * Moves value change listeners in map with given index range by count + */ + private void moveListeners(Range movedRange, int diff) { + if (diff < 0) { + for (Integer i = movedRange.getStart(); i < movedRange.getEnd(); ++i) { + moveListener(i, i + diff); + } + } else if (diff > 0) { + for (Integer i = movedRange.getEnd() - 1; i >= movedRange + .getStart(); --i) { + moveListener(i, i + diff); + } + } else { + // diff == 0 should not happen. If it does, should be no-op + return; + } + } + + private void moveListener(Integer oldIndex, Integer newIndex) { + assert valueChangeListeners.get(newIndex) == null : "Overwriting existing listener"; + + GridValueChangeListener listener = valueChangeListeners + .remove(oldIndex); + assert listener != null : "Moving nonexisting listener."; + valueChangeListeners.put(newIndex, listener); + } } /** @@ -663,7 +687,7 @@ public class RpcDataProviderExtension extends AbstractExtension { * taking all the corner cases into account. */ - Map listeners = activeRowHandler.valueChangeListeners; + Map listeners = activeRowHandler.valueChangeListeners; for (GridValueChangeListener listener : listeners.values()) { listener.removeListener(); } diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java index 08f903b3fe..e83031f227 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java +++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java @@ -232,6 +232,18 @@ public class GridStructureTest extends GridBasicFeaturesTest { .findElements(By.tagName("tr")).size()); } + @Test + public void testRemoveFirstRowTwice() { + openTestURL(); + + selectMenuPath("Component", "Body rows", "Remove first row"); + selectMenuPath("Component", "Body rows", "Remove first row"); + + getGridElement().scrollToRow(50); + assertFalse("Listener setup problem occurred.", + logContainsText("AssertionError: Value change listeners")); + } + @Test public void testVerticalScrollBarVisibilityWhenEnoughRows() throws Exception { -- 2.39.5