RFR: 8235491: Tree/TableView: implementation of isSelected(int) violates contract [v4]

[Jeanette Winzenburg]

On Thu, 28 Jul 2022 16:37:33 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 2829:
>> 
>>> 2827:                 return selectedCellsMap.isSelected(index, -1);
>>> 2828:             }
>>> 2829:         }
>> 
>> wondering why you distinguish between not/ cellSelection? This method is all about row selection (that is the dimension available in base/multiple selection) - shouldn't it behave the same way, independent of whether the second - cell - dimension is turned on? If so, we could simply delegate to super - or not override it at all.
>
> Good question!
> The implementation is lifted from its isSelected(int,Tree/TableColumn) sibling with the logic altered for the case of enabled cell selection.
> Let me check if the logic can be delegated to superclass.

Think that I found a case where this implementation violates the invariant   `getSelectedIndices().contains(row) == isSelected(row)` -  happens in cell selection mode when we select the last column and then hide that column:

    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));

the last line fails for cell selection enabled with this implementation and passes when delegating to super (or not override `isSelected(int)` at all)

Aside: there is a general issue with cell selection not updated on columns modification (the selection visual is kept on the same column index without changing selectedCells accordingly - technically due to looping across the visibleLeafCells vs. all leaf columns. Might or might not be what ux requires, but the selectedCells must be in sync with the visuals always). Don't remember if we have it covered in JBS?

The complete test set:

    @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);
        TablePosition<Person, ?> pos = new TablePosition<>(table, row, column);

        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 at example.com"),
                        new Person("Isabella", "Johnson", "isabella.johnson at example.com"),
                        new Person("Ethan", "Williams", "ethan.williams at example.com"),
                        new Person("Emma", "Jones", "emma.jones at example.com"),
                        new Person("Michael", "Brown", "michael.brown at 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;
    }


arrgg .. this site is killing me again - no idea why this comment is duplicated .. sry

-------------

PR: https://git.openjdk.org/jfx/pull/839