8235491: Tree/TableView: implementation of isSelected(int) violates c…

…ontract

Backport-of: 7cb8d67

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, 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
@@ -198,9 +198,8 @@ public SelectionModel() { }
     public abstract void clearSelection();
 
     /**
-     * <p>Convenience method to inform if the given index is currently selected
-     * in this SelectionModel. Is functionally equivalent to calling
-     * <code>getSelectedIndices().contains(index)</code>.
+     * This method tests whether the given index is currently selected
+     * in this SelectionModel.
      *
      * @param index The index to check as to whether it is currently selected
      *      or not.

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

@@ -2812,10 +2812,6 @@ private void quietClearSelection() {
             stopAtomic();
         }
 
-        @Override public boolean isSelected(int index) {
-            return isSelected(index, null);
-        }
-
         @Override
         public boolean isSelected(int row, TableColumn<S,?> column) {
             // When in cell selection mode, if the column is null, then we interpret

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2022, 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
@@ -441,7 +441,7 @@ private void updateSelection() {
         if (index == -1 || getTreeTableView() == null) return;
         if (getTreeTableView().getSelectionModel() == null) return;
 
-        boolean isSelected = getTreeTableView().getSelectionModel().isSelected(index);
+        boolean isSelected = getTreeTableView().getSelectionModel().isSelected(index, null);
         if (isSelected() == isSelected) return;
 
         updateSelected(isSelected);

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

@@ -3164,10 +3164,6 @@ private void quietClearSelection() {
             stopAtomic();
         }
 
-        @Override public boolean isSelected(int index) {
-            return isSelected(index, null);
-        }
-
         @Override public boolean isSelected(int row, TableColumnBase<TreeItem<S>,?> column) {
             // When in cell selection mode, if the column is null, then we interpret
             // the users query to be asking if _all_ of the cells in the row are selected,

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2022, 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
@@ -714,7 +714,7 @@ private void test_rt_38306(boolean selectTwoRows) {
             assertFalse(sm.isSelected(row, firstNameCol));
             assertFalse(sm.isSelected(row, lastNameCol));
             assertTrue(sm.isSelected(row, emailCol));
-            assertFalse(sm.isSelected(row));
+            assertTrue(sm.isSelected(row));
 
             // and assert that the visuals are accurate
             // (some TableCells should be selected, but TableRows should not be)
@@ -787,7 +787,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
         if (cellSelection) {
             // Because we are in cell selection mode, this has the effect of
             // selecting just the one cell.
-            assertFalse(sm.isSelected(0));
+            assertTrue(sm.isSelected(0));
             assertTrue(sm.isSelected(0, firstNameCol));
             assertFalse(sm.isSelected(0, lastNameCol));
             assertFalse(sm.isSelected(0, emailCol));
@@ -813,7 +813,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
         if (cellSelection) {
             // Everything should remain the same, except the
             // column of the single selected cell should change to lastNameCol.
-            assertFalse(sm.isSelected(0));
+            assertTrue(sm.isSelected(0));
             assertFalse(sm.isSelected(0, firstNameCol));
             assertTrue(sm.isSelected(0, lastNameCol));
             assertFalse(sm.isSelected(0, emailCol));

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, 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
@@ -40,6 +40,8 @@
 import javafx.scene.control.TableView.TableViewSelectionModel;
 import javafx.scene.control.TableViewShim;
 import javafx.scene.control.TreeViewShim;
+import javafx.scene.control.cell.PropertyValueFactory;
+import test.com.sun.javafx.scene.control.test.Person;
 
 import org.junit.After;
 import org.junit.AfterClass;
@@ -172,7 +174,7 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
         assertFalse(model.isSelected(1, col0));
         assertFalse(model.isSelected(3, col0));
         assertFalse(cells(model), model.isSelected(3, null));
-        assertFalse(model.isSelected(3));
+        assertTrue(model.isSelected(3));
         assertEquals(1, model.getSelectedCells().size());
     }
 
@@ -185,7 +187,7 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
         assertFalse(model.isSelected(1, col0));
         assertFalse(model.isSelected(3, col0));
         assertFalse(cells(model), model.isSelected(3, null));
-        assertFalse(model.isSelected(3));
+        assertTrue(model.isSelected(3));
         assertEquals(1, model.getSelectedCells().size());
     }
 
@@ -522,6 +524,52 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
         assertTrue(model.isSelected(4));
     }
 
+    @Test
+    public void selectIndividualCells() {
+        model.setSelectionMode(SelectionMode.MULTIPLE);
+        model.setCellSelectionEnabled(true);
+        model.clearSelection();
+
+        model.select(0, col0);
+        assertTrue(cells(model), model.isSelected(0));
+        assertFalse(cells(model), model.isSelected(1));
+        assertFalse(cells(model), model.isSelected(2));
+
+        model.select(1, col0);
+        model.select(1, col1);
+        assertTrue(cells(model), model.isSelected(0));
+        assertTrue(cells(model), model.isSelected(1));
+        assertFalse(cells(model), model.isSelected(2));
+
+        model.select(2, col0);
+        model.select(2, col1);
+        model.select(2, col2);
+        assertTrue(cells(model), model.isSelected(0));
+        assertTrue(cells(model), model.isSelected(1));
+        assertTrue(cells(model), model.isSelected(2));
+
+        assertFalse(cells(model), model.isSelected(3));
+
+        assertEquals(6, model.getSelectedCells().size());
+
+        model.clearSelection(0, col0);
+        assertFalse(cells(model), model.isSelected(0));
+
+        model.clearSelection(1, col0);
+        assertTrue(cells(model), model.isSelected(1));
+        model.clearSelection(1, col1);
+        assertFalse(cells(model), model.isSelected(1));
+
+        model.clearSelection(2, col0);
+        assertTrue(cells(model), model.isSelected(2));
+        model.clearSelection(2, col1);
+        assertTrue(cells(model), model.isSelected(2));
+        model.clearSelection(2, col2);
+        assertFalse(cells(model), model.isSelected(2));
+
+        assertEquals(0, model.getSelectedCells().size());
+    }
+
     /***************************************************************************
      *
      * FOCUS TESTS
@@ -852,4 +900,96 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
         model.clearSelection(2);
         model.getSelectedItems().removeListener(listener);
     }
+
+    /**
+     * Analysing failing tests when fixing JDK-8219720.
+     *
+     * Suspect: isSelected(int row) violates contract.
+     *
+     * @see #selectRowWhenInSingleCellSelectionMode()
+     * @see #selectRowWhenInSingleCellSelectionMode2()
+     */
+    @Test
+    public void testSelectRowWhenInSingleCellSelectionModeIsSelected() {
+        model.setSelectionMode(SelectionMode.SINGLE);
+        model.setCellSelectionEnabled(true);
+        model.select(3);
+        // test against contract
+        assertEquals("selected index", 3, model.getSelectedIndex());
+        assertTrue("contained in selected indices", model.getSelectedIndices().contains(3));
+        // test against spec
+        assertEquals("is selected index", model.getSelectedIndices().contains(3), model.isSelected(3));
+    }
+
+    @Test
+    public void testRowSelectionAfterSelectAndHideLastColumnMultipleCellEnabled() {
+        assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.MULTIPLE, true);
+    }
+
+    @Test
+    public void testRowSelectionAfterSelectAndHideLastColumnMultipleNotCellEnabled() {
+        assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.MULTIPLE, false);
+    }
+
+    @Test
+    public void testRowSelectionAfterSelectAndHideLastColumnSingleCellEnabled() {
+        assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.SINGLE, true);
+    }
+
+    @Test
+    public void testRowSelectionAfterSelectAndHideLastColumnSingleNotCellEnabled() {
+        assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.SINGLE, false);
+    }
+
+    public void assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode mode, boolean cellEnabled) {
+        TableView<Person> table = createPersonTableView();
+
+        TableView.TableViewSelectionModel<Person> sm = table.getSelectionModel();
+        sm.setCellSelectionEnabled(cellEnabled);
+        sm.setSelectionMode(mode);
+        int row = 1;
+        int col = table.getColumns().size() - 1;
+        assertRowSelectionAfterSelectAndHideColumn(table, row, col);
+    }
+
+    private void assertRowSelectionAfterSelectAndHideColumn(TableView<Person> table, int row, int col) {
+        TableViewSelectionModel<Person> sm = table.getSelectionModel();
+        TableColumn<Person, ?> column = table.getColumns().get(col);
+
+        sm.select(row, column);
+        assertTrue("sanity: row " + row + "contained in selectedIndices", sm.getSelectedIndices().contains(row));
+        assertTrue("sanity: row must be selected" , sm.isSelected(row));
+        column.setVisible(false);
+        assertTrue("after hiding column: row " + row + "contained in selectedIndices", sm.getSelectedIndices().contains(row));
+        assertTrue("after hiding column: row must be selected" , sm.isSelected(row));
+    }
+
+    /**
+     * Creates and returns a TableView with Persons and columns for all their properties.
+     */
+    private TableView<Person> createPersonTableView() {
+        final ObservableList<Person> data =
+                FXCollections.observableArrayList(
+                        new Person("Jacob", "Smith", "jacob.smith@example.com"),
+                        new Person("Isabella", "Johnson", "isabella.johnson@example.com"),
+                        new Person("Ethan", "Williams", "ethan.williams@example.com"),
+                        new Person("Emma", "Jones", "emma.jones@example.com"),
+                        new Person("Michael", "Brown", "michael.brown@example.com"));
+
+        TableView<Person> table = new TableView<>();
+        table.setItems(data);
+
+        TableColumn<Person, String> firstNameCol = new TableColumn("First Name");
+        firstNameCol.setCellValueFactory(new PropertyValueFactory<Person, String>("firstName"));
+
+        TableColumn<Person, String> lastNameCol = new TableColumn("Last Name");
+        lastNameCol.setCellValueFactory(new PropertyValueFactory<Person, String>("lastName"));
+
+        TableColumn<Person, String> emailCol = new TableColumn("Email");
+        emailCol.setCellValueFactory(new PropertyValueFactory<Person, String>("email"));
+
+        table.getColumns().addAll(firstNameCol, lastNameCol, emailCol);
+
+        return table;
+    }
 }

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewMouseInputTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2022, 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
@@ -889,7 +889,7 @@ private void test_rt_38306(boolean selectTwoRows) {
             assertFalse(sm.isSelected(row, firstNameCol));
             assertFalse(sm.isSelected(row, lastNameCol));
             assertTrue(sm.isSelected(row, emailCol));
-            assertFalse(sm.isSelected(row));
+            assertTrue(sm.isSelected(row));
 
             // and assert that the visuals are accurate
             // (some TableCells should be selected, but TableRows should not be)
@@ -966,7 +966,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
         if (cellSelection) {
             // Because we are in cell selection mode, this has the effect of
             // selecting just the one cell.
-            assertFalse(sm.isSelected(0));
+            assertTrue(sm.isSelected(0));
             assertTrue(sm.isSelected(0, firstNameCol));
             assertFalse(sm.isSelected(0, lastNameCol));
             assertFalse(sm.isSelected(0, emailCol));
@@ -992,7 +992,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
         if (cellSelection) {
             // Everything should remain the same, except the
             // column of the single selected cell should change to lastNameCol.
-            assertFalse(sm.isSelected(0));
+            assertTrue(sm.isSelected(0));
             assertFalse(sm.isSelected(0, firstNameCol));
             assertTrue(sm.isSelected(0, lastNameCol));
             assertFalse(sm.isSelected(0, emailCol));