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

[Jeanette Winzenburg]

On Wed, 27 Jul 2022 21:36:10 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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 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

Basically good fix, left a couple of change suggestions/questions inline :)

forgot: will not be near my IDE until next Monday - then I'll run the tests and see for myself :)

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.

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 3181:

> 3179:                 return selectedCellsMap.isSelected(index, -1);
> 3180:             }
> 3181:         }

same comment as tableView - just to not forget :)

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 :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewSelectionModelImplTest.java line 139:

> 137:         assertTrue(model.isSelected(3));
> 138:         assertEquals(1, model.getSelectedCells().size());
> 139:     }

same comment as TableView :)

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

Changes requested by fastegal (Reviewer).

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