On Sat, 8 Jan 2022 00:17:36 GMT, Marius Hanl <mh...@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