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

[Jeanette Winzenburg]

On Wed, 15 Apr 2020 15:46:16 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   ChoiceBox: added FIXME with reference to issue
>
> 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.

yeah, you are right:

a) the implementation of ChoiceBoxSelectionModel is broken when it comes to handling of unselectable items (such as
Separator): next/previous try to move on, the others simply select. The implementation changed in fix of
[JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261) - before select(index) tried to handle it, after this
was moved into next/previous. Arguably, the model can do what it wants without specification ;)

b) the skin is responsible to sync the selection state of its toggles with the state of model: if the
selectedIndex/Item does not have a corresponding toggle (f.i. if it's a separator), all toggles must be unselected.

c) my test related to the Separator is broken - as you correctly noted, it will fail if a future implementation decides
to select a really selectable item :)

My plan:

1. do nothing for a (don't feel like filing yet another bug around selection ;) and b (the skin behaves correctly, I
think) 2. fix the test to be resistant against implementation changes of selectionModel

Thanks for the extensive review, very much appreciated :)

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

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