On Fri, 12 Jun 2020 21:17:09 GMT, Kevin Rushforth <k...@openjdk.org> 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