On Wed, 15 Apr 2020 15:46:16 GMT, Ambarish Rapte <ara...@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
