RFR: 8087863: Mac: "Select All" within ListView/TreeView is handled differently depending on the useSystemMenuBar value [v3]

[Martin Fox]

On Tue, 8 Oct 2024 19:21:16 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Martin Fox has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'upstream/master' into sysmenudup
>>  - Added some comments
>>  - Removed redundant check to avoid having the same NSEvent sent into
>>    the scene graph twice.
>>  - performKeyEquivalent and keyDown now do the same thing with a guard
>>    to prevent double-processing.
>>  - Merge remote-tracking branch 'upstream/master' into sysmenudup
>>  - Updated robot test to run on other platforms
>>  - Added Robot test
>>  - If JavaFX processes a shortcut it is not sent on to the system menu bar
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2196:
> 
>> 2194:                 (sceneFocusOwner != null && sceneFocusOwner.getScene() == Scene.this) ? sceneFocusOwner : Scene.this;
>> 2195: 
>> 2196:         if (eventTarget == null) return false;
> 
> minor: insert braces?

This single-line pattern is used throughout this file. I was under the impression that it was within the coding guidelines but can't seem to find a citation for that.

> modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.m line 250:
> 
>> 248:         self->handlingKeyEvent = NO;
>> 249:         self->didCommitText = NO;
>> 250: 
> 
> do we need
> `[lastKeyEvent release];`
> here?

No, this is one-time initialization when the object is first created. We never re-initialize it so we should always set this field to nil.

> tests/system/src/test/java/test/robot/javafx/scene/MenuDoubleShortcutTest.java line 59:
> 
>> 57: 
>> 58:     static volatile Stage stage;
>> 59:     static volatile TestApp testApp;
> 
> +1 for making this field `volatile`

I am almost certainly copied that code from somewhere else so kudos to the original author :-)

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1793700899
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1793701267
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1793701792