RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE

[John Hendrikx]

On Sat, 8 Jan 2022 00:17:36 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

> This PR fixes a bunch of NPEs when a null `SelectionModel` or `FocusModel` is set on a `ListView`.
> 
> The following NPEs are fixed (all are also covered by exactly one test case):
> NPEs with null selection model:
> - Mouse click on a `ListCell`
> - SPACE key press
> - KP_UP (arrow up) key press
> - HOME key press
> - END key press
> - BACK_SLASH + CTRL key press
> 
> NPEs with null focus model:
> - SPACE key press
> - Select an items: getSelectionModel().select(1)
> - Clear-Select an item and add one after: listView.getSelectionModel().clearAndSelect(1); listView.getItems().add("3");

> We had a similar discussion on #557. We came to the conclusion that we allow null as a selection or focusmodel and don't set a noop selection model or something of this kind.

Even earlier though in https://github.com/javafxports/openjdk-jfx/issues/569 a `NONE` model was suggested.  And I still think that makes much more sense, because in effect the "behavior" of the `NONE` model is now spread out over dozens of places in the code (as the `null` checks are what is defining the behavior) instead of in a single place.

> The biggest problem with this is the following example: I as a developer set the selection model to null via `setSelectionModel(null)`. Now if the code silently set the selection model to an empty noop selection model, we won't get null when calling `getSelectionModel()`, which a developer would expect.

This is an example from `ListView` which already does something custom because the property is lazily instantiated:

    public final MultipleSelectionModel<T> getSelectionModel() {
        return selectionModel == null ? null : selectionModel.get();
    }

Just write that as:

    public final MultipleSelectionModel<T> getSelectionModel() {
        return selectionModel == null ? null : 
             selectionModel.get() == NONE_SELECTION_MODEL ? null : selectionModel.get();
    }
    
`NONE_SELECTION_MODEL` is stateless so only a single instance is needed.
 
> Also from a look of the code, even if we use a noop focus model, we would still the focus because of the `anchor` stuff, which is set in the `ListViewBehaviour`. If we do a null check and fast return like now, they won't be set -> there are not visible. So it might not even fix our problem but makes even more.

A check can also be done to see if something matches the `NONE` model, just like you can check for `null`, so you can still fast return in special cases.

The chosen approach works as well of course.

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

PR: https://git.openjdk.java.net/jfx/pull/711