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

[Jeanette Winzenburg]

On Sat, 3 Jul 2021 12:58:16 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> This PR fixes 2 NPEs in Choice-and ComboBox, when the selection model is null.
>> 
>> ChoiceBox: 
>> - Null check in **valueProperty()** listener
>> 
>> ComboBox:
>> - Null check in **valueProperty()** listener
>> - Null check in **ComboBoxListViewSkin#updateValue()**
>> 
>> The tests checks, that no NPE is printed to the console. They also 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:
> 
>  - removed now unused imports
>  - Review adjustments

fix looks good, as already mentioned in a previous comment, I think it's okay to guard against null selection model before accessing its properties everywhere - that's  already done nearly everywhere, except where it was forgotten ;)

Verified the new tests are failing/passing before/after  respectively, and old tests still passing.

Left a couple of comments to the tests

modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java line 81:

> 79:                 Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
> 80:             }
> 81:         });

setting the uncaughtHandler should be the very first thingy to do in before, and removing it should be the very last to do in after (which is missing here completely, must be added)

modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java line 176:

> 174:             fail("ChoiceBox.setValue() should not throw an exception.");
> 175:         }
> 176: 

why the try? let the exception bubble up as-thrown (just call the formerly misbehaving method) - otherwise we are loosing information as to the origin of the error :)

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

> 305:             fail("ComboBox.setValue() should not throw an exception.");
> 306:         }
> 307: 

same as for ChoiceBoxTest

also: there are two locations of the fix (in combo itself and its skin), so there should be 2 separate tests, each concentrating on one - for testing the combo-only, just don't install a skin :)

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

Changes requested by fastegal (Reviewer).

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