RFR: 8235491: Tree/TableView: implementation of isSelected(int) violates contract [v4]

[Andy Goryachev]

On Thu, 28 Jul 2022 15:44:28 GMT, Jeanette Winzenburg <fastegal at 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