[Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection

[Ambarish Rapte]

On Wed, 15 Apr 2020 10:02:47 GMT, Jeanette Winzenburg <fastegal at 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.
>
> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   ChoiceBox: added FIXME with reference to issue

The change looks good to me, however I have minor concern about selecting Separator, which should be taken in a follow
on issue. It may need a change in `Separator` selection test here.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java line 416:

> 415:                 } else {
> 416:                     toggleGroup.selectToggle(null);
> 417:                 }

The `else` part here means that user programmatically has selected a `Separator` or `SeparatorMenuItem`. The behavior
in such scenario is not defined in doc but the methods, `select()`, `selectNext()`, `selectPrevious()`  of
`ChoiceBox.ChoiceBoxSelectionModel` imply that if index points to a `Separator` then a valid item should be selected.
However these method do handle this correctly.  If these methods are implemented such that no Separator is allowed to
be selected then this `else` part would not be needed and we might be able to remove the `instanceof` checks.

The fix in this PR looks good to me.
But we should also decide the behavior in above scenario and may be file a JBS.
If we decide that when a `Separator` is chosen for selection then the current selection should not be changed or a
valid item should be selected, then the test related to Separator selection need to be changed. Or all of it can be
handled in a follow on issue.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java line 347:

> 346:     // Test only purpose
> 347:     ContextMenu getChoiceBoxPopup() {
> 348:         return popup;

I would recommend to prefix the method name with `test_.` It is not followed across, only `TabPaneSkin` has `test_`
prefixed method.

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

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