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

feat: add support to deeplink and file association on macOS #422

Merged
merged 21 commits into from
Jul 12, 2023

Conversation

meowtec
Copy link
Contributor

@meowtec meowtec commented Jun 12, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

This feature is necessary for the file-association function: open files with tauri-based app.
ref: tauri-apps/tauri#4320

@meowtec meowtec requested a review from a team June 12, 2022 04:12
@meowtec meowtec force-pushed the feat-open-file branch 2 times, most recently from c3d582c to e8ec189 Compare June 16, 2022 02:45
@meowtec meowtec requested a review from a team as a code owner June 16, 2022 02:45
meowtec referenced this pull request in meowtec/tauri Jun 17, 2022
@meowtec meowtec requested a review from a team as a code owner June 23, 2022 15:33
Copy link
Member

@keiya01 keiya01 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. Please check my review.

src/platform_impl/macos/app_delegate.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/app_delegate.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
@FabianLars
Copy link
Member

Did you try using https://developer.apple.com/documentation/appkit/nsapplicationdelegate/2887193-application?language=objc instead?

I'm asking because that's what we need for "deep linking" support and it conflicts with openFile, and i'm not sure yet how we want to handle this conflict. I guess if we can use one interface for both that would be the easiest/best solution.

Anyway, i wanted to try making deep linking work on macos tomorrow, so i can try it out myself too.

@meowtec
Copy link
Contributor Author

meowtec commented Jun 25, 2022

Did you try using https://developer.apple.com/documentation/appkit/nsapplicationdelegate/2887193-application?language=objc instead?

I'm asking because that's what we need for "deep linking" support and it conflicts with openFile, and i'm not sure yet how we want to handle this conflict. I guess if we can use one interface for both that would be the easiest/best solution.

Anyway, i wanted to try making deep linking work on macos tomorrow, so i can try it out myself too.

I did not aware of the conflict before, and i will try openURLs and find how to make both situations work.

@meowtec
Copy link
Contributor Author

meowtec commented Jun 26, 2022

Did you try using https://developer.apple.com/documentation/appkit/nsapplicationdelegate/2887193-application?language=objc instead?

I'm asking because that's what we need for "deep linking" support and it conflicts with openFile, and i'm not sure yet how we want to handle this conflict. I guess if we can use one interface for both that would be the easiest/best solution.

Anyway, i wanted to try making deep linking work on macos tomorrow, so i can try it out myself too.

I have tried openURLs and it seems that we can use openURLs instead of openFile for handling file. When using openURLs, a NSArray<NSURL> are received, in which files are like file:///path/to/file

@meowtec meowtec changed the title feat: add OpenFile Event for macOS feat: add OpenURLs Event for macOS Jun 28, 2022
@lucasfernog lucasfernog changed the title feat: add OpenURLs Event for macOS feat: add OpenFile Event for macOS Oct 5, 2022
@lucasfernog
Copy link
Member

I think OpenFile is the correct way to implement this. Let's implement deep link after this so we can see how to handle it.

Electron has both events btw.

@lucasfernog
Copy link
Member

Actually I saw it gets weird on multiple files :(

This scares me about openURLs urls argument though:

An array of URLs to open. The list does not include URLs for which your app has a defined document type.

@amrbashir
Copy link
Member

electron doesn't mention anything about this in their docs so I wonder how electron behaves if you register for both events, anyways, lets follow the official docs.

And in light of this limitation, we need to use the callback style then like this PR rust-windowing/winit#1759 so it can be opt-in for whichever event you want.

@lucasfernog
Copy link
Member

We could also emit OpenFile if the url is file:// and OpenUrl for other urls. I couldn't find it in the electron codebase, maybe it's related to chromium.

@amrbashir
Copy link
Member

maybe that's something that could be offloaded to tauri instead and tao just allows registering both and documents that only one should be registered.

@Toldoven
Copy link

What is the progress on this issue?

@amrbashir amrbashir requested a review from wusyong January 24, 2023 14:34
@amrbashir
Copy link
Member

@wusyong could you check out this PR when you have some time? especially the OpenFile event and see if modifying the mutable reference success has any effect at all.

@wusyong
Copy link
Member

wusyong commented Jan 26, 2023

@amrbashir How do I test this? Is there an example to run?

@amrbashir
Copy link
Member

@wusyong

use tao::{
  event::{Event, WindowEvent},
  event_loop::{ControlFlow, EventLoop},
  window::WindowBuilder,
};

#[allow(clippy::single_match)]
fn main() {
  let event_loop = EventLoop::new();

  let window = Some(
    WindowBuilder::new()
      .with_title("A fantastic window!")
      .with_inner_size(tao::dpi::LogicalSize::new(128.0, 128.0))
      .build(&event_loop)
      .unwrap(),
  );

  event_loop.run(move |event, _, control_flow| {
    *control_flow = ControlFlow::Wait;
    println!("{:?}", event);

    match event {
      Event::OpenFile { filename, success } => {
        dbg!(filename);
        *success = false; // reject the drop, change to true to accept it
      }
      Event::WindowEvent {
        event: WindowEvent::Destroyed,
        window_id: _,
        ..
      } => {
        *control_flow = ControlFlow::Exit;
      }
      Event::MainEventsCleared => {
        if let Some(w) = &window {
          w.request_redraw();
        }
      }
      _ => (),
    }
  });
}

@wusyong
Copy link
Member

wusyong commented Jan 26, 2023

I tired example above but it couldn't really test if events work. If anyone knows how to test it, please let me know.

AppState::open_file(filename, &mut success);
trace!("Completed `application:openFile:`");

success as i8

Choose a reason for hiding this comment

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

This line throws error since return should be BOOL

@lucasfernog lucasfernog changed the title feat: add OpenURLs Event for macOS feat: add support to deeplink and file association on macOS Jul 3, 2023
@lucasfernog
Copy link
Member

I'm testing it again and seems like openURLs is enough for our use case; The URL scheme is file:// when opening files, and the deeplink scheme otherwise. Did I forget something @FabianLars ? This PR is so old I always forget something, but from my tests it looks enough and we could drop the openFile[s] logic.

@FabianLars
Copy link
Member

I honestly forgot most of it too, but yeah, openurls is enough unless we need to respond to the openfile event (tho tbh i've not seen a usecase for that yet) and and could be used to simplify the implementation 🤷

One thing tho, did you test it with multiple files too? we've had issues with that when we tried to abuse my deep link plugin for that.
Also testing the behavior of different plist configs, like only urls, only files, and having both. Again using the deep link plugin as an example, some users had to add the file scheme to the plist for some messed up reason.

So if all of that works i personally don't see much of a reason to keep openFile/s around

@lucasfernog
Copy link
Member

I did test with multiple files, seems to work fine. I guess we can go with the openURLs approach and wait for community feedback.

@amrbashir
Copy link
Member

I'd support openFile but default to openURLs because this will and should be upstreamed to winit and could serve as a reference the en

@lucasfernog
Copy link
Member

@amrbashir when we upstream we could always use cfc768f (#422) as a reference. But at least for our own use case it seems like openURLs is enough since it also support file opening.

@amrbashir
Copy link
Member

yeah I know it is enough but I never want to restrict tao to an API and if possible allow both ways if it is not much hassle. Just to be safe from a future feature request where someone might request it.

@lucasfernog lucasfernog merged commit 093d8fb into tauri-apps:dev Jul 12, 2023
@github-actions github-actions bot mentioned this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants