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");
Fix and tests look rather complete, NPEs are fixed
There is still one unguarded access to the selectionModel in
ListViewSkin.queryAccessibleAttributes: don't know if that's ever reached
without actually having a selectionModel?
As we have similar NPEs in all our behaviors (of virtualized controls), we
might decided here which behavior we want to support and then apply that
decision everywhere. In particular what should happen if we have a null
selection and a not-null focusModel with
- activate/edit: that happens on the focusedIndex/Cell with selection only an
after-thought
- focus navigation (often ctrl-somekey): is inconsistently either supported
(ctrl-end) or not (ctrl-pageDown)
Personally, I think that we should support both - we have a separate focusModel
so should use it independently of the selectionModel where possible. A good
starter for this issue might be to implement the activate such that it doesn't
back out on null selection (just leave out the select statement). The
consistent support of navigation might be done in a follow-up issue.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewMouseInputTest.java
line 397:
> 395:
> 396: loader.dispose();
> 397: }
the stageloader is not disposed if the test fails - so a safer route is to keep
it in a field which is then disposed in @ after (which we decided to do years
ago, but tend to forget occasionally :), see f.i. ListViewKeyInputTest
BTW: good to explicitely create the stageloader and __not__ rely on the one
implicitly created by the flowUtils!
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2196:
> 2194:
> 2195: StageLoader stageLoader = new StageLoader(listView);
> 2196:
just to not forget: stageloader again
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2208:
> 2206:
> 2207: StageLoader stageLoader = new StageLoader(listView);
> 2208:
just to not forget: stageloader again
-------------
Changes requested by fastegal (Reviewer).
PR: https://git.openjdk.org/jfx/pull/711
