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

[Jeanette Winzenburg]

On Wed, 27 Jul 2022 05:47:56 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> thank you for your comments, Ajit!  below are responses, please let me know if you agree or not:
>> 
>> 1. javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:
>> `
>>    /**
>>      * Convenience function which tests whether the given row and column index
>>      * is currently selected in this table instance. If the table control is in its
>>      * 'cell selection' mode (where individual cells can be selected, rather than
>>      * entire rows), and if the column argument is null, this method should return
>>      * true only if all cells in the given row are selected.
>>      * @param row the row
>>      * @param column the column
>>      * @return true if the given row and column index is currently selected in
>>      * this table instance
>>      */
>>     public abstract boolean isSelected(int row, TableColumnBase<T,?> column);
>> `
>> 
>> 2. TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.
>> 3. good point, fixed.
>
>>     1. javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:
> Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change. I see that you have captured this in "Compatibility Impact" section of the CSR.
> 
> 
>>     2. TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.
> I see that there are overridden methods `public boolean isSelected(int index)` and `public boolean isSelected(int row, TableColumnBase<TreeItem<S>,?> column)` in class `TreeTableViewArrayListSelectionModel` present in TreeTableView.java. Hence, I think that the change that you have made in `TableView.java - isSelected(int index)` method should also be made in `TreeTableView.java - isSelected(int index)` as well.
> 
> 
>>     3. good point, fixed.
> Thanks!

@aghaisas 

> Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change.

If any application breaks, it's its own fault: they coded against the implementation of TableXXSelectionModels which was (incorrectly) `isSelected(int) == isSelected(int, null)`.

That might be due to the specification mess, though: 

- the old spec on `SelectionModel.isSelected(int)` was incorrectly arguing with api on MultipleSelectionModel, that is _Is functionally equivalent to calling getSelectedIndices().contains(index)_
- that invariant actually belongs into `MultipleSelectionModel.isSelected(int)` which has no specialized spec
- the method is first implemented in the (unfortunately hidden) `MultipleSelectionModelBase.isSelected(int)` fulfilling the constraint but also without spec.

To clean this up completely, we could also change the of MultipleSelectionModel and move the old `isSelected(int) == getSelectedIndices().contains(int)` into the spec of its isSelected. Probably could be done in a follow-up issue (or added to one of the open doc errors around selection models).

What do you think?

-------------

PR: https://git.openjdk.org/jfx/pull/839