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

Reply via email to