[Rev 01] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar

[Jeanette Winzenburg]

On Tue, 12 May 2020 12:21:14 GMT, Ajit Ghaisas <aghaisas at 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