On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl <mh...@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
