changeset 9764:db27fc0c6b8b jdk-9+118

8088896: TableView: selectedIndices fires incorrect change on items modification
author jgiles
date Mon, 09 May 2016 13:34:34 +1200
parents 8994ba3ef12b
children ed1c8d95ce79 5b64910ad0b7
files modules/controls/src/main/java/javafx/scene/control/TableView.java modules/controls/src/main/java/javafx/scene/control/TreeTableView.java modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
diffstat 3 files changed, 108 insertions(+), 46 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/control/TableView.java	Mon May 09 11:33:03 2016 +1200
+++ b/modules/controls/src/main/java/javafx/scene/control/TableView.java	Mon May 09 13:34:34 2016 +1200
@@ -35,7 +35,6 @@
 import java.util.List;
 import java.util.Set;
 import java.util.WeakHashMap;
-import java.util.stream.Collectors;
 
 import com.sun.javafx.scene.control.Logging;
 import com.sun.javafx.scene.control.Properties;
@@ -980,6 +979,10 @@
 
             if (oldValue != null) {
                 oldValue.cellSelectionEnabledProperty().removeListener(weakCellSelectionModelInvalidationListener);
+
+                if (oldValue instanceof TableViewArrayListSelectionModel) {
+                    ((TableViewArrayListSelectionModel)oldValue).dispose();
+                }
             }
 
             oldValue = get();
@@ -2049,7 +2052,7 @@
             super(tableView);
             this.tableView = tableView;
 
-            this.tableView.itemsProperty().addListener(new InvalidationListener() {
+            this.itemsPropertyListener = new InvalidationListener() {
                 private WeakReference<ObservableList<S>> weakItemsRef = new WeakReference<>(tableView.getItems());
 
                 @Override public void invalidated(Observable observable) {
@@ -2059,7 +2062,8 @@
 
                     ((SelectedItemsReadOnlyObservableList)getSelectedItems()).setItemsList(tableView.getItems());
                 }
-            });
+            };
+            this.tableView.itemsProperty().addListener(itemsPropertyListener);
 
             selectedCellsMap = new SelectedCellsMap<TablePosition<S,?>>(this::fireCustomSelectedCellsListChangeEvent) {
                 @Override public boolean isCellSelectionEnabled() {
@@ -2108,8 +2112,19 @@
             });
         }
 
+        private void dispose() {
+            this.tableView.itemsProperty().removeListener(itemsPropertyListener);
+
+            ObservableList<S> items = getTableView().getItems();
+            if (items != null) {
+                items.removeListener(weakItemsContentListener);
+            }
+        }
+
         private final TableView<S> tableView;
 
+        final InvalidationListener itemsPropertyListener;
+
         final ListChangeListener<S> itemsContentListener = c -> {
             updateItemCount();
 
@@ -2283,52 +2298,50 @@
                 }
             }
 
-            if (shift != 0 && startRow >= 0) {
-                List<TablePosition<S,?>> newIndices = new ArrayList<>(selectedCellsMap.size());
-
-                for (int i = 0; i < selectedCellsMap.size(); i++) {
-                    final TablePosition<S,?> old = selectedCellsMap.get(i);
-                    final int oldRow = old.getRow();
-                    final int newRow = Math.max(0, oldRow < startRow ? oldRow : oldRow + shift);
-
-                    if (oldRow < startRow) {
-                        continue;
-                    }
-
-                    // Special case for RT-28637 (See unit test in TableViewTest).
-                    // Essentially the selectedItem was correct, but selectedItems
-                    // was empty.
-                    if (oldRow == 0 && shift == -1) {
-                        newIndices.add(new TablePosition<>(getTableView(), 0, old.getTableColumn()));
-                        continue;
-                    }
-
-                    newIndices.add(new TablePosition<>(getTableView(), newRow, old.getTableColumn()));
+            TablePosition<S,?> anchor = TableCellBehavior.getAnchor(tableView, null);
+            if (shift != 0 && startRow >= 0 && anchor != null && (c.wasRemoved() || c.wasAdded())) {
+                if (isSelected(anchor.getRow(), anchor.getTableColumn())) {
+                    TablePosition<S,?> newAnchor = new TablePosition<>(tableView, anchor.getRow() + shift, anchor.getTableColumn());
+                    TableCellBehavior.setAnchor(tableView, newAnchor, false);
                 }
-
-                final int newIndicesSize = newIndices.size();
-
-                if ((c.wasRemoved() || c.wasAdded()) && newIndicesSize > 0) {
-                    TablePosition<S,?> anchor = TableCellBehavior.getAnchor(tableView, null);
-                    if (anchor != null) {
-                        boolean isAnchorSelected = isSelected(anchor.getRow(), anchor.getTableColumn());
-                        if (isAnchorSelected) {
-                            TablePosition<S,?> newAnchor = new TablePosition<>(tableView, anchor.getRow() + shift, anchor.getTableColumn());
-                            TableCellBehavior.setAnchor(tableView, newAnchor, false);
+            }
+
+            shiftSelection(startRow, shift, new Callback<ShiftParams, Void>() {
+                @Override public Void call(ShiftParams param) {
+
+                    // we make the shifts atomic, as otherwise listeners to
+                    // the items / indices lists get a lot of intermediate
+                    // noise. They eventually get the summary event fired
+                    // from within shiftSelection, so this is ok.
+                    startAtomic();
+
+                    final int clearIndex = param.getClearIndex();
+                    final int setIndex = param.getSetIndex();
+                    TablePosition<S,?> oldTP = null;
+                    if (clearIndex > -1) {
+                        for (int i = 0; i < selectedCellsMap.size(); i++) {
+                            TablePosition<S,?> tp = selectedCellsMap.get(i);
+                            if (tp.getRow() == clearIndex) {
+                                oldTP = tp;
+                                selectedCellsMap.remove(tp);
+                            } else if (tp.getRow() == setIndex && !param.isSelected()) {
+                                selectedCellsMap.remove(tp);
+                            }
                         }
                     }
 
-                    quietClearSelection();
-
-                    // Fix for RT-22079
-                    blockFocusCall = true;
-                    for (int i = 0; i < newIndicesSize; i++) {
-                        TablePosition<S, ?> tp = newIndices.get(i);
-                        select(tp.getRow(), tp.getTableColumn());
+                    if (oldTP != null && param.isSelected()) {
+                        TablePosition<S,?> newTP = new TablePosition<>(
+                                tableView, param.getSetIndex(), oldTP.getTableColumn());
+
+                        selectedCellsMap.add(newTP);
                     }
-                    blockFocusCall = false;
+
+                    stopAtomic();
+
+                    return null;
                 }
-            }
+            });
 
             previousModelSize = getItemCount();
         }
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Mon May 09 11:33:03 2016 +1200
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Mon May 09 13:34:34 2016 +1200
@@ -1035,6 +1035,10 @@
                     // in order to set pseudo-class state
                     if (oldValue != null) {
                         oldValue.cellSelectionEnabledProperty().removeListener(weakCellSelectionModelInvalidationListener);
+
+                        if (oldValue instanceof TreeTableViewArrayListSelectionModel) {
+                            ((TreeTableViewArrayListSelectionModel)oldValue).dispose();
+                        }
                     }
 
                     oldValue = get();
@@ -2316,9 +2320,7 @@
             this.treeTableView = treeTableView;
 
             this.treeTableView.rootProperty().addListener(weakRootPropertyListener);
-            this.treeTableView.showRootProperty().addListener(o -> {
-                shiftSelection(0, treeTableView.isShowRoot() ? 1 : -1, null);
-            });
+            this.treeTableView.showRootProperty().addListener(showRootPropertyListener);
             updateTreeEventListener(null, treeTableView.getRoot());
 
             selectedCellsMap = new SelectedCellsMap<TreeTablePosition<S,?>>(this::fireCustomSelectedCellsListChangeEvent) {
@@ -2348,6 +2350,16 @@
             });
         }
 
+        private void dispose() {
+            this.treeTableView.rootProperty().removeListener(weakRootPropertyListener);
+            this.treeTableView.showRootProperty().removeListener(showRootPropertyListener);
+
+            TreeItem<S> root = this.treeTableView.getRoot();
+            if (root != null) {
+                root.removeEventHandler(TreeItem.<S>expandedItemCountChangeEvent(), weakTreeItemListener);
+            }
+        }
+
         private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
             if (oldRoot != null && weakTreeItemListener != null) {
                 oldRoot.removeEventHandler(TreeItem.<S>expandedItemCountChangeEvent(), weakTreeItemListener);
@@ -2365,6 +2377,10 @@
             updateTreeEventListener(oldValue, newValue);
         };
 
+        private InvalidationListener showRootPropertyListener = o -> {
+            shiftSelection(0, treeTableView.isShowRoot() ? 1 : -1, null);
+        };
+
         private EventHandler<TreeItem.TreeModificationEvent<S>> treeItemListener = new EventHandler<TreeItem.TreeModificationEvent<S>>() {
             @Override public void handle(TreeItem.TreeModificationEvent<S> e) {
 
--- a/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java	Mon May 09 11:33:03 2016 +1200
+++ b/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java	Mon May 09 13:34:34 2016 +1200
@@ -36,6 +36,7 @@
 import static org.junit.Assert.assertTrue;
 
 import java.util.*;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import javafx.collections.FXCollections;
@@ -1272,4 +1273,36 @@
         assertEquals("sanity: state unchanged", selectedIndices, model.getSelectedIndices());
         assertEquals("must not fire if nothing changed", 0, counter.get());
     }
+
+    @Test
+    public void test_jdk_8088896() {
+        model.setSelectionMode(SelectionMode.MULTIPLE);
+
+        model.selectRange(2, 4);
+        assertEquals(2, model.getSelectedIndices().size());
+        assertEquals(2, model.getSelectedItems().size());
+
+        AtomicInteger counter = new AtomicInteger();
+        model.getSelectedIndices().addListener((ListChangeListener)c -> {
+            counter.incrementAndGet();
+            while (c.next()) {
+                if (c.wasAdded()) {
+                    assertTrue(c.getAddedSubList().contains(4));
+                }
+                if (c.wasRemoved()) {
+                    assertTrue(c.getRemoved().contains(2));
+                }
+            }
+        });
+
+        assertEquals(0, counter.get());
+        if (isTree()) {
+            addItem(0, new TreeItem<>("new item"));
+        } else {
+            addItem(0, "new item");
+        }
+        assertEquals(2, model.getSelectedIndices().size());
+        assertEquals(2, model.getSelectedItems().size());
+        assertEquals(1, counter.get());
+    }
 }