RFR: 8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode [v6]

[Jeanette Winzenburg]

On Mon, 22 Aug 2022 21:08:39 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> The issue is caused by TreeTableRow incorrectly selected when cell selection mode is enabled.
>> 
>> Changes:
>> - modified TreeTableRow.updateSelection()
>
> Andy Goryachev has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8292353: typo
>  - 8292353: typo

Fix looks good (left a minor inline comment which you might follow or not :)

The tests, though, ... need some love:

As a general rule, it's preferred to assert a single state per test method instead of changing the state repeatedly inside the same method. We can see that something is wrong when having code comments like: 

    line 240:         assertFalse(row.isSelected()); // JDK-8292353 failure

    line 249:         assertFalse(row.isSelected()); // JDK-8292353 failure

running the test before the fix and the second failure is never reached. We _want_ to see the failures, all of them :)

Write a test as near to the cause as possible: here the cause is an incorrect selection state of the TreeTableRow under certain conditions. That's unrelated to any user interaction, so a setup faking such an interaction is a detour which might (or not) bring in their own trouble (aside: I do see the css parser warnings when running TreeAndTableViewTest inside Eclipse - that is [JDK-8291853](https://bugs.openjdk.org/browse/JDK-8291853) which we don't yet understand, except that something fishy might be going on ;) Best to use the most simple setup to test that exact state, here that would be an isolated Tree/TableRow thats configured with a Tree/TableView at some index and setup the view's selection state as needed, for examples see TreeTableRowTest or something like the following

    @Test
    public void testRowSelectionStateOnCellSelectionEnabledMultiple() {
        // configure view
        addColumns();
        tree.getSelectionModel().setCellSelectionEnabled(true);
        tree.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
        // configure tableRow        
        int selected = 1;
        cell.updateTreeTableView(tree);
        cell.updateIndex(selected);

        // select all columns if possible
        tree.getSelectionModel().select(selected, null);
        assertEquals("sanity: all columns selected", tree.getColumns().size(),
                tree.getSelectionModel().getSelectedCells().size());
        assertFalse("row must not be selected in cell selection mode", cell.isSelected());
    }

and one for the second failure before the fix plus (for coverage) the other combinations of cell selection enablement/selection mode :) These should reside in TreeTableRowTest, IMO, to be "near" the other row state tests.

For coverage, we should also have the corresponding tests for TableRow (also isolated, not wrapped into faked user interaction). They should reside in TableRowTest. Which we don't have yet, time to add it :)

My suggestions: 

- convert the monolithic test methods into separate tests, each without changing the state you want to test
- configure and test the selection state of an isolated row (without faking user interaction, similar to the example above)
- for TreeTableRow, add the tests to TreeTableRowTest
- for TableRow, add TableRowTest and add analogue methods as those you added for TreeTableRow
- remove TreeAndTableViewTest

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java line 450:

> 448:         boolean isSelected = !sm.isCellSelectionEnabled() && sm.isSelected(index);
> 449:         if (isSelected() != isSelected) {
> 450:             updateSelected(isSelected);

looks good now - we might consider to copy the code comment from TableRow updateSelection for emphasis of the implementation intention. But then, it might be considered clutter, with tests in place. Leaving it to you to decide :)

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

Changes requested by fastegal (Reviewer).

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