changeset 7264:9f5303fb78fb

RT-37538: TableView SelectionModel selected items listener can cause an infinite loop.
author jgiles
date Fri, 13 Jun 2014 12:36:57 +1200
parents e6fc9e16cece
children 28bccddb2475
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/javafx/scene/control/ListViewTest.java modules/controls/src/test/java/javafx/scene/control/TableViewTest.java modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java modules/controls/src/test/java/javafx/scene/control/TreeViewTest.java
diffstat 6 files changed, 257 insertions(+), 69 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/control/TableView.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/main/java/javafx/scene/control/TableView.java	Fri Jun 13 12:36:57 2014 +1200
@@ -2808,45 +2808,44 @@
             // here we are considering whether to notify the observers of the
             // selectedItems list. However, we can't just blindly do that, as
             // noted below. This is a part of the fix for RT-37429.
-            while (c.next()) {
-                boolean fireChangeEvent;
-                outer: if (c.wasReplaced()) {
-                    // if a replace happened, we need to check to see if the
-                    // change actually impacts on the selected items - it may
-                    // be that the index changed to the new location of the same
-                    // item (i.e. if a sort occurred). Only if the item has changed
-                    // should we fire an event to the observers of the selectedItems
-                    // list
-                    for (int i = 0; i < c.getRemovedSize(); i++) {
-                        TablePosition<S,?> removed = c.getRemoved().get(i);
-                        S removedItem = removed.getItem();
+            c.next();
+            boolean fireChangeEvent;
+            outer: if (c.wasReplaced()) {
+                // if a replace happened, we need to check to see if the
+                // change actually impacts on the selected items - it may
+                // be that the index changed to the new location of the same
+                // item (i.e. if a sort occurred). Only if the item has changed
+                // should we fire an event to the observers of the selectedItems
+                // list
+                for (int i = 0; i < c.getRemovedSize(); i++) {
+                    TablePosition<S,?> removed = c.getRemoved().get(i);
+                    S removedItem = removed.getItem();
 
-                        boolean matchFound = false;
-                        for (int j = 0; j < c.getAddedSize(); j++) {
-                            TablePosition<S,?> added = c.getAddedSubList().get(j);
-                            S addedItem = added.getItem();
+                    boolean matchFound = false;
+                    for (int j = 0; j < c.getAddedSize(); j++) {
+                        TablePosition<S,?> added = c.getAddedSubList().get(j);
+                        S addedItem = added.getItem();
 
-                            if (removedItem.equals(addedItem)) {
-                                matchFound = true;
-                                break;
-                            }
-                        }
-
-                        if (! matchFound) {
-                            fireChangeEvent = true;
-                            break outer;
+                        if (removedItem.equals(addedItem)) {
+                            matchFound = true;
+                            break;
                         }
                     }
-                    fireChangeEvent = false;
-                } else {
-                    fireChangeEvent = true;
+
+                    if (! matchFound) {
+                        fireChangeEvent = true;
+                        break outer;
+                    }
                 }
+                fireChangeEvent = false;
+            } else {
+                fireChangeEvent = true;
+            }
 
-                if (fireChangeEvent) {
-                    // create an on-demand list of the removed objects contained in the
-                    // given rows.
-                    selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
-                }
+            if (fireChangeEvent) {
+                // create an on-demand list of the removed objects contained in the
+                // given rows.
+                selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
             }
             c.reset();
 
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Fri Jun 13 12:36:57 2014 +1200
@@ -3035,45 +3035,44 @@
             // here we are considering whether to notify the observers of the
             // selectedItems list. However, we can't just blindly do that, as
             // noted below. This is a part of the fix for RT-37429.
-            while (c.next()) {
-                boolean fireChangeEvent;
-                outer: if (c.wasReplaced()) {
-                    // if a replace happened, we need to check to see if the
-                    // change actually impacts on the selected items - it may
-                    // be that the index changed to the new location of the same
-                    // item (i.e. if a sort occurred). Only if the item has changed
-                    // should we fire an event to the observers of the selectedItems
-                    // list
-                    for (int i = 0; i < c.getRemovedSize(); i++) {
-                        TreeTablePosition<S,?> removed = c.getRemoved().get(i);
-                        TreeItem<S> removedTreeItem = removed.getTreeItem();
-
-                        boolean matchFound = false;
-                        for (int j = 0; j < c.getAddedSize(); j++) {
-                            TreeTablePosition<S,?> added = c.getAddedSubList().get(j);
-                            TreeItem<S> addedTreeItem = added.getTreeItem();
-
-                            if (removedTreeItem.equals(addedTreeItem)) {
-                                matchFound = true;
-                                break;
-                            }
-                        }
-
-                        if (! matchFound) {
-                            fireChangeEvent = true;
-                            break outer;
+            c.next();
+            boolean fireChangeEvent;
+            outer: if (c.wasReplaced()) {
+                // if a replace happened, we need to check to see if the
+                // change actually impacts on the selected items - it may
+                // be that the index changed to the new location of the same
+                // item (i.e. if a sort occurred). Only if the item has changed
+                // should we fire an event to the observers of the selectedItems
+                // list
+                for (int i = 0; i < c.getRemovedSize(); i++) {
+                    TreeTablePosition<S,?> removed = c.getRemoved().get(i);
+                    TreeItem<S> removedTreeItem = removed.getTreeItem();
+
+                    boolean matchFound = false;
+                    for (int j = 0; j < c.getAddedSize(); j++) {
+                        TreeTablePosition<S,?> added = c.getAddedSubList().get(j);
+                        TreeItem<S> addedTreeItem = added.getTreeItem();
+
+                        if (removedTreeItem.equals(addedTreeItem)) {
+                            matchFound = true;
+                            break;
                         }
                     }
-                    fireChangeEvent = false;
-                } else {
-                    fireChangeEvent = true;
+
+                    if (! matchFound) {
+                        fireChangeEvent = true;
+                        break outer;
+                    }
                 }
-
-                if (fireChangeEvent) {
-                    // create an on-demand list of the removed objects contained in the
-                    // given rows.
-                    selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
-                }
+                fireChangeEvent = false;
+            } else {
+                fireChangeEvent = true;
+            }
+
+            if (fireChangeEvent) {
+                // create an on-demand list of the removed objects contained in the
+                // given rows.
+                selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
             }
             c.reset();
 
--- a/modules/controls/src/test/java/javafx/scene/control/ListViewTest.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/test/java/javafx/scene/control/ListViewTest.java	Fri Jun 13 12:36:57 2014 +1200
@@ -41,10 +41,12 @@
 import com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.ReadOnlyBooleanWrapper;
+import javafx.beans.property.ReadOnlyObjectWrapper;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.value.ChangeListener;
 import javafx.beans.value.ObservableValue;
 import javafx.collections.FXCollections;
+import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
 import javafx.event.ActionEvent;
 import javafx.event.Event;
@@ -903,4 +905,47 @@
         assertEquals(0, rt_37061_index_counter);
         assertEquals(0, rt_37061_item_counter);
     }
+
+    private int rt_37538_count = 0;
+    @Test public void test_rt_37538_noCNextCall() {
+        test_rt_37538(false, false);
+    }
+
+    @Test public void test_rt_37538_callCNextOnce() {
+        test_rt_37538(true, false);
+    }
+
+    @Test public void test_rt_37538_callCNextInLoop() {
+        test_rt_37538(false, true);
+    }
+
+    private void test_rt_37538(boolean callCNextOnce, boolean callCNextInLoop) {
+        ListView<Integer> list = new ListView<>();
+        for ( int i = 1; i <= 50; i++ ) {
+            list.getItems().add(i);
+        }
+
+        list.getSelectionModel().getSelectedItems().addListener((ListChangeListener.Change<? extends Integer> c) -> {
+            if (callCNextOnce) {
+                c.next();
+            } else if (callCNextInLoop) {
+                while (c.next()) {
+                    // no-op
+                }
+            }
+
+            if (rt_37538_count >= 1) {
+                Thread.dumpStack();
+                fail("This method should only be called once");
+            }
+
+            rt_37538_count++;
+        });
+
+        StageLoader sl = new StageLoader(list);
+        assertEquals(0, rt_37538_count);
+        list.getSelectionModel().select(0);
+        assertEquals(1, rt_37538_count);
+        sl.dispose();
+    }
 }
--- a/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java	Fri Jun 13 12:36:57 2014 +1200
@@ -3671,4 +3671,50 @@
 
         sl.dispose();
     }
+
+    private int rt_37538_count = 0;
+    @Test public void test_rt_37538_noCNextCall() {
+        test_rt_37538(false, false);
+    }
+
+    @Test public void test_rt_37538_callCNextOnce() {
+        test_rt_37538(true, false);
+    }
+
+    @Test public void test_rt_37538_callCNextInLoop() {
+        test_rt_37538(false, true);
+    }
+
+    private void test_rt_37538(boolean callCNextOnce, boolean callCNextInLoop) {
+        TableView<Integer> table = new TableView<>();
+        for ( int i = 1; i <= 50; i++ ) {
+            table.getItems().add(i);
+        }
+        final TableColumn<Integer, Integer> column = new TableColumn<>("Column");
+        table.getColumns().add(column);
+        column.setCellValueFactory( cdf -> new ReadOnlyObjectWrapper<Integer>(cdf.getValue()));
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener.Change<? extends Integer> c) -> {
+            if (callCNextOnce) {
+                c.next();
+            } else if (callCNextInLoop) {
+                while (c.next()) {
+                    // no-op
+                }
+            }
+
+            if (rt_37538_count >= 1) {
+                Thread.dumpStack();
+                fail("This method should only be called once");
+            }
+
+            rt_37538_count++;
+        });
+
+        StageLoader sl = new StageLoader(table);
+        assertEquals(0, rt_37538_count);
+        table.getSelectionModel().select(0);
+        assertEquals(1, rt_37538_count);
+        sl.dispose();
+    }
 }
--- a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java	Fri Jun 13 12:36:57 2014 +1200
@@ -3719,4 +3719,55 @@
 
         sl.dispose();
     }
+
+    private int rt_37538_count = 0;
+    @Test public void test_rt_37538_noCNextCall() {
+        test_rt_37538(false, false);
+    }
+
+    @Test public void test_rt_37538_callCNextOnce() {
+        test_rt_37538(true, false);
+    }
+
+    @Test public void test_rt_37538_callCNextInLoop() {
+        test_rt_37538(false, true);
+    }
+
+    private void test_rt_37538(boolean callCNextOnce, boolean callCNextInLoop) {
+        // create table with a bunch of rows and 1 column...
+        TreeItem<Integer> root = new TreeItem<>(0);
+        root.setExpanded(true);
+        for (int i = 1; i <= 50; i++) {
+            root.getChildren().add(new TreeItem<>(i));
+        }
+
+        final TreeTableColumn<Integer, Integer> column = new TreeTableColumn<>("Column");
+        column.setCellValueFactory( cdf -> new ReadOnlyObjectWrapper<Integer>(cdf.getValue().getValue()));
+
+        final TreeTableView<Integer> table = new TreeTableView<>(root);
+        table.getColumns().add( column );
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener.Change<? extends TreeItem<Integer>> c) -> {
+            if (callCNextOnce) {
+                c.next();
+            } else if (callCNextInLoop) {
+                while (c.next()) {
+                    // no-op
+                }
+            }
+
+            if (rt_37538_count >= 1) {
+                Thread.dumpStack();
+                fail("This method should only be called once");
+            }
+
+            rt_37538_count++;
+        });
+
+        StageLoader sl = new StageLoader(table);
+        assertEquals(0, rt_37538_count);
+        table.getSelectionModel().select(0);
+        assertEquals(1, rt_37538_count);
+        sl.dispose();
+    }
 }
--- a/modules/controls/src/test/java/javafx/scene/control/TreeViewTest.java	Thu Jun 12 17:04:53 2014 -0400
+++ b/modules/controls/src/test/java/javafx/scene/control/TreeViewTest.java	Fri Jun 13 12:36:57 2014 +1200
@@ -46,6 +46,7 @@
 
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.ReadOnlyBooleanWrapper;
+import javafx.beans.property.ReadOnlyObjectWrapper;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
@@ -1988,4 +1989,51 @@
             return super.getChildren();
         }
     }
+
+    private int rt_37538_count = 0;
+    @Test public void test_rt_37538_noCNextCall() {
+        test_rt_37538(false, false);
+    }
+
+    @Test public void test_rt_37538_callCNextOnce() {
+        test_rt_37538(true, false);
+    }
+
+    @Test public void test_rt_37538_callCNextInLoop() {
+        test_rt_37538(false, true);
+    }
+
+    private void test_rt_37538(boolean callCNextOnce, boolean callCNextInLoop) {
+        // create table with a bunch of rows and 1 column...
+        TreeItem<Integer> root = new TreeItem<>(0);
+        root.setExpanded(true);
+        for (int i = 1; i <= 50; i++) {
+            root.getChildren().add(new TreeItem<>(i));
+        }
+
+        final TreeView<Integer> tree = new TreeView<>(root);
+
+        tree.getSelectionModel().getSelectedItems().addListener((ListChangeListener.Change<? extends TreeItem<Integer>> c) -> {
+            if (callCNextOnce) {
+                c.next();
+            } else if (callCNextInLoop) {
+                while (c.next()) {
+                    // no-op
+                }
+            }
+
+            if (rt_37538_count >= 1) {
+                Thread.dumpStack();
+                fail("This method should only be called once");
+            }
+
+            rt_37538_count++;
+        });
+
+        StageLoader sl = new StageLoader(tree);
+        assertEquals(0, rt_37538_count);
+        tree.getSelectionModel().select(0);
+        assertEquals(1, rt_37538_count);
+        sl.dispose();
+    }
 }