On Thu, 21 Jul 2022 21:10:10 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> …ontract >> >> 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 incrementally with two additional > commits since the last revision: > > - 8235491: whitespace > - 8235491: javadoc 1. I see that you have made below change for cell selection mode - - Before : `isSelected(int)` method used to return `True` only if **all** of the cells in that row were selected. - After : `isSelected(int)` method returns `True` if **any** of the cells in that row are selected. Now, if someone wants to know - while in Cell selection mode, whether all of the cells in a particular row are selected? then they must use another already existing API `isSelected(int row, TableColumn<S,?> column)` with `column` parameter as `null`. This is important and should be captured somewhere as recommendation. 2. Similar change needs to be made for `TreeTableView` as well 3. I have listed a minor comment on coding style modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 2819: > 2817: public boolean isSelected(int index) { > 2818: final boolean isCellSelectionEnabled = > isCellSelectionEnabled(); > 2819: if (isCellSelectionEnabled) { Code Style : Why to create a local variable with the same name as method it gets its value from? In this case changing to `if (isCellSelectionEnabled())` condition check looks simple enough. ------------- Changes requested by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/839