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

[Ajit Ghaisas]

On Thu, 3 Nov 2022 15:56:33 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

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

What I mean by code simplification is -

        boolean isAlreadySelected = false;
        MultipleSelectionModel<?> sm = getSelectionModel();
        if (sm != null) {
            isAlreadySelected = sm.isSelected(index);

            if (isAlreadySelected && shortcutDown) {
                sm.clearSelection(index);
                getFocusModel().focus(index);
                isAlreadySelected = false;
            } else {
                sm.clearAndSelect(index);
            }
        }

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

There is a `return null` on line 176.
In that case, we will return from either line 173 or line 176. Hence, there is no fall through.

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

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