8196065: ListChangeListener getRemoved() returns items that were not …

…removed.

Reviewed-by: arapte, kcr

modules/javafx.base/src/test/java/test/javafx/collections/MockListObserver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2021, 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
@@ -29,6 +29,9 @@
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Objects;
+import java.util.function.BiPredicate;
+
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
 import static org.junit.Assert.* ;
@@ -94,36 +97,44 @@ public void check1AddRemove(ObservableList<E> list,
                        List<E> removed,
                        int from,
                        int to) {
-        assertFalse(tooManyCalls);
-        assertEquals(1, calls.size());
+        checkN(1);
         checkAddRemove(0, list, removed, from, to);
     }
 
     public void checkAddRemove(int idx, ObservableList<E> list,
                        List<E> removed,
                        int from,
                        int to) {
+        checkAddRemove(idx, list, removed, Objects::equals, from, to);
+    }
+
+    public void checkAddRemove(int idx, ObservableList<E> list,
+                               List<E> removed,
+                               BiPredicate<E, E> equalityComparer,
+                               int from,
+                               int to) {
         if (removed == null) {
-            removed = Collections.<E>emptyList();
+            removed = Collections.emptyList();
         }
         assertFalse(tooManyCalls);
         Call<E> call = calls.get(idx);
         assertSame(list, call.list);
-        assertEquals(removed, call.removed);
+        assertEquals(removed.size(), call.removed.size());
+        for (int i = 0; i < removed.size(); ++i) {
+            assertTrue(equalityComparer.test(removed.get(i), call.removed.get(i)));
+        }
         assertEquals(from, call.from);
         assertEquals(to, call.to);
         assertEquals(0, call.permutation.length);
     }
 
     public void check1Permutation(ObservableList<E> list, int[] perm) {
-        assertFalse(tooManyCalls);
-        assertEquals(1, calls.size());
+        checkN(1);
         checkPermutation(0, list, 0, list.size(), perm);
     }
 
     public void check1Permutation(ObservableList<E> list, int from, int to, int[] perm) {
-        assertFalse(tooManyCalls);
-        assertEquals(1, calls.size());
+        checkN(1);
         checkPermutation(0, list, from, to, perm);
     }
 
@@ -138,8 +149,7 @@ public void checkPermutation(int idx, ObservableList<E> list, int from, int to,
     }
 
     public void check1Update(ObservableList<E> list, int from, int to) {
-        assertFalse(tooManyCalls);
-        assertEquals(1, calls.size());
+        checkN(1);
         checkUpdate(0, list, from, to);
     }
 
@@ -155,8 +165,12 @@ public void checkUpdate(int idx, ObservableList<E> list, int from, int to) {
     }
 
     public void check1() {
+        checkN(1);
+    }
+
+    public void checkN(int n) {
         assertFalse(tooManyCalls);
-        assertEquals(1, calls.size());
+        assertEquals(n, calls.size());
     }
 
     public void clear() {

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableList.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -46,11 +46,12 @@ public SelectedItemsReadOnlyObservableList(ObservableList<Integer> selectedIndic
         this.itemsRefList = new ArrayList<>();
 
         selectedIndices.addListener((ListChangeListener<Integer>)c -> {
+            int totalRemovedSize = 0;
             beginChange();
 
             while (c.next()) {
                 if (c.wasReplaced()) {
-                    List<E> removed = getRemovedElements(c);
+                    List<E> removed = getRemovedElements(c, totalRemovedSize);
                     List<E> added = getAddedElements(c);
                     if (!removed.equals(added)) {
                         nextReplace(c.getFrom(), c.getTo(), removed);
@@ -60,10 +61,11 @@ public SelectedItemsReadOnlyObservableList(ObservableList<Integer> selectedIndic
                 } else if (c.wasRemoved()) {
                     int removedSize = c.getRemovedSize();
                     if (removedSize == 1) {
-                        nextRemove(c.getFrom(), getRemovedModelItem(c.getFrom()));
+                        nextRemove(c.getFrom(), getRemovedModelItem(totalRemovedSize + c.getFrom()));
                     } else {
-                        nextRemove(c.getFrom(), getRemovedElements(c));
+                        nextRemove(c.getFrom(), getRemovedElements(c, totalRemovedSize));
                     }
+                    totalRemovedSize += removedSize;
                 } else if (c.wasPermutated()) {
                     int[] permutation = new int[size()];
                     for (int i = 0; i < size(); i++) {
@@ -117,11 +119,11 @@ private E getRemovedModelItem(int index) {
         return index < 0 || index >= itemsRefList.size() ? null : itemsRefList.get(index).get();
     }
 
-    private List<E> getRemovedElements(ListChangeListener.Change<? extends Integer> c) {
+    private List<E> getRemovedElements(ListChangeListener.Change<? extends Integer> c, int totalRemovedSize) {
         List<E> removed = new ArrayList<>(c.getRemovedSize());
         final int startPos = c.getFrom();
         for (int i = startPos, max = startPos + c.getRemovedSize(); i < max; i++) {
-            removed.add(getRemovedModelItem(i));
+            removed.add(getRemovedModelItem(i + totalRemovedSize));
         }
         return removed;
     }

modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java

@@ -32,6 +32,9 @@
 import javafx.scene.Node;
 import javafx.scene.Parent;
 import javafx.scene.Scene;
+
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.stream.Collectors;
 
@@ -68,7 +71,7 @@ static void requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(Control c) {
     }
 
     static <T> ListChangeListener.Change<T> buildClearAndSelectChange(
-            ObservableList<T> list, List<T> removed, int retainedRow) {
+            ObservableList<T> list, List<T> removed, T retainedRow, Comparator<T> rowComparator) {
         return new ListChangeListener.Change<T>(list) {
             private final int[] EMPTY_PERM = new int[0];
 
@@ -83,9 +86,7 @@ static <T> ListChangeListener.Change<T> buildClearAndSelectChange(
             private int from = -1;
 
             {
-                int midIndex = retainedRow >= removedSize ? removedSize :
-                               retainedRow < 0 ? 0 :
-                               retainedRow;
+                int midIndex = -Collections.binarySearch(removed, retainedRow, rowComparator) - 1;
                 firstRemovedRange = removed.subList(0, midIndex);
                 secondRemovedRange = removed.subList(midIndex, removedSize);
             }
@@ -105,6 +106,7 @@ static <T> ListChangeListener.Change<T> buildClearAndSelectChange(
             }
 
             @Override public int getRemovedSize() {
+                checkState();
                 return atFirstRange ? firstRemovedRange.size() : secondRemovedRange.size();
             }
 
@@ -114,12 +116,12 @@ static <T> ListChangeListener.Change<T> buildClearAndSelectChange(
             }
 
             @Override public boolean next() {
-                if (invalid && atFirstRange) {
+                if (invalid) {
                     invalid = false;
 
                     // point 'from' to the first position, relative to
                     // the underlying selectedCells index.
-                    from = 0;
+                    from = atFirstRange ? 0 : 1;
                     return true;
                 }
 
@@ -137,7 +139,7 @@ static <T> ListChangeListener.Change<T> buildClearAndSelectChange(
 
             @Override public void reset() {
                 invalid = true;
-                atFirstRange = true;
+                atFirstRange = !firstRemovedRange.isEmpty();
             }
 
             private void checkState() {

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java

@@ -372,7 +372,8 @@ boolean isAtomic() {
          *   return the same number - the place where the removed elements were positioned in the list.
          */
         if (wasSelected) {
-            change = ControlUtils.buildClearAndSelectChange(selectedIndices, previousSelectedIndices, row);
+            change = ControlUtils.buildClearAndSelectChange(
+                    selectedIndices, previousSelectedIndices, row, Comparator.naturalOrder());
         } else {
             int changeIndex = Math.max(0, selectedIndices.indexOf(row));
             change = new NonIterableChange.GenericAddRemoveChange<>(

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2021, 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
@@ -2465,7 +2465,8 @@ private void updateSelection(ListChangeListener.Change<? extends S> c) {
              *   return the same number - the place where the removed elements were positioned in the list.
              */
             if (wasSelected) {
-                change = ControlUtils.buildClearAndSelectChange(selectedCellsSeq, previousSelection, row);
+                change = ControlUtils.buildClearAndSelectChange(
+                        selectedCellsSeq, previousSelection, newTablePosition, Comparator.comparing(TablePosition::getRow));
             } else {
                 final int changeIndex = isCellSelectionEnabled ? 0 : Math.max(0, selectedCellsSeq.indexOf(newTablePosition));
                 final int changeSize = isCellSelectionEnabled ? getSelectedCells().size() : 1;

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2021, 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
@@ -2814,7 +2814,8 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
              *   return the same number - the place where the removed elements were positioned in the list.
              */
             if (wasSelected) {
-                change = ControlUtils.buildClearAndSelectChange(selectedCellsSeq, previousSelection, row);
+                change = ControlUtils.buildClearAndSelectChange(
+                        selectedCellsSeq, previousSelection, newTablePosition, Comparator.comparing(TreeTablePosition::getRow));
             } else {
                 final int changeIndex = isCellSelectionEnabled ? 0 : Math.max(0, selectedCellsSeq.indexOf(newTablePosition));
                 final int changeSize = isCellSelectionEnabled ? getSelectedCells().size() : 1;