On Tue, 8 Oct 2024 19:21:16 GMT, Andy Goryachev <ango...@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
