On Fri, 12 Jun 2020 21:17:09 GMT, Kevin Rushforth <[email protected]> wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review fixes and test addition
>
> 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).
-------------
PR: https://git.openjdk.java.net/jfx/pull/216
