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

[Andy Goryachev]

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!

1. Would you suggest possible clarification please?  The javadoc for SelectionModel.isSelected(int) is changed to say

Convenience method to inform if the given index is currently selected
     * in this SelectionModel.  It will return true when at least one cell
     * is selected in the specified row index.


2. you are right!  fixed.

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

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