On Sat, 13 Jun 2020 09:06:01 GMT, Jeanette Winzenburg <faste...@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
