On Thu, 28 Jul 2022 15:44:28 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - 8235491: tree table view >> - Merge remote-tracking branch 'origin/master' into 8235491.isselected >> - 8235491: review comments >> - 8235491: whitespace >> - 8235491: javadoc >> - 8235491: 2022 >> - 8235491: Tree/TableView: implementation of isSelected(int) violates >> contract > > modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java > line 2829: > >> 2827: return selectedCellsMap.isSelected(index, -1); >> 2828: } >> 2829: } > > wondering why you distinguish between not/ cellSelection? This method is all > about row selection (that is the dimension available in base/multiple > selection) - shouldn't it behave the same way, independent of whether the > second - cell - dimension is turned on? If so, we could simply delegate to > super - or not override it at all. Good question! The implementation is lifted from its isSelected(int,Tree/TableColumn) sibling with the logic altered for the case of enabled cell selection. Let me check if the logic can be delegated to superclass. > modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java > line 178: > >> 176: assertEquals(1, model.getSelectedCells().size()); >> 177: } >> 178: > > do we have tests isSelected(index) for all combinations of > single/multiple/cellSelection? If not, now might be a good time to add them :) a *very* good point! should definitely cover all the scenarios. ------------- PR: https://git.openjdk.org/jfx/pull/839
