RFR: 8087555: [ChoiceBox] uncontained value not shown

[Jeanette Winzenburg]

On Fri, 24 Apr 2020 14:05:27 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

> I have not reviewed the code but have only tested it.

Thanks for taking an initial look :) Just keep in mind that this issue is focused on displaying of the box' value
reliably.

> The value of index is not defined when an uncontained item is selected.

yeah the spec is rather .. well, suboptimal at some crucial points (see also
[JDK-8088012](https://bugs.openjdk.java.net/browse/JDK-8088012)). To keep the state logically consistent, we need an
invariant like:

     assertEquals(getItems().indexOf(selectedItem), selectedIndex)

Which seems to be hinted at in a code comment in `SingleSelectionModel.select(item)`:

        // if we are here, we did not find the item in the entire data model.
        // Even still, we allow for this item to be set to the give object.
        // We expect that in concrete subclasses of this class we observe the
        // data model such that we check to see if the given item exists in it,
        // whilst SelectedIndex == -1 && SelectedItem != null.

For ChoiceBox, that will be fixed in [JDK-8241999](https://bugs.openjdk.java.net/browse/JDK-8241999) (ready for push as
soon as this is integrated).

> With the current implementation (with and without this PR change) there is a difference of behavior when an uncontained
> item is selected Vs when an existing item is selected, in scenarios as below,
> => clearSelection()
> a) scenario 1
> 
>     1. Select/set an uncontained item. `selectedIndex` would be -1.
> 
>     2. call `clearSelection()`, Does NOT clear the selected item to null

true, that's spec'ed in ComboBox only .. so by analogy I considered it being the same here. Hmm .. might need an update
of the doc?

>        b) scenario 2
> 
>     3. Select an item at index 2, `selectedIndex` would be 2.
> 
>     4. Select/set an uncontained item. `selectedIndex` would remain 2.
> 

exactly that is [JDK-8241999](https://bugs.openjdk.java.net/browse/JDK-8241999)

>     5. call `clearSelection()`, => clears the selected item to null  and selected index to -1
> 

interesting, hadn't noticed yet - will add a test to the other fix, thanks :)

> 
> and similarly the difference of behavior can be observed with `selectNext()`, `selectPrevious()`
> 

yeah, that's another issue .. selection issues are a bottomless pit ..

> This is not formally documented(nor decided), so we might need to decide on a value of index for an uncontained value.

see the "natural" constraint above - without we get into hell, IMO.

Good points all, thanks again!

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

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