changeset 4855:9245ffbe8c4f

RT-32618: change listener on ListView is called one extra time. The same problem was also being exhibited on the other virtualised controls, so this changeset resolves it in all cases, and adds a parameterised test for both single and multiple selection models, to prevent this appearing again.
author jgiles
date Sat, 31 Aug 2013 12:18:39 +1200
parents 91a0d78259d2
children a11427375c9b
files modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java 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/MultipleSelectionModelImplTest.java modules/controls/src/test/java/javafx/scene/control/SelectionModelImplTest.java
diffstat 5 files changed, 79 insertions(+), 11 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Sat Aug 31 08:07:40 2013 +1200
+++ b/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Sat Aug 31 12:18:39 2013 +1200
@@ -313,8 +313,10 @@
         // RT-32411 We used to call quietClearSelection() here, but this
         // resulted in the selectedItems and selectedIndices lists never
         // reporting that they were empty.
-        // quietClearSelection();
+        // makeAtomic toggle added to resolve RT-32618
+        makeAtomic = true;
         clearSelection();
+        makeAtomic = false;
 
         // and select
         select(row);
--- a/modules/controls/src/main/java/javafx/scene/control/TableView.java	Sat Aug 31 08:07:40 2013 +1200
+++ b/modules/controls/src/main/java/javafx/scene/control/TableView.java	Sat Aug 31 12:18:39 2013 +1200
@@ -1874,7 +1874,8 @@
                     // There is a unit test for this, so if a more elegant solution
                     // can be found in the future and this code removed, the unit
                     // test will fail if it isn't fixed elsewhere.
-                    if (selectedItems.isEmpty() && getSelectedItem() != null) {
+                    // makeAtomic toggle added to resolve RT-32618
+                    if (! makeAtomic && selectedItems.isEmpty() && getSelectedItem() != null) {
                         setSelectedItem(null);
                     }
                     
@@ -2158,9 +2159,12 @@
             // RT-32411 We used to call quietClearSelection() here, but this
             // resulted in the selectedItems and selectedIndices lists never
             // reporting that they were empty.
-            // quietClearSelection();
+            // makeAtomic toggle added to resolve RT-32618
+            makeAtomic = true;
             clearSelection();
+            makeAtomic = false;
 
+            // and select
             select(row, column);
         }
 
@@ -2372,8 +2376,11 @@
         }
 
         @Override public void clearSelection() {
-            updateSelectedIndex(-1);
-            focus(-1);
+            if (! makeAtomic) {
+                updateSelectedIndex(-1);
+                focus(-1);
+            }
+
             quietClearSelection();
         }
 
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Sat Aug 31 08:07:40 2013 +1200
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Sat Aug 31 12:18:39 2013 +1200
@@ -2164,7 +2164,8 @@
                     // There is a unit test for this, so if a more elegant solution
                     // can be found in the future and this code removed, the unit
                     // test will fail if it isn't fixed elsewhere.
-                    if (selectedItems.isEmpty() && getSelectedItem() != null) {
+                    // makeAtomic toggle added to resolve RT-32618
+                    if (!makeAtomic && selectedItems.isEmpty() && getSelectedItem() != null) {
                         setSelectedItem(null);
                     }
 
@@ -2396,12 +2397,15 @@
         }
 
         @Override public void clearAndSelect(int row, TableColumnBase<TreeItem<S>,?> column) {
-            // RT-32411 We used to call quietClearSelection() here, but this
+            // RT-32411: We used to call quietClearSelection() here, but this
             // resulted in the selectedItems and selectedIndices lists never
             // reporting that they were empty.
-            // quietClearSelection();
+            // makeAtomic toggle added to resolve RT-32618
+            makeAtomic = true;
             clearSelection();
+            makeAtomic = false;
 
+            // and select
             select(row, column);
         }
 
@@ -2616,8 +2620,11 @@
         }
 
         @Override public void clearSelection() {
-            updateSelectedIndex(-1);
-            focus(-1);
+            if (! makeAtomic) {
+                updateSelectedIndex(-1);
+                focus(-1);
+            }
+
             quietClearSelection();
         }
 
--- a/modules/controls/src/test/java/javafx/scene/control/MultipleSelectionModelImplTest.java	Sat Aug 31 08:07:40 2013 +1200
+++ b/modules/controls/src/test/java/javafx/scene/control/MultipleSelectionModelImplTest.java	Sat Aug 31 12:18:39 2013 +1200
@@ -37,6 +37,8 @@
 import java.util.Iterator;
 import java.util.List;
 
+import javafx.beans.value.ChangeListener;
+import javafx.beans.value.ObservableValue;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
@@ -199,6 +201,9 @@
                 treeTableView.setFocusModel((TreeTableViewFocusModel) focusModel);
                 currentControl = treeTableView;
             }
+
+            // ensure the selection mode is set to multiple
+            model.setSelectionMode(SelectionMode.MULTIPLE);
         } catch (Exception ex) {
             ex.printStackTrace();
         }
@@ -270,12 +275,12 @@
     }
 
     @Test public void testSelectAllWithSingleSelection() {
+        msModel().setSelectionMode(SelectionMode.SINGLE);
         msModel().selectAll();
         ensureInEmptyState();
     }
 
     @Test public void testSelectAllWithMultipleSelection() {
-        msModel().setSelectionMode(SelectionMode.MULTIPLE);
         msModel().selectAll();
 
         assertEquals(19, model.getSelectedIndex());
@@ -896,4 +901,23 @@
         assertEquals(2, rt_32411_add_count);
         assertEquals(1, rt_32411_remove_count);
     }
+
+    private int rt32618_count = 0;
+    @Test public void test_rt32618_multipleSelection() {
+        model.selectedItemProperty().addListener(new ChangeListener<Object>() {
+            @Override public void changed(ObservableValue<? extends Object> ov, Object t, Object t1) {
+                rt32618_count++;
+            }
+        });
+
+        assertEquals(0, rt32618_count);
+
+        model.select(0);
+        assertEquals(1, rt32618_count);
+        assertEquals(ROW_1_VALUE, getValue(model.getSelectedItem()));
+
+        model.clearAndSelect(1);
+        assertEquals(2, rt32618_count);
+        assertEquals(ROW_2_VALUE, getValue(model.getSelectedItem()));
+    }
 }
--- a/modules/controls/src/test/java/javafx/scene/control/SelectionModelImplTest.java	Sat Aug 31 08:07:40 2013 +1200
+++ b/modules/controls/src/test/java/javafx/scene/control/SelectionModelImplTest.java	Sat Aug 31 12:18:39 2013 +1200
@@ -28,6 +28,8 @@
 import java.util.Arrays;
 import java.util.Collection;
 
+import javafx.beans.value.ChangeListener;
+import javafx.beans.value.ObservableValue;
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
 import javafx.scene.control.ChoiceBox.ChoiceBoxSelectionModel;
@@ -49,6 +51,7 @@
 import com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
 
 import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
 
 /**
  * Unit tests for the SelectionModel abstract class used by ListView
@@ -218,6 +221,12 @@
                 focusModel = null;
                 currentControl = comboBox;
             }
+
+            // ensure the selection mode is set to single (if the selection model
+            // is actually a MultipleSelectionModel subclass)
+            if (model instanceof MultipleSelectionModel) {
+                ((MultipleSelectionModel)model).setSelectionMode(SelectionMode.SINGLE);
+            }
         } catch (Exception ex) {
             ex.printStackTrace();
         }
@@ -488,4 +497,23 @@
             assertTrue(cell.isSelected());
         }
     }
+
+    private int rt32618_count = 0;
+    @Test public void test_rt32618_singleSelection() {
+        model.selectedItemProperty().addListener(new ChangeListener<Object>() {
+            @Override public void changed(ObservableValue<? extends Object> ov, Object t, Object t1) {
+                rt32618_count++;
+            }
+        });
+
+        assertEquals(0, rt32618_count);
+
+        model.select(0);
+        assertEquals(1, rt32618_count);
+        assertEquals(ROW_1_VALUE, getValue(model.getSelectedItem()));
+
+        model.clearAndSelect(1);
+        assertEquals(2, rt32618_count);
+        assertEquals(ROW_2_VALUE, getValue(model.getSelectedItem()));
+    }
 }