On Thu, 16 Apr 2020 09:05:12 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> 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 :) 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? ------------- PR: https://git.openjdk.java.net/jfx/pull/177