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

[Ambarish Rapte]

On Thu, 16 Apr 2020 09:17:19 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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 :)
>
> btw: just noticed that there are methods in ChoiceBoxSkin testing the fix for next/prev
> 
>     @Test public void test_jdk_8988261_selectNext() {
>     @Test public void test_jdk_8988261_selectPrevious() {
> 
> the name look like they want to point to the corresponding issue .. but the id is incorrect: that id doesn't exist,
> should be 8088261 (spelling error, I think) - is it okay to change them to the right id?

>     1. do nothing for a (don't feel like filing yet another bug around selection ;) and b (the skin behaves correctly, I
>     think)

I am good with this. Though I will file a JBS for the correction in ChoiceBoxSelectionModel.
`seletPrevious()`, `selectNext()` need one more check `value instanceof SeparatorMenuItem`.
and similarly `selectFirst()` and `selectLast()` should be overridden correctly.
and I can't think of why `select()` was changed so may be rethink about it :).
We can discuss it again whenever we start fixing it.


>     2. fix the test to be resistant against implementation changes of selectionModel

Thanks for link to [JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261), As mentioned in this bug
description, "Culprit is an incorrect override of select(int): it may reject the new index if that would select a
separator, but it must not select an arbitrary index instead",  So It is not sure to me what should `select()` do in
such scenario. So I think the test can also go as is, in case we change the behavior then test can be changed with it.

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

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