On Mon, 13 Apr 2020 10:36:51 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
> Macroscopic issue is that initially, the toggle is not sync'ed to the
> selection state. Root reason is an missing else
> block when updating toggle selection state (see report for details).
> Fixed by introducing the else block and removing all follow-up errors that
> tried to amend the consequences of the
> incorrect selection state
> - removed listener to selected item
> - removed toggle selection update in showing listener
>
> The former also fixed the memory leak when replacing the selectionModel plus
> an unreported NPE when the selectionModel
> is null initially.
> Added tests that failed before the fix and passed after. As there had been no
> tests around toggle state, so added some
> to verify that the change doesn't break. Enhanced shim/skin to allow access
> to popup for testing. Removed the
> informally ignored test part for memory leak.
Code changes and test look OK to me.
I have a minor comment that I have listed separately.
modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java line
185:
> 184:
> sm.selectedItemProperty().addListener(selectedItemListener);
> 185: // unfixed part of JDK-8090015 - why exclude null?
> 186: if (sm.getSelectedItem() != null && !
> valueProperty().isBound()) {
Add a TODO: or FIXME: if you intend to work on it. Also, it will be better to
create a JBS issue.
If not - please remove the comment.
-------------
PR: https://git.openjdk.java.net/jfx/pull/177
