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