changeset 9519:64f4605ad7b7

8089709: ListView: selectedIndices fires incorrect change on items modification
author jgiles
date Tue, 19 Jan 2016 11:02:33 +1300
parents 23da5bee7c09
children b43a5bd9cbb1
files modules/controls/src/main/java/com/sun/javafx/scene/control/MultipleAdditionAndRemovedChange.java modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeViewBehavior.java modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java modules/controls/src/test/java/test/javafx/scene/control/ListViewTest.java modules/controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java modules/controls/src/test/java/test/javafx/scene/control/TreeViewTest.java
diffstat 7 files changed, 224 insertions(+), 53 deletions(-) [+]
line wrap: on
line diff
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/MultipleAdditionAndRemovedChange.java	Tue Jan 19 11:02:33 2016 +1300
@@ -0,0 +1,136 @@
+/*
+ * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package com.sun.javafx.scene.control;
+
+import javafx.collections.ListChangeListener;
+import javafx.collections.ObservableList;
+
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Often times there are multiple additions that are applied to a list, but they are not contiguous. This class
+ * abstracts away the details and creates the right number of next() iterations to correctly report all changes.
+ *
+ * @param <E> The element contained in the underlying list data structure
+ */
+public class MultipleAdditionAndRemovedChange<E> extends ListChangeListener.Change<E> {
+
+    private static final int[] EMPTY_PERM = new int[0];
+
+    private boolean invalid = true;
+
+    private final List<E> addedElements;
+    private final List<E> removedElements;
+
+    private boolean iteratingThroughAdded = true;
+    private boolean returnedRemovedElements = false;
+    private int addedIndex = 0;
+
+    private int from;
+    private int to;
+
+    public MultipleAdditionAndRemovedChange(List<E> addedElements, List<E> removedElements, ObservableList<E> list) {
+        super(list);
+        this.addedElements = addedElements;
+        this.removedElements = removedElements;
+    }
+
+    @Override public boolean next() {
+        if (invalid) {
+            invalid = false;
+        }
+
+        // we firstly work through all additions, finding contiguous results and grouping them together
+        if (addedIndex < addedElements.size()) {
+            from = getList().indexOf(addedElements.get(addedIndex));
+            to = from + 1;
+
+            addedIndex++;
+            for (int i = 0; (i + addedIndex) < addedElements.size(); i++) {
+                // check to see if the next element in the added list is also the next element in the actual list,
+                // stop when this is no longer true.
+                E nextElement = addedElements.get(i + addedIndex);
+                if (nextElement != getList().get(from + i)) {
+                    to = from + 1 + i;
+                    addedIndex = addedIndex + i;
+                    break;
+                }
+            }
+
+            return true;
+        } else if (!returnedRemovedElements) {
+            returnedRemovedElements = true;
+            iteratingThroughAdded = false;
+            from = 0;
+            to = 0;
+
+            return !removedElements.isEmpty();
+        }
+
+        return false;
+    }
+
+    @Override public void reset() {
+        invalid = true;
+        from = 0;
+        to = 0;
+        addedIndex = 0;
+        iteratingThroughAdded = true;
+        returnedRemovedElements = false;
+    }
+
+    @Override public int getFrom() {
+        checkState();
+        return from;
+    }
+
+    @Override public int getTo() {
+        checkState();
+        return to;
+    }
+
+    @Override public List<E> getRemoved() {
+        return iteratingThroughAdded ? Collections.<E>emptyList() : removedElements;
+    }
+
+    @Override protected int[] getPermutation() {
+        return EMPTY_PERM;
+    }
+
+    @Override public boolean wasAdded() {
+        return iteratingThroughAdded && !addedElements.isEmpty();
+    }
+
+    @Override public boolean wasRemoved() {
+        return !iteratingThroughAdded && !removedElements.isEmpty();
+    }
+
+    private void checkState() {
+        if (invalid) {
+            throw new IllegalStateException("Invalid Change state: next() must be called before inspecting the Change.");
+        }
+    }
+}
--- a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java	Tue Jan 19 11:02:33 2016 +1300
@@ -259,10 +259,13 @@
     private boolean selectionChanging = false;
 
     private final ListChangeListener<Integer> selectedIndicesListener = c -> {
+        int newAnchor = getAnchor();
+
         while (c.next()) {
             if (c.wasReplaced()) {
                 if (ListCellBehavior.hasDefaultAnchor(getNode())) {
                     ListCellBehavior.removeAnchor(getNode());
+                    continue;
                 }
             }
 
@@ -273,28 +276,38 @@
             // there are no selected items, so lets clear out the anchor
             if (! selectionChanging) {
                 if (sm.isEmpty()) {
-                    setAnchor(-1);
+                    newAnchor = -1;
                 } else if (hasAnchor() && ! sm.isSelected(getAnchor() + shift)) {
-                    setAnchor(-1);
+                    newAnchor = -1;
                 }
             }
 
-            int addedSize = c.getAddedSize();
-            if (addedSize > 0 && ! hasAnchor()) {
-                List<? extends Integer> addedSubList = c.getAddedSubList();
-                int index = addedSubList.get(addedSize - 1);
-                setAnchor(index);
+            // we care about the situation where the selection changes, and there is no anchor. In this
+            // case, we set a new anchor to be the selected index
+            if (newAnchor == -1) {
+                int addedSize = c.getAddedSize();
+                newAnchor = addedSize > 0 ? c.getAddedSubList().get(addedSize - 1) : newAnchor;
             }
         }
+
+        if (newAnchor > -1) {
+            setAnchor(newAnchor);
+        }
     };
 
     private final ListChangeListener<T> itemsListListener = c -> {
         while (c.next()) {
-            if (c.wasAdded() && c.getFrom() <= getAnchor()) {
-                setAnchor(getAnchor() + c.getAddedSize());
-            } else if (c.wasRemoved() && c.getFrom() <= getAnchor()) {
-                setAnchor(getAnchor() - c.getRemovedSize());
+            if (!hasAnchor()) continue;
+
+            int newAnchor = (hasAnchor() ? getAnchor() : 0);
+
+            if (c.wasAdded() && c.getFrom() <= newAnchor) {
+                newAnchor += c.getAddedSize();
+            } else if (c.wasRemoved() && c.getFrom() <= newAnchor) {
+                newAnchor -= c.getRemovedSize();
             }
+
+            setAnchor(newAnchor < 0 ? 0 : newAnchor);
         }
     };
 
--- a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeViewBehavior.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeViewBehavior.java	Tue Jan 19 11:02:33 2016 +1300
@@ -96,10 +96,13 @@
     private boolean selectionChanging = false;
     
     private final ListChangeListener<Integer> selectedIndicesListener = c -> {
+        int newAnchor = getAnchor();
+
         while (c.next()) {
             if (c.wasReplaced()) {
                 if (TreeCellBehavior.hasDefaultAnchor(getNode())) {
                     TreeCellBehavior.removeAnchor(getNode());
+                    continue;
                 }
             }
 
@@ -110,19 +113,23 @@
             // there are no selected items, so lets clear out the anchor
             if (! selectionChanging) {
                 if (sm.isEmpty()) {
-                    setAnchor(-1);
+                    newAnchor = -1;
                 } else if (hasAnchor() && ! sm.isSelected(getAnchor() + shift)) {
-                    setAnchor(-1);
+                    newAnchor = -1;
                 }
             }
 
-            int addedSize = c.getAddedSize();
-            if (addedSize > 0 && ! hasAnchor()) {
-                List<? extends Integer> addedSubList = c.getAddedSubList();
-                int index = addedSubList.get(addedSize - 1);
-                setAnchor(index);
+            // we care about the situation where the selection changes, and there is no anchor. In this
+            // case, we set a new anchor to be the selected index
+            if (newAnchor == -1) {
+                int addedSize = c.getAddedSize();
+                newAnchor = addedSize > 0 ? c.getAddedSubList().get(addedSize - 1) : newAnchor;
             }
         }
+
+        if (newAnchor > -1) {
+            setAnchor(newAnchor);
+        }
     };
     
     private final ChangeListener<MultipleSelectionModel<TreeItem<T>>> selectionModelListener = 
--- a/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Tue Jan 19 11:02:33 2016 +1300
@@ -30,7 +30,9 @@
 import static javafx.scene.control.SelectionMode.SINGLE;
 
 import java.util.*;
+import java.util.stream.Collectors;
 
+import com.sun.javafx.scene.control.MultipleAdditionAndRemovedChange;
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
 import javafx.collections.ListChangeListener.Change;
@@ -82,8 +84,13 @@
                 // items list, as a index permutation implies the items list is
                 // unchanged, not changed!
                 boolean hasRealChangeOccurred = false;
-                while (c.next() && ! hasRealChangeOccurred) {
-                    hasRealChangeOccurred = c.wasAdded() || c.wasRemoved();
+
+                if (c instanceof MultipleAdditionAndRemovedChange) {
+                    hasRealChangeOccurred = false;
+                } else {
+                    while (!hasRealChangeOccurred && c.next()) {
+                        hasRealChangeOccurred = c.wasAdded() || c.wasRemoved();
+                    }
                 }
 
                 if (hasRealChangeOccurred) {
@@ -91,7 +98,7 @@
                         selectedItemsSeq.callObservers(selectedItemChange);
                     } else {
                         c.reset();
-                        selectedItemsSeq.callObservers(new MappingChange<Integer, T>(c, map, selectedItemsSeq));
+                        selectedItemsSeq.callObservers(new MappingChange<>(c, map, selectedItemsSeq));
                     }
                 }
                 c.reset();
@@ -245,11 +252,17 @@
         Collections.sort(shifts, (s1, s2) -> Integer.compare(s2.getKey(), s1.getKey()));
         final int lowestShiftPosition = shifts.get(shifts.size() - 1).getKey();
 
-        boolean hasPermutated = false;
+        // make a copy of the selectedIndices before so we can compare to it afterwards
+        BitSet selectedIndicesCopy = (BitSet) selectedIndices.clone();
+
         for (Pair<Integer, Integer> shift : shifts) {
-            hasPermutated = doShift(shift, callback, perm) || hasPermutated;
+            doShift(shift, callback, perm);
         }
 
+        // strip out all useless -1 default values from the perm array
+        final int[] prunedPerm = Arrays.stream(perm).filter(value -> value > -1).toArray();
+        final boolean hasSelectionChanged = prunedPerm.length > 0;
+
         // This ensure that the selection remains accurate when a shift occurs.
         final int selectedIndex = getSelectedIndex();
         if (selectedIndex >= lowestShiftPosition && selectedIndex > -1) {
@@ -272,7 +285,7 @@
             // changed to check if hasPermutated, and to call select(..) for RT-40010.
             // This forces the selection event to go through the system and fire
             // the necessary events.
-            if (hasPermutated) {
+            if (hasSelectionChanged) {
                 selectedIndices.set(newSelectionLead, true);
             } else {
                 select(newSelectionLead);
@@ -282,26 +295,32 @@
 //            focus(newSelectionLead);
         }
 
-        if (hasPermutated) {
-            selectedIndicesSeq.callObservers(
-                new NonIterableChange.SimplePermutationChange<>(
-                        0, 
-                        selectedIndicesCardinality, 
-                        perm, 
-                        selectedIndicesSeq));
+        if (hasSelectionChanged) {
+            // work out what indices were removed and added
+            BitSet removed = (BitSet) selectedIndicesCopy.clone();
+            removed.andNot(selectedIndices);
+
+            BitSet added = (BitSet) selectedIndices.clone();
+            added.andNot(selectedIndicesCopy);
+
+            selectedIndicesSeq.reset();
+            selectedIndicesSeq.callObservers(new MultipleAdditionAndRemovedChange<>(
+                    added.stream().boxed().collect(Collectors.toList()),
+                    removed.stream().boxed().collect(Collectors.toList()),
+                    selectedIndicesSeq
+            ));
         }
     }
 
-    private boolean doShift(Pair<Integer, Integer> shiftPair, final Callback<ShiftParams, Void> callback, int[] perm) {
+    private void doShift(Pair<Integer, Integer> shiftPair, final Callback<ShiftParams, Void> callback, int[] perm) {
         final int position = shiftPair.getKey();
         final int shift = shiftPair.getValue();
 
         // with no check here, we get RT-15024
-        if (position < 0) return false;
-        if (shift == 0) return false;
+        if (position < 0) return;
+        if (shift == 0) return;
 
         int idx = (int) Arrays.stream(perm).filter(value -> value > -1).count();
-        boolean hasPermutated = false;
 
         int selectedIndicesSize = selectedIndices.size() - idx;   // number of bits reserved
 
@@ -318,7 +337,6 @@
 
                 if (selected) {
                     perm[idx++] = i + 1;
-                    hasPermutated = true;
                 }
             }
             selectedIndices.clear(position);
@@ -337,12 +355,9 @@
 
                 if (selected) {
                     perm[idx++] = i;
-                    hasPermutated = true;
                 }
             }
         }
-
-        return hasPermutated;
     }
 
     @Override public void clearAndSelect(int row) {
--- a/modules/controls/src/test/java/test/javafx/scene/control/ListViewTest.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/test/java/test/javafx/scene/control/ListViewTest.java	Tue Jan 19 11:02:33 2016 +1300
@@ -1316,7 +1316,7 @@
         assertEquals("a", fm.getFocusedItem());
         assertEquals(0, fm.getFocusedIndex());
 
-        int anchor = ListCellBehavior.getAnchor(stringListView, null);
+        int anchor = ListCellBehavior.getAnchor(stringListView, -1);
         assertTrue(ListCellBehavior.hasNonDefaultAnchor(stringListView));
         assertEquals(0, anchor);
 
@@ -1336,7 +1336,7 @@
 
         // The second bug was that the anchor was not being pushed down as well
         // (when it should).
-        anchor = ListCellBehavior.getAnchor(stringListView, null);
+        anchor = ListCellBehavior.getAnchor(stringListView, -1);
         assertTrue(ListCellBehavior.hasNonDefaultAnchor(stringListView));
         assertEquals(1, anchor);
     }
--- a/modules/controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java	Tue Jan 19 11:02:33 2016 +1300
@@ -3612,9 +3612,9 @@
         // pushed down, but this should not result in an item permutation event,
         // as it remains unchanged
         two.setExpanded(true);
-        assertEquals(0, rt_37395_index_removeCount);
-        assertEquals(1, rt_37395_index_addCount);
-        assertEquals(1, rt_37395_index_permutationCount);
+        assertEquals(1, rt_37395_index_removeCount);
+        assertEquals(2, rt_37395_index_addCount);
+        assertEquals(0, rt_37395_index_permutationCount);
         assertEquals(0, rt_37395_item_removeCount);
         assertEquals(1, rt_37395_item_addCount);
         assertEquals(0, rt_37395_item_permutationCount);
@@ -3623,9 +3623,9 @@
         // Same argument as in step two above: no addition or removal, just a
         // permutation on the index
         two.setExpanded(false);
-        assertEquals(0, rt_37395_index_removeCount);
-        assertEquals(1, rt_37395_index_addCount);
-        assertEquals(2, rt_37395_index_permutationCount);
+        assertEquals(2, rt_37395_index_removeCount);
+        assertEquals(3, rt_37395_index_addCount);
+        assertEquals(0, rt_37395_index_permutationCount);
         assertEquals(0, rt_37395_item_removeCount);
         assertEquals(1, rt_37395_item_addCount);
         assertEquals(0, rt_37395_item_permutationCount);
--- a/modules/controls/src/test/java/test/javafx/scene/control/TreeViewTest.java	Mon Jan 18 23:06:45 2016 +0300
+++ b/modules/controls/src/test/java/test/javafx/scene/control/TreeViewTest.java	Tue Jan 19 11:02:33 2016 +1300
@@ -1933,9 +1933,9 @@
         // pushed down, but this should not result in an item permutation event,
         // as it remains unchanged
         two.setExpanded(true);
-        assertEquals(0, rt_37395_index_removeCount);
-        assertEquals(1, rt_37395_index_addCount);
-        assertEquals(1, rt_37395_index_permutationCount);
+        assertEquals(1, rt_37395_index_removeCount);
+        assertEquals(2, rt_37395_index_addCount);
+        assertEquals(0, rt_37395_index_permutationCount);
         assertEquals(0, rt_37395_item_removeCount);
         assertEquals(1, rt_37395_item_addCount);
         assertEquals(0, rt_37395_item_permutationCount);
@@ -1944,9 +1944,9 @@
         // Same argument as in step two above: no addition or removal, just a
         // permutation on the index
         two.setExpanded(false);
-        assertEquals(0, rt_37395_index_removeCount);
-        assertEquals(1, rt_37395_index_addCount);
-        assertEquals(2, rt_37395_index_permutationCount);
+        assertEquals(2, rt_37395_index_removeCount);
+        assertEquals(3, rt_37395_index_addCount);
+        assertEquals(0, rt_37395_index_permutationCount);
         assertEquals(0, rt_37395_item_removeCount);
         assertEquals(1, rt_37395_item_addCount);
         assertEquals(0, rt_37395_item_permutationCount);