On Thu, 16 Apr 2020 11:18:55 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> 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.
> should be 8088261 (spelling error, I think) - is it okay to change them to
> the right id?
That will be good to change, but not sure if as part of this bug. It will be
unrelated to fix.
-------------
PR: https://git.openjdk.java.net/jfx/pull/177
