On Tue, 29 Apr 2025 22:43:03 GMT, Pabulaner IV <d...@openjdk.org> wrote:
>> When trying to register an open URI handler when using JavaFX with a native >> menu, this task fails on Mac. >> Either the native menu is not shown or the URIs are not received. >> >> This pull request fixes this issue if AWT is registered after JavaFX, so >> that AWT runs embedded inside JavaFX. >> It fixes this by introducing a native event to AWT, which can be used by >> JavaFX to forward events such as an openURL event. >> >> JavaFX Pull Request: https://github.com/openjdk/jfx/pull/1755 >> Co-Author: @FlorianKirmaier > > Pabulaner IV has updated the pull request incrementally with three additional > commits since the last revision: > > - 8332947: [macos] java.awt.desktop.OpenURIHandler is not receiving events > - 8332947: [macos] java.awt.desktop.OpenURIHandler is not receiving events > - 8332947: [macos] java.awt.desktop.OpenURIHandler is not receiving events You have an objective-C bug in two places in your proposed fix. I left a comment with suggestions as to how to update your fix to address the bug. With that updated fix, I am able to run in both modes -- AWT as primary with JavaFX embedded, and JavaFX as primary with AWT embedded -- and see the OpenURIHandler events being delivered. Without these fixes, the OpenURIHandler events are only delivered when AWT is the primary toolkit. I also verified that there are no problems running with a fixed AWT and unfixed JavaFX or an unfixed AWT and fixed JavaFX. When only one of them (JavaFX or AWT) is fixed, OpenURIHandler events are not delivered when JavaFX is the primary toolkit, which is the same behavior as today. So that's good. I left a couple other comments with requested changes. src/java.desktop/macosx/native/libawt_lwawt/awt/ApplicationDelegate.m line 132: > 130: NSNotificationCenter *ctr = [NSNotificationCenter defaultCenter]; > 131: Class clz = [ApplicationDelegate class]; > 132: [ctr addObserver:clz selector:@selector(_embeddedEvent:) > name:@"EmbeddedEvent" object:nil]; `EmbeddedEvent` is too generic a name. I recommend two things: 1. Prefix it with `AWT`, so it would become `AWTEmbeddedEvent` (I think the method name, `_embeddedEvent `, is fine, since it is private to this file) 2. Create a static NSString constant for this string with a comment that it is intended for the primary toolkit to send an event to AWT when AWT is embedded. You probably want to put this string near the top of the file. Related to the second point, this is only needed when running AWT as an embedded toolkit. I recommend putting this registration code in an `if (!isApplicationOwner) { ... }` block. src/java.desktop/macosx/native/libawt_lwawt/awt/ApplicationDelegate.m line 302: > 300: > 301: NSString *url = [[openURLEvent > paramDescriptorForKeyword:keyDirectObject] stringValue]; > 302: [self _openURL:url]; `_openURL` is a class method. Using `self` is incorrect and causes a failure in the normal (non-embedded) case. Suggestion: [ApplicationDelegate _openURL:url]; src/java.desktop/macosx/native/libawt_lwawt/awt/ApplicationDelegate.m line 492: > 490: if ([name isEqualToString:@"openURL"]) { > 491: NSString *url = notification.userInfo[@"url"]; > 492: [self _openURL:url]; `_openURL` is a class method. Using `self` is incorrect (technically, it works here because the caller is also a class method in the same class, but it's still a mistake). Suggestion: [ApplicationDelegate _openURL:url]; ------------- PR Review: https://git.openjdk.org/jdk/pull/24379#pullrequestreview-2819539381 PR Review Comment: https://git.openjdk.org/jdk/pull/24379#discussion_r2076217229 PR Review Comment: https://git.openjdk.org/jdk/pull/24379#discussion_r2076219525 PR Review Comment: https://git.openjdk.org/jdk/pull/24379#discussion_r2076221920
