On Sat, 3 Jul 2021 12:58:16 GMT, Marius Hanl <[email protected]> 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
