On Wed, 27 Jul 2022 21:36:10 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 1. reword SelectionModel.isSelected(int) javadoc, removing incorrect >> statement "Is functionally equivalent to calling >> <code>getSelectedIndices().contains(index)</code>." >> 2. reimplement TableView.TableViewSelectionModel.isSelected(int) method to >> return true when at least one cell in *any* column is selected on the given >> row (was: *all* columns) >> 3. change selectRowWhenInSingleCellSelectionMode() and >> selectRowWhenInSingleCellSelectionMode2() in TableViewSelectionModelImplTest >> to reflect new reality. >> >> NOTE: proposed change alters semantics of isSelected(int) method (in the >> right direction, in my opinion). > > 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 Basically good fix, left a couple of change suggestions/questions inline :) forgot: will not be near my IDE until next Monday - then I'll run the tests and see for myself :) 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. modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 3181: > 3179: return selectedCellsMap.isSelected(index, -1); > 3180: } > 3181: } same comment as tableView - just to not forget :) 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 :) modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewSelectionModelImplTest.java line 139: > 137: assertTrue(model.isSelected(3)); > 138: assertEquals(1, model.getSelectedCells().size()); > 139: } same comment as TableView :) ------------- Changes requested by fastegal (Reviewer). PR: https://git.openjdk.org/jfx/pull/839
