On Thu, 8 Jul 2021 21:08:56 GMT, Marius Hanl <[email protected]> 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
