winit icon indicating copy to clipboard operation
winit copied to clipboard

Allow listening to file open events on macos

Open ArturKovacs opened this issue 3 years ago • 43 comments

Closes #1751

  • [x] Tested on all platforms changed
  • [x] Compilation warnings were addressed
  • [x] cargo fmt has been run on this branch
  • [x] cargo doc builds successfully
  • [x] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • [x] Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • [x] Created or updated an example program if it would help users understand this functionality
  • [x] Updated feature matrix, if new features were added or implemented

I did not check the example program point because I would indeed like to create an example for this but that would need to be bundled with cargo bundle and a script would be required to patch the Info.plist file in the bundle to allow associating certain file-types with the app. I could create an example program in a separate repo where all this is handled. But I'm not sure how that would be or if that should be referenced from this repo.

ArturKovacs avatar Nov 08 '20 12:11 ArturKovacs

I created an example repo at https://github.com/ArturKovacs/winit_open_file

ArturKovacs avatar Nov 08 '20 15:11 ArturKovacs

That's a fair assesment. I made this implementation based on the discussion at #1751. Would you suggest using a platform specific event API instead? Something like:

enum Event {
   WindowEvent {...}
    // All the other events events 

    PlatformSpecific(PlatformSpecificEvent)
}

enum MacOsEvent {
    ...
}
impl PlatformSpecificEventMacOsExt for PlatformSpecificEvent {
    fn into_macos_event(self) -> MacOsEvent {...}
}

ArturKovacs avatar Nov 08 '20 20:11 ArturKovacs

Events are optional anyways. So it's probably easiest to just emit an event and document clearly when it is emitted.

chrisduerr avatar Nov 08 '20 20:11 chrisduerr

I agree but @alvinhochun seemed to be against adding a variant to Event which only gets triggered on one platform.

ArturKovacs avatar Nov 08 '20 22:11 ArturKovacs

@chrisduerr if you are in executive position I'll just implement this as an event so that this can be closed. Otherwise I'm in a rather uncomfortable situation here as I'm willing to go with either approach but I don't know what I'm supposed to do to get this resolved. I would like to get some guidance for what should be the next step from here.

ArturKovacs avatar Nov 12 '20 20:11 ArturKovacs

@chrisduerr if you are in executive position

I am not. I am not the maintainer of the macOS backend. That would officially be @vbogaevsky I believe, I'm not sure if he's still around. Though if he's not around, I'm probably as good as it's gonna get.

chrisduerr avatar Nov 12 '20 21:11 chrisduerr

I just updated the implementation so that now an application level Event is sent. I also updated the example at https://github.com/ArturKovacs/winit_open_file

ArturKovacs avatar Nov 22 '20 10:11 ArturKovacs

It seems to me that I addressed all the feedback. Is there anything I'm missing?

ArturKovacs avatar Nov 28 '20 17:11 ArturKovacs

To be honest I'm not really a fan either of how this is done in macOS, but if this is not exposed by winit, how should winit applications open associated files on macOS? I personally don't see any other way, and denying this feature would mean that no image viewer, video player, text editor, etc. could be properly made for macOS using winit.

If this PR is not that way to solve the issue, that's fine, but I would be very sad if this wouldn't be solved at all.

ArturKovacs avatar Dec 04 '20 19:12 ArturKovacs

how should winit applications open associated files on macOS

Some things, especially those that do not integrate at all with any other platforms (like this one), are better provided separately outside of winit.

chrisduerr avatar Dec 04 '20 20:12 chrisduerr

Okay, that's reasonable if said feature can be implemented outside of winit. I don't see how this could be done outside of winit while using winit for the rest. Please describe how that would be done.

ArturKovacs avatar Dec 04 '20 20:12 ArturKovacs

I wouldn't want to involve other contributors but we seem to be completely stuck with something that I believe is quite important for macOS applications. @francesca64 and @ryanisaacg you both seem to have the skills and the authority required to help find a solution here, so I'd like to ask you to help.

ArturKovacs avatar Dec 12 '20 22:12 ArturKovacs

Sorry, I've never done any macOS-related development. I just happen to have a mac that I can compile code on, but I don't know anything about mac-specific programming.

ryanisaacg avatar Dec 13 '20 22:12 ryanisaacg

It seems that the problem here is the lack of a generic way for users to handle native events that aren't handled by Winit. Since Winit is in control of the NSApplication delegates, outside code simply can't intercept the macOS-specific openFiles event and others, so this function cannot be implemented externally. (I'm just assuming this is the case.)

If this is true, then Winit should will need to expose these native events for users to intercept them, or perhaps there can be a way to allow the user to set their own NSApplication delegates. Otherwise users won't be able to use Winit to build applications that relies on such platform-specific events.

I don't know macOS application programming at all so I can't really give any more constructive comments.

alvinhochun avatar Dec 14 '20 18:12 alvinhochun

I actually wrote the macOS EL2 backend originally, even though I'm not involved in it much anymore. Feel free to ping me on macOS stuff until we find a new maintainer for it!

Speaking of, @ArturKovacs, would you be interested in maintaining the macOS backend? You seem very thoughtful, and that's the main qualification (besides having time to burn).

The implementation looks reasonable (I can give a proper review later), so it's just the design that's contentious here, wrt platform uniformity? If we don't believe this event will be used on other platforms, we could just add a macOS EventLoopExt method that gives you a receiver for this. The extension traits are basically made for covering things we can't provide a consistent cross-platform API for, and I'd have an easy time accepting this PR if we took that route.

While there's merit in exploring ways that problems like this could be solved outside of winit, I don't think that's necessary in this case. The implementation is small, self-contained, and straightforward, and seems like a normal thing for winit to cover.

francesca64 avatar Dec 14 '20 21:12 francesca64

While there's merit in exploring ways that problems like this could be solved outside of winit, I don't think that's necessary in this case. The implementation is small, self-contained, and straightforward, and seems like a normal thing for winit to cover.

Does it? It's not related to windowing at all, so it seems far more out of scope than many other topics that have been denied.

chrisduerr avatar Dec 14 '20 21:12 chrisduerr

@chrisduerr please address my comment above: https://github.com/rust-windowing/winit/pull/1759#discussion_r542742125

francesca64 avatar Dec 15 '20 18:12 francesca64

Speaking of, @ArturKovacs, would you be interested in maintaining the macOS backend?

This suprised me and made me very excited. I would definitely be interested, yes.

we could just add a macOS EventLoopExt method that gives you a receiver for this.

I would be fine with this. When you say receiver, are you referring to mpsc::channel or just as in some way of informing the client?

ArturKovacs avatar Dec 16 '20 20:12 ArturKovacs

Awesome! You've been memberized. And yeah, mpsc::channel, if that sounds good to you.

francesca64 avatar Dec 16 '20 22:12 francesca64

I think the idea proposed by @alvinhochun makes much more sense for winit. Worrying mainly about windowing needs and allowing applications to hook into the platform specific APIs themselves when necessary.

chrisduerr avatar Dec 16 '20 22:12 chrisduerr

The part that's not clear to me about using a channel is how would we avoid leaking memory if many of these "events" end up being unhandled. We cannot guarantee that the receiver will be pulled from.

In the first commmit of this PR I had an implementation which used the callback approach proposed by @alvinhochun but I didn't make an EventLoopExt, I instead used the EventLoopWindowTargetExtMacOS just because that already existed, and because I don't really know what the difference is. More importantly I made it such that the file open callback gets called on the same thread where the main event handling callback gets called, and this happens while processing all the other events. If we decide to use a callback it seems like a reasonable alternative to require that callback is Send and call it synchronously from application:openFiles and application:openFile. This would allow the user to decide how they want to indicate the success of the operation by, let's say a boolean return value.

Admittedly this would require some method by which the closure gets transferred to the application delegate. Although I have to say I'm a bit confused about what's running on separate threads because I would think that the main thread, the event loop callback thread, and the application delegate thread are the same.

ArturKovacs avatar Dec 17 '20 10:12 ArturKovacs

Ah, that's a super good point. We could just only create the channel if the user calls the method to get the receiver, or are you thinking about the case where the user only pulls from it infrequently?

Either way, I think I actually prefer the sound of the callback approach, since your point about indicating success sounds like something we definitely want.

because I don't really know what the difference is

EventLoopExt is typically used for adding different constructors (which is probably what we'd use if we wanted channels), whereas EventLoopWindowTargetExt gives you methods you can call from within run/run_return, which sounds like the right choice for using callbacks.

Although I have to say I'm a bit confused about what's running on separate threads

Hey, me too! I once observed that the methods on our view were run asynchronously, which was scary... I don't really have a strong mental model of what happens once we cross the barrier into ObjC/AppKit land.

francesca64 avatar Dec 17 '20 19:12 francesca64

@chrisduerr please don't participate in design discussion until after addressing us in https://github.com/rust-windowing/winit/pull/1759#discussion_r544595255. I want to assume good faith, but it's difficult to do that if you won't communicate.

francesca64 avatar Dec 17 '20 21:12 francesca64

I'm sorry, but it's difficult to assume "good faith" from people just trying to stir up drama. I will not be participating in that.

But it doesn't seem like quality is any priority with winit's current leadership, which is quite unfortunate.

chrisduerr avatar Dec 17 '20 21:12 chrisduerr

I’ve implemented the callback method again, but I’m observing a strange behaviour. When I double click on a file in Finder there’s a delay of about 5 seconds between the application appearing at the Dock and its window appearing. If I just run the application directly, this delay isn’t there.

My first guess was that my implementation of the openFiles function was somehow blocking execution. But according to the Console log of the traces, this is not the case. The delay can be seen after the Completed `applicationDidFinishLaunching` message. Also the SecTaskLoadEntitlements failed error=22 message always appears there when opening an associated file.

Click for the log
19/12/20 16:41:49,000 winit_open[0]: hello world
19/12/20 16:41:49,307 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
19/12/20 16:41:49,318 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
19/12/20 16:41:49,000 winit_open[0]: Triggered `viewDidMoveToWindow`
19/12/20 16:41:49,000 winit_open[0]: Completed `viewDidMoveToWindow`
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Locked shared state in `set_fullscreen`
19/12/20 16:41:49,000 winit_open[0]: Unlocked shared state in `set_fullscreen`
19/12/20 16:41:49,000 winit_open[0]: Triggered `windowDidBecomeKey:`
19/12/20 16:41:49,000 winit_open[0]: Completed `windowDidBecomeKey:`
19/12/20 16:41:49,471 appleeventsd[49]: SecTaskLoadEntitlements failed error=22
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Triggered `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Completed `validAttributesForMarkedText`
19/12/20 16:41:49,000 winit_open[0]: Triggered `applicationDidBecomeActive`
19/12/20 16:41:49,000 winit_open[0]: Completed `applicationDidBecomeActive`
19/12/20 16:41:49,000 winit_open[0]: Triggered `application:openFiles:`
19/12/20 16:41:49,000 winit_open[0]: Completed `application:openFiles:`
19/12/20 16:41:49,000 winit_open[0]: Triggered `applicationDidFinishLaunching`
19/12/20 16:41:49,000 winit_open[0]: Completed `applicationDidFinishLaunching`
19/12/20 16:41:54,441 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
19/12/20 16:41:54,442 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
19/12/20 16:41:54,000 winit_open[0]: Triggered `mouseEntered`
19/12/20 16:41:54,000 winit_open[0]: Completed `mouseEntered`
19/12/20 16:41:55,000 winit_open[0]: Triggered `activationHackMouseMoved`
19/12/20 16:41:55,000 winit_open[0]: Completed `activationHackMouseMoved`
19/12/20 16:41:55,000 winit_open[0]: Triggered `mouseExited`
19/12/20 16:41:55,000 winit_open[0]: Completed `mouseExited`
19/12/20 16:41:57,000 winit_open[0]: Triggered `windowDidResignKey:`
19/12/20 16:41:57,000 winit_open[0]: Completed `windowDidResignKey:`
19/12/20 16:41:57,000 winit_open[0]: Triggered `applicationDidResignActive`
19/12/20 16:41:57,000 winit_open[0]: Completed `applicationDidResignActive`
19/12/20 16:43:25,000 winit_open[0]: Triggered `windowDidBecomeKey:`
19/12/20 16:43:25,000 winit_open[0]: Completed `windowDidBecomeKey:`
19/12/20 16:43:25,000 winit_open[0]: Triggered `applicationDidBecomeActive`
19/12/20 16:43:25,000 winit_open[0]: Completed `applicationDidBecomeActive`
19/12/20 16:43:26,000 winit_open[0]: Triggered `mouseEntered`
19/12/20 16:43:26,000 winit_open[0]: Completed `mouseEntered`
19/12/20 16:43:26,000 winit_open[0]: Triggered `mouseExited`
19/12/20 16:43:26,000 winit_open[0]: Completed `mouseExited`
19/12/20 16:43:27,000 winit_open[0]: Triggered `windowShouldClose:`
19/12/20 16:43:27,000 winit_open[0]: Completed `windowShouldClose:`

The log has considerably different contents when there’s no issue during startup (starting the application directly, using the .app).

Click for the log
20/12/20 14:31:27,000 winit_open[0]: hello world
20/12/20 14:31:27,317 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
20/12/20 14:31:27,322 launchservicesd[75]: SecTaskLoadEntitlements failed error=22
20/12/20 14:31:27,000 winit_open[0]: Triggered `viewDidMoveToWindow`
20/12/20 14:31:27,000 winit_open[0]: Completed `viewDidMoveToWindow`
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Locked shared state in `set_fullscreen`
20/12/20 14:31:27,000 winit_open[0]: Unlocked shared state in `set_fullscreen`
20/12/20 14:31:27,000 winit_open[0]: Triggered `windowDidBecomeKey:`
20/12/20 14:31:27,000 winit_open[0]: Completed `windowDidBecomeKey:`
20/12/20 14:31:27,376 appleeventsd[49]: SecTaskLoadEntitlements failed error=22
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Triggered `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Completed `validAttributesForMarkedText`
20/12/20 14:31:27,000 winit_open[0]: Triggered `applicationDidBecomeActive`
20/12/20 14:31:27,000 winit_open[0]: Completed `applicationDidBecomeActive`
20/12/20 14:31:27,000 winit_open[0]: Triggered `applicationDidFinishLaunching`
20/12/20 14:31:27,000 winit_open[0]: Completed `applicationDidFinishLaunching`
20/12/20 14:31:28,000 winit_open[0]: Triggered `keyDown`
20/12/20 14:31:28,000 winit_open[0]: Triggered `hasMarkedText`
20/12/20 14:31:28,000 winit_open[0]: Completed `hasMarkedText`
20/12/20 14:31:28,000 winit_open[0]: Triggered `hasMarkedText`
20/12/20 14:31:28,000 winit_open[0]: Completed `hasMarkedText`
20/12/20 14:31:28,000 winit_open[0]: Triggered `doCommandBySelector`
20/12/20 14:31:28,000 winit_open[0]: Completed `doCommandBySelector`
20/12/20 14:31:28,000 winit_open[0]: Completed `keyDown`

Using the Instrument app I recorded a trace where I could roughly locate the wait interval. The trace includes system calls and the time spent in them.

Screen Shot 2020-12-20 at 14 42 51

Here are the stack traces and durations for those long blocking invocations of these system calls:

First - 4.84 sec:

   0 libsystem_kernel.dylib mach_msg_trap
   1 libsystem_kernel.dylib mach_msg
   2 CoreFoundation __CFRunLoopServiceMachPort
   3 CoreFoundation __CFRunLoopRun
   4 CoreFoundation CFRunLoopRunSpecific
   5 AppKit _NSEventThread
   6 libsystem_pthread.dylib _pthread_body
   7 libsystem_pthread.dylib _pthread_start
   8 libsystem_pthread.dylib thread_start

Second (main thread) - 4.52 sec:

   0 libsystem_kernel.dylib mach_msg_trap
   1 libsystem_kernel.dylib mach_msg
   2 CoreFoundation __CFRunLoopServiceMachPort
   3 CoreFoundation __CFRunLoopRun
   4 CoreFoundation CFRunLoopRunSpecific
   5 HIToolbox RunCurrentEventLoopInMode
   6 HIToolbox ReceiveNextEventCommon
   7 HIToolbox _BlockUntilNextEventMatchingListInModeWithFilter
   8 AppKit _DPSNextEvent
   9 AppKit -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
  10 AppKit -[NSApplication run]
  11 winit_open winit::platform_impl::platform::event_loop::EventLoop$LT$T$GT$::run::he2b3290e18b06902
  12 winit_open winit::event_loop::EventLoop$LT$T$GT$::run::h89caa778cd324342
  13 winit_open winit_open::main::h990a67d0184d3864
  14 winit_open std::sys_common::backtrace::__rust_begin_short_backtrace::h8f105761de818030
  15 winit_open std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h5238b75dd9633a1d (.llvm.3847561433247225113)
  16 winit_open std::rt::lang_start_internal::hd0c760d47f593c0a
  17 winit_open main
  18 libdyld.dylib start

Last - 3.85 sec:

   0 libsystem_kernel.dylib kevent_qos
   1 libdispatch.dylib _dispatch_mgr_invoke
   2 libdispatch.dylib _dispatch_mgr_thread

At this point I'm guessing the answer may lie somewhere in how winit's macos backend works. Do you have an idea about this @francesca64?

ArturKovacs avatar Dec 20 '20 14:12 ArturKovacs

Following this thread it really seems that the delay I experienced was unrelated to winit. The window now appears instantly if I open an associated file from Finder. For me the fix was two part. First I ran

defaults write io.github.arturkovacs.winit_open NSQuitAlwaysKeepsWindows -bool false

But after this, the delay was still present so then I removed the saved state folder using AppCleaner as it was shown in the thread above. This resolved it for me.

ArturKovacs avatar Jan 01 '21 10:01 ArturKovacs

I pushed a new commit which implements the callback design and I updated the example to use this latest commit. Example: https://github.com/ArturKovacs/winit_open_file

ArturKovacs avatar Jan 01 '21 10:01 ArturKovacs

Sorry for the delay!

No worries! I think I addressed all the feedback. 😊

ArturKovacs avatar Feb 06 '21 14:02 ArturKovacs

Also, I would say your winit_open_file example is flawed; most applications should either open a new window for every file:

let window_target = event_loop.window_target();
event_loop.set_file_open_callback(Some(|paths: Vec<PathBuf>| {
    for path in paths.iter() {
        let filename = path.as_os_str().to_owned().into_string().unwrap();
        let title = format!("> Opened: '{}'", filename);
        WindowBuilder::new().with_title(title).build(&event_loop).unwrap();
    }
    FileOpenResult::Success
}));

Or open a single window with all the files (like Preview.app does):

let window_target = event_loop.window_target();
event_loop.set_file_open_callback(Some(|paths: Vec<PathBuf>| {
    let mut title = "> Opened: ".into();
    for path in paths.iter() {
        let filename = path.as_os_str().to_owned().into_string().unwrap();
        title.push_str(&filename);
    }
    WindowBuilder::new().with_title(title).build(&event_loop).unwrap();
    FileOpenResult::Success
}));

(Terrible example code, untested!)

madsmtm avatar Feb 07 '21 10:02 madsmtm

Thank you @madsmtm. I will address your feedback properly at a later time but I just wanted to note that it's probably a good idea to merge #1853 before this so that this can also use the "application stopping user callback panic".

ArturKovacs avatar Feb 07 '21 21:02 ArturKovacs