Allow listening to file open events on macos

[ArturKovacs]

Closes #1751

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• 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

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]

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

[chrisduerr]

I personally think that adding callbacks to a library that is usually based on an event loop it ships itself makes things extremely annoying to use downstream.

The state management required to integrate with the rest of the application would likely just require locking whenever the shared state is accessed, all of which each downstream user is responsible for themselves.

[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 {...}
}

[chrisduerr]

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

[ArturKovacs]

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

[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.

[chrisduerr]

@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.

[ArturKovacs]

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]

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

[chrisduerr]

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

I'm not really a fan of how this is done in macOS and the resulting implementation in winit. So I personally don't really feel like it's going to be very beneficial to merge it.

[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.

[chrisduerr]

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.

[ArturKovacs]

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]

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.

[ryanisaacg]

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.

[alvinhochun]

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.

[francesca64]

The ffi module is our designated dumping ground for constants and such.

[francesca64]

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.

[chrisduerr]

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.

[francesca64]

@chrisduerr please address my comment above: #1759 (comment)

[ArturKovacs]

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?

[francesca64]

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

[chrisduerr]

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.

[ArturKovacs]

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.

[francesca64]

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.

[ArturKovacs]

I rebased these changes on top of the master and I don't experience the delay at startup anymore.

For the record, the commit onto which this was rebased is 629cd86

[francesca64]

Weird! There's only 3 commits that could've caused that, right? It might be worth bisecting to verify that it's actually fixed and not just an issue that comes and goes.

[ArturKovacs]

I'm assuming you mean these three:
629cd86
ffe2143
4192d04

Although these were also added between creating this PR and rebasing:
ba704c4
0487876
0d634a0

I'm not sure how this could be bisected. If I understand correctly, all changes in this PR are on top of 629cd86, so when checking out a commit before this PR, I can't handle the "file open event". The best idea I had so far is to squash these changes on a local repo and rebase that on top of each of those commits and for each, test if the delay is there.

[ArturKovacs]

I still have a copy of the not-rebased repo locally if that helps. I'm not sure what to do here. I guess the question is how do we become reasonably sure that the delay is fixed. (The difficulty is that it may be spurious happen randomly.) @madsmtm and @francesca64 this might be strange to ask but have you tested it locally? Has the delay never come up for you?

[ArturKovacs]

I learned that I've been using the word "spurious" incorrectly 😬

I seem to remember that @madsmtm you mentioned somewhere that you intend to spend some time with this. (Maybe I only dreamt that :D) I just want to add that it would be very helpful because due to this, the macOS release of emulsion is really lacking.

[madsmtm]

I spent a few hours trying this locally, and couldn't reproduce the delay you're experiencing - I also tried it on the older branch, and it wasn't present there either (I think, there were quite a lot of conflicts and stuff, so maybe I missed something).

So yeah, idk. - it seems to work for me, but I'm not knowledgeable enough in the "savedState" / "restorable" stuff to tell for sure

[Korne127]

Hey; will this still be merged at some point?
I read through this whole conversation and it seems like there has been a consensus to implement this, but it has now been unchanged for two years.
I don't know if it has just been forgotten and overshadowed by other things or if there are reasons why it's in this state and won't be merged (for now).
But I can just say that it looks like much work has been put into this and I'd definitely appreciate this feature to be implemented and file open events to be possible on macOS.

[raphamorim]

Hey; will this still be merged at some point?
I read through this whole conversation and it seems like there has been a consensus to implement this, but it has now been unchanged for two years.
I don't know if it has just been forgotten and overshadowed by other things or if there are reasons why it's in this state and won't be merged (for now).
But I can just say that it looks like much work has been put into this and I'd definitely appreciate this feature to be implemented and file open events to be possible on macOS.

+ 1

And if needs with help testing and updating this PR, I would be glad to take it as well cc @ArturKovacs

[madsmtm]

I've posted some information on how I think the design of this feature should be in #1751 (comment). This does not entirely resolve usability concerns, though I think those are less important than getting something that at least works.

I'm truly sorry for never getting this in, but I think the approach I've suggested there should be a good way forward (and could also set precedent for how we solve similar issues).

I'll close this PR for now though, since a lot of the discussion + code is out of date, and I want to make space for a new implementation.