Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Tue, 11 Jan 2022 17:21:13 GMT, Michael Strauß wrote: > The question is whether or not `null` should be a valid value for the > `selectionModel` and `focusModel` properties. I think there are good reasons > to think that it shouldn't. The primitive property classes > (`IntegerProperty`, `BooleanProperty`, etc.) have the same issue, and they > deal with this situation by coercing the invalid `null` value to their > respective default value. I think the property classes for primitives are another use case, since those values really can't be null. I get your point but I think a null `selectionModel` or `focusModel` should be allowed. At least an use case would be to have a read-only `ListView` which should not be selectable nor focusable. If you just set a ListView to uneditable (via `setEditable(..)`), you can still select entries. If you disable the `ListView` it is grey styled which is not desired in this use case. Of course I can change the style, but a null selection and-focusModel may make sense as well. But I agree to a certain point, maybe it make's sense to fail for some properties when null is set/or use default values (but without changing the property?). Right now some properties in e.g. `Node` can be set to null and it will throw a NPE somewhere (e.g. `setCacheHint(null)`, `setRotationAxis(null)`, `Labeled.setFont(null)` ...). But on the other hand a lot of code handle properties with null with some kind of default behaviour (the property itself is never changed). Examples for this: - `Labeled.getTextOverrun()` -> null will be handled as ELLIPSIS in LabeledSkinBase - `Labeled.getEllipsisString()` -> null will be handled as "" in LabeledSkinBase - ... So there is no consistent behaviour for this, but a lot of code already handles null by behaving in some kind of default way without changing the property directly, and I think it might be the best to adapt to this. - PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Tue, 11 Jan 2022 13:04:52 GMT, Marius Hanl wrote: > When a null value is possible, guards against are needed. Unfortunately there > is no built-in way to forbid null in Java as whole. I think null checks should only be done to verify the preconditions of an operation, and not to guard against bugs. This means that, when `null` is not a _valid_ value for a property (regardless of whether it is a _possible_ value), no attempt should be made to guard against this in downstream code. In fact, I would argue that doing so is a strong code smell. The question is whether or not `null` should be a valid value for the `selectionModel` and `focusModel` properties. I think there are good reasons to think that it shouldn't. The primitive property classes (`IntegerProperty`, `BooleanProperty`, etc.) have the same issue, and they deal with this situation by coercing the invalid `null` value to their respective default value. It's not clear to me whether the `ListCellBehavior.anchor` attached property setter would need any special-casing. When `null` (or `-1` in case of `ListViewBehavior.setAnchor`) is passed in, it seems to remove the anchor. Isn't that what we'd expect? - PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Tue, 11 Jan 2022 09:01:51 GMT, John Hendrikx wrote:
> ```
> public final MultipleSelectionModel getSelectionModel() {
> return selectionModel == null ? null :
> selectionModel.get() == NONE_SELECTION_MODEL ? null :
> selectionModel.get();
> }
> ```
That would work altough I think it's a bit hacky as when I listen on the
`selectionModelProperty` I still would get the noop selection model.
I'm in general not a fan of 'self-changing' properties. When a null value is
possible, guards against are needed.
Unfortunately there is no built-in way to forbid null in Java as whole.
> 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.
But then I don't see any advantage as it makes no difference if we check for
NONE model or null. Checks are needed in any case.
-
PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Sat, 8 Jan 2022 00:17:36 GMT, Marius Hanl 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 getSelectionModel() {
return selectionModel == null ? null : selectionModel.get();
}
Just write that as:
public final MultipleSelectionModel 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
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Sat, 8 Jan 2022 14:32:30 GMT, John Hendrikx wrote: > Same goes for the selection model; if set to `null` it should not allow any > kind of selection, which again seems best achieved by installing a model that > ignores all such actions and always returns empty values. We had a similar discussion on https://github.com/openjdk/jfx/pull/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. 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. 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. - PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Sat, 8 Jan 2022 00:17:36 GMT, Marius Hanl 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");
Was it considered to not allow these models to be set to `null` instead? Or to
use a Null Object? I'd much prefer this instead of polluting the code with
`null` checks on every access of these models.
Furthermore, the API makes no mention what is supposed to happen when these
models are set to `null`, so this seems undefined at this time. If `null` is
supposed to be valid then I think the documentation should clearly state what
this means. Having no focus model for example should probably mean that
nothing can ever get focus and that the focused item is always null and index
is always -1. Instead of adding null checks to achieve this however I'd use a
`NullFocusModel` instance as it is easy to forget a `null` check (also when
making future changes).
Same goes for the selection model; if set to `null` it should not allow any
kind of selection, which again seems best achieved by installing a model that
ignores all such actions and always returns empty values.
-
PR: https://git.openjdk.java.net/jfx/pull/711
RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
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");
-
Commit messages:
- 8279640: ListView with null SelectionModel/FocusModel throws NPE
Changes: https://git.openjdk.java.net/jfx/pull/711/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=711=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8279640
Stats: 145 lines in 4 files changed: 130 ins; 2 del; 13 mod
Patch: https://git.openjdk.java.net/jfx/pull/711.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/711/head:pull/711
PR: https://git.openjdk.java.net/jfx/pull/711
