On Tue, 12 May 2020 12:21:14 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
>> line 485:
>>
>>> 484: }
>>> 485:
>>> 486: if (focusedMenu != null && focusedMenuIndex != -1) {
>>
>> don't quite understand why this change is necessary? The guard in the
>> listener seems to be enough, at least the test
>> passes without this. If it's needed to fix another potentially breaking path
>> to this, would it be possible to add a
>> test method that fails/passes before/after?
>
> This was the place where exception was occurring. Hence, I added this check.
> When I ran against the test, I still got the exception at caller lambda. I
> added that check later.
> Wanted to remove this check - but simple code scan revealed this method gets
> called from engine.addTraverseListener()
> listener with index = 0. Hence, I have kept this check. Covering this
> scenario in test seems tough.
Hmm .. re-reading my comment: was assuming and then formulating round a corner,
sry. Will try again
My assumption (from the method implementation before the fix) was that the
method expects only valid indices (either -1
or an index in range) such that
assertEquals(focusedMenuIndex, getMenus().indexOf(focusedMenu);
Which would leave the validity check to the caller.
With that assumption, the old implementation were just fine. The one location
that calls it with an invalid index is
the one you fixed (not called if empty), the other is the traversalListener
(which I silently and incorrectly dropped
from my mind, because it's never notified anyway - which is nothing but a bold
guess ;)
With the change, the constraint doesn't hold (as it didn't before if the caller
passed in an invalid index) it just
doesn't blow. Not sure what - if any - bad might happen if we have a
focusedMenuIndex >= getMenu().size() (in
particular empty menus).
The other way round: the method might take the responsibility to protect itself
(no precondition and the postcondition
to guarantee the constraint above).
My preference would be to keep the method as is, and change the callers to
check for a valid index. Hard to test, one
way or other ;)
-------------
PR: https://git.openjdk.java.net/jfx/pull/216
