On Fri, 8 May 2020 13:51:34 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review_fixes
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 485:
>
>> 484: }
>> 485:
>> 486: if (focusedMenu != null && focusedMenuIndex != -1) {
>
> don't quite understand why this change is necessary? The guard in the
> listener seems to be enough, at least the test
> passes without this. If it's needed to fix another potentially breaking path
> to this, would it be possible to add a
> test method that fails/passes before/after?
This was the place where exception was occurring. Hence, I added this check.
When I ran against the test, I still got the exception at caller lambda. I
added that check later.
Wanted to remove this check - but simple code scan revealed this method gets
called from engine.addTraverseListener()
listener with index = 0. Hence, I have kept this check. Covering this scenario
in test seems tough.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
> line 307:
>
>> 306: unSelectMenus();
>> 307: if (!container.getChildren().isEmpty()) {
>> 308: menuModeStart(0);
>
> wondering whether this would be an appropriate time to simplify the logic a
> bit: as unSelectMenus is called in both if
> and else block, it could be condensed to
> unSelectMenus();
> if (t1 && !container.getChildren().isEmpty()) {
> ...
> }
>
> might overlook something, though
Good suggestion. Fixed.
-------------
PR: https://git.openjdk.java.net/jfx/pull/216
