On Thu, 16 Apr 2020 09:17:19 GMT, Jeanette Winzenburg <faste...@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
