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

[Jeanette Winzenburg]

On Fri, 15 May 2020 09:19:19 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> 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.

hmm .. yeah I'm aware of getContainer vs. getMenus - but they should be the same size, shouldn't they?

Anyway, if focusedIndex != getMenus().indexOf all users of focusedIndex have to include a check for validity. That
might be prevented by not allowing it here in the method, in setting its value not unconditionally to the given index
but guard it against being valid:

    focusedMenuIndex = index >= getMenus().size() ? -1 : index

Then focused is either valid or -1.

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

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