RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v3]

[Jeanette Winzenburg]

On Thu, 8 Jul 2021 21:08:56 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>>> > the first two are naturally within the original scope, the third is near enough (a property on one of the covered controls) to be included .. the last is arguable, IMO - would tend to not include it here but open a follow-up to then include _all_ sync issues in the skin (probably needs more digging and definitely more testing).
>>> > Thoughts?
>>> 
>>> I reviewed the files and found those two places in files being edited in this PR were missing the newly added null check. It is logical to add null checks in all places in files being edited in this PR.
>> 
>> good point, except that it's not _all_ in the skin *grinning (there are two in listeners to listView selectionModel properties)
>> 
>>> Fixing review comments in the same PR or creating a follow-up issue is up to the author and reviewer's agreement.
>>> Let us not undo the change which is already done in this PR :)
>>> 
>> 
>> I like the _not undoing_ aspect :) - so the question is: should we find  and fix all in combo's skin in this issue or leave this as-is and create a follow-up for the not yet included? 
>> 
>>> If we are sure that the similar fix is needed at other places (apart from the files which are being fixed in this PR), we can do it in a single OR multiple follow-up issue(s).
>> 
>> probably was unclear: didn't mean other controls - suspect there are, which we wait to bubble up sometime :)
>
> Good point. I have no problem fixing more NPEs which are directly related to setting the selectionModel to null. That might be also better then creating a lot of new bugs/follow-ups. Will have a look later on this.

okay, go ahead and fix them too :)

Afaics, there are still two unguarded accessors to combo's selectionModel properties, both in createListView. 

Little anecdote: didn't search but simply stumbled across one of them when playing with your layout related test. Me: can't be that the test fails without fix, flag is set by the skin which had no reason to, blabla. The test: I do .. haha.

And here is why:

     // your test, exposing the issue in layout
     // configure the combo
     comboBox.setValue(..);
     comboBox.setSelectionModel(null);
     comboBox.layout();

     // another test, exposing one of the NPEs in createList
     ComboBox<String> comboBox = new ComboBox<>(items);
     comboBox.setSelectionModel(null);
     installDefaultSkin(comboBox);

the difference is that setup already installs a skin - so at the time of init the selectionModel still is available ;)

The other access is in the listener to listView's selectedIndex .. it might be a bit tricky to expose in a test, I would go for a combo in a stage/loader, access the list and change its selectedIndex (maybe needs the popup open and/or firing a key onto it)

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

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