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

[Jeanette Winzenburg]

On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model is null.
>> 
>> ChoiceBox: 
>> - Null check in **valueProperty()** listener
>> 
>> ComboBox:
>> - Null check in **editableProperty()* listener*
>> - Null check in **valueProperty()** listener
>> - Null check in **ComboBoxListViewSkin#updateValue()**
>> - Null check in **ComboBoxListViewSkin#layoutChildren()**
>> 
>> ~~The tests checks, that no NPE is printed to the console.~
>> Some checks, that the set value is still displayed (either in the ComboBox button cell or the ChoiceBox display label)~~
>
> Marius Hanl has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fixed NPE for setEditable() and layoutChildren()
>  - removed unneeded button cell in test

- fix for value/editable change looks fine, 
- suggest to not include the (yet incomplete) fix for sync of combo/list selection in skin

left some inline comments to the new tests

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 324:

> 322: 
> 323:         comboBox.layout();
> 324: 

KISS again :)

- buttonCell is unrelated
- setting the value is a not immediately obvious trigger of the NPE
- the test relies on the fact that the combo's skin is installed in setup - when installing it later, we'd run into a NPE at a different location in code

but then, this might be something to remember for a future bug/fix, if we agree on not including the list/combo sync into this bug

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 338:

> 336:         assertNull(comboBox.getValue());
> 337:     }
> 338: 

- ideally, tests should not cement implementation details (particularly not if these are potential bugs, like forcing a null value :) In other words: testing the value doesn't belong here (it's null before and null after toggling editable)
- test should cover both directions (from editable -> !editable as well as  from editable -> !editable): it's only accidental that the value is forced in one direction, otherwise we might need to start over if the implementation should change in future
- personally, I prefer one tests to be focused on exactly one change (here: one per direction to not rely on intermediate state, but that's a matter of taste/convention

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

Changes requested by fastegal (Reviewer).

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