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

[Kevin Rushforth]

On Sat, 13 Jun 2020 09:06:01 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 764:
>> 
>>> 763:     void setFocusedIndex(int index) {
>>> 764:         this.setFocusedMenuIndex(0);
>>> 765:     }
>> 
>> Shouldn't this be `this.setFocusedMenuIndex(index)`? The only reason this isn't causing problems is that the test you
>> added never calls it with anything other than 0.
>> Also, the naming of this method is a bit confusing. I might recommend calling it `test_setFocusedMenuIndex`, or else
>> just change the private `setFocusedMenuIndex` method to be package-scope. Either way adding a comment indicating that
>> it is used for testing purposes seems a good idea (if you keep this method then the comment could say that it is only
>> for testing purposes; if you make `setFocusedMenuIndex` package-scope then you could say that it is package scope
>> because it is used for testing).
>
> darn .. overlooked that fixed index ..
> 
> As to the naming: personally, I hate violation of naming conventions even for test-only methods, so would tend to make
> setFocusedMenuIndex  package and doc as being used for testing as well as internally.
> We might consider aligning the corresponding methods in the shim (they are also slightly confusing in
> get/set/FocusedIndex).

Good suggestion.

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

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