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

[Ajit Ghaisas]

On Wed, 13 May 2020 10:42:41 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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 ;)

I differ on this suggestion.
My thinking is - list access in setFocusedMenuIndex() method should have this check. It is not up to the caller to know
the internal details of the method. That's the root cause of Exception. I added another check in
menuBarFocusedPropertyListener because, it accesses the different list - container.getChildren(). In general, I feel,
the validity check near the list usage is logical and readable as well.

-------------

PR: https://git.openjdk.java.net/jfx/pull/216