Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Support open urls macos #3225

Closed
wants to merge 1 commit into from

Conversation

raphamorim
Copy link

@raphamorim raphamorim commented Nov 14, 2023

Draft (open to discussion)

Opened this draft PR but haven't properly tested yet (will do during this week in a terminal emulator I work on), just wanted to confirm is something winit would like to add before proceed

Related raphamorim/rio#107

TODO

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@raphamorim raphamorim requested a review from madsmtm as a code owner November 14, 2023 11:36
///
/// ## Others
///
/// - **Android / Wayland / Windows / Orbital:** Unsupported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - **Android / Wayland / Windows / Orbital:** Unsupported.
/// - **Android / Wayland / Windows / Orbital / Web:** Unsupported.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm out of the loop on this, but I remember in #1759 that just implementing similar methods causes trouble, as now suddenly the application launched differently.

Before we merge this, we'll have to check if that is also the case with application:openUrls:.

Comment on lines +72 to +73
#[method(applicationOpenUrls:)]
fn application_open_urls(&self, urls: Url) -> () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has to match that in icrate::Foundation::AppKit::NSApplicationDelegate::application_openURLs exactly, otherwise it just plain won't work.

Suggested change
#[method(applicationOpenUrls:)]
fn application_open_urls(&self, urls: Url) -> () {
#[method(application:openUrls:)]
fn application_openURLs(&self, application: &NSApplication, urls: &NSArray<NSURL>) {

@madsmtm
Copy link
Member

madsmtm commented Nov 15, 2023

wanted to confirm is something winit would like to add before proceed

Thanks!

I'm not fundamentally opposed to the addition, but I will say that this reminds me too much of the aforementioned issue, which is honestly mentally taxing, so I probably won't really spend much time on this.

Apologies for the early letdown :/

@raphamorim
Copy link
Author

I'm not fundamentally opposed to the addition, but I will say that this reminds me too much of the aforementioned issue, which is honestly mentally taxing, so I probably won't really spend much time on this.
Apologies for the early letdown :/

I understand, thanks for the honesty! Then I think might be better close this PR 🙏

@raphamorim raphamorim closed this Nov 15, 2023
@rib
Copy link
Contributor

rib commented Nov 15, 2023

Just for reference here - I happened to also implement support for application:openUrls in Winit very recently (except currently against winit 0.28 which we're still based on) along with an OpenURL event that is dispatched in response.

I'd be very interested in having something like that upstream at some point.

Currently our macOS application depends on some Apple Script trickery as a workaround for not being hooked into application:openURLs but it would be nicer if we could get an event via Winit instead.

One consideration is that application:openURLs was add in 10.13 where as FEATURES.md currently says winit aims to support macOS 10.7+ - though I'd guess it's probably OK to just not support the event on earlier version instead of adding complexity for those versions.

Especially when we look to porting to Winit 0.29 I'd be very interested in re-visiting this hopefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants