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

[Kevin Rushforth]

On Tue, 20 Sep 2022 19:06:19 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 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