On Mon, 22 Aug 2022 21:08:39 GMT, Andy Goryachev <ango...@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
