RFR: 8187145: (Tree)TableView with null selectionModel: throws NPE on sorting [v5]

[Andy Goryachev]

On Thu, 3 Nov 2022 08:05:24 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model
>>  - 8187145: added tests
>>  - 8187145: clear selected tree table row when setting null selection model
>>  - 8187145: clear selection when setting selection model
>>  - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model
>>  - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model
>>  - 8187145: whitespace
>>  - 8187145: clear focus
>>  - 8187145: tree table view
>>  - 8187145: fall through
>>  - ... and 1 more: https://git.openjdk.org/jfx/compare/7cb8d679...fed5dfdb
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/CellBehaviorBase.java line 274:
> 
>> 272:     protected void simpleSelect(MouseButton button, int clickCount, boolean shortcutDown) {
>> 273:         final int index = getIndex();
>> 274:         boolean isAlreadySelected;
> 
> Code simplification: if `isAlreadySelected` is initialized to `false` on this line, then we can have only `if(sm != null)` condition below instead of if-else.

it's not: `isAlreadySelected` is set on line 278 or 280.  Line 274 is just a declaration.

> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java line 203:
> 
>> 201:     protected void simpleSelect(MouseButton button, int clickCount, boolean shortcutDown) {
>> 202:         final TableSelectionModel<S> sm = getSelectionModel();
>> 203:         boolean isAlreadySelected;
> 
> Code simplification: if `isAlreadySelected` is initialized to `false` on this line, then we can have only `if(sm != null)` condition below instead of if-else.

same thing: `isAlreadySelected` is set on line 206 or 210.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java line 178:
> 
>> 176:                 return null;
>> 177:             }
>> 178:             // fall through
> 
> There is no "fall through" here as we return from all conditions in previous case.

there is a fall through (return is inside an `if` on line 173). 
The original code is a bit confusing, it took me a while to see the fall through cases, that's why I added explicit comments.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java line 192:
> 
>> 190:                 return null;
>> 191:             }
>> 192:             // fall through
> 
> There is no "fall through" here as we return from all conditions in previous case.

same comment, there is a fall through.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 486:
> 
>> 484:                 return null;
>> 485:             }
>> 486:             // fall through
> 
> There is no "fall through" here as we return from all conditions in previous case.

same comment, there is a fall through.

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

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