On Fri, 24 Apr 2020 14:05:27 GMT, Ambarish Rapte <[email protected]> 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
