On Tue, 20 Sep 2022 19:06:19 GMT, Andy Goryachev <[email protected]> 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 one additional
> commit since the last revision:
>
> 8292353: review comments
The fix looks good.
As for the tests, I somewhat agree with the comments left by @kleopatra,
especially the comment about the suitability of the tests in
`TreeAndTableViewTest`. Having said that, they aren't harmful, so I will leave
it up to you as to whether to remove that test class or leave it in.
I left a few specific comments on the tests inline. You can either update this
PR (probably best) or file a follow-up test bug, which can be done during an
upcoming test sprint.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlUtils.java
line 55:
> 53: * Miscellaneous convenience methods to support javafx.controls tests.
> 54: */
> 55: public class ControlUtils {
Minor: You might consider adding a private no-arg constructor to highlight the
fact that it is a collection of static utility methods.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 28:
> 26:
> 27: import static org.junit.jupiter.api.Assertions.assertFalse;
> 28: import static org.junit.jupiter.api.Assertions.assertTrue;
Best not to mix JUnit4 and JUnit5 methods in the same test class. So I
recommend changing all of the `org.junit.jupiter.api.Assertions` imports their
JUnit4 equivalents in the modified and added tests.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 54:
> 52: }
> 53:
> 54: /** TableView with cell selection enabled should not select
> TreeTableRows */
Typo: should be `TableRows`
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 56:
> 54: /** TableView with cell selection enabled should not select
> TreeTableRows */
> 55: @Test
> 56: public void test_TableView_jdk_8292353_select_all() {
Minor: I recommend removing `jdk_8292353` from the name of the tests, as we no
longer follow that pattern. Instead, you can add the bug ID in the method
comment, if you like.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 81:
> 79: sm.select(0, col0);
> 80: sm.select(0, col1);
> 81: sm.select(0, col2);
In addition to this test method, which verifies that selecting all columns
individually doesn't select the row when in cell selection mode, I recommend
adding another test method that selects all of them as a group using
`sm.select(0, null)`.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 89:
> 87: }
> 88:
> 89: /** TableView with cell selection enabled should not select
> TreeTableRows */
Typo: should be "TableRows"
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
line 120:
> 118: assertFalse(row.isSelected());
> 119: }
> 120: }
Unless you are certain that they are covered elsewhere (not counting anything
in `TreeAndTableViewTest`), having similar test methods to the above with cell
selection disabled would be useful.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeAndTableViewTest.java
line 29:
> 27: import static org.junit.jupiter.api.Assertions.assertEquals;
> 28: import static org.junit.jupiter.api.Assertions.assertFalse;
> 29: import static org.junit.jupiter.api.Assertions.assertTrue;
Replace with JUnit4 equivalents.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableRowTest.java
line 32:
> 30: import static org.junit.Assert.assertSame;
> 31: import static org.junit.jupiter.api.Assertions.assertFalse;
> 32: import static org.junit.jupiter.api.Assertions.assertTrue;
Replace with JUnit4 equivalents.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableRowTest.java
line 840:
> 838: /** TreeTableView with cell selection enabled should not select
> TreeTableRows */
> 839: @Test
> 840: public void test_TreeTableView_jdk_8292353_select_all() {
Ditto the comments I made in `TableViewRowTest` about the test method names and
the need for additional test methods.
-------------
PR: https://git.openjdk.org/jfx/pull/875
