Support handling any event on macOS#2141
Support handling any event on macOS#2141ArturKovacs wants to merge 5 commits intorust-windowing:masterfrom
Conversation
two parameters should now be possible
by the application
|
@madsmtm I think I'm making good progress with this but I hit an issue that I don't understand. The idea is that I always declare a new delegate class when a new callback is added. The new delegate subclasses the previous one to allow having mutliple callbacks on one method. This also means that the delegate handler must call the superclass' method. But when I do this, it recurses infinitely and I get a sweet stack overflow. If you execute the "native_events" example it hits an infinite recursion. But as far as I understand, it shouldn't. Here's where the recurson happens, but I don't get why |
Can you base this off of the |
Technically yes, but that would make a very unpractical API for users. See: #2120 (comment) But anyways, I think I managed to resolve (ie work around) the issue. |
madsmtm
left a comment
There was a problem hiding this comment.
I'd really like to register the methods before the event loop is created (EventLoopBuilder::with_application_method instead of EventLoop::set_application_method). I know I've said this before, but let me try to properly argue why I believe it is the right course of action:
My starting point is your motivating example of accessing a window inside a delegate method, in the end I believe the downsides below outweigh the usability gain.
with_application_methodcommunicates the correct intent. I would not want users to think that they can dynamically callset_application_methodinsideEventLoop::runand expect it to work (I think it could technically be supported, but it is a can of worms that I'm not prepared to open).- There is no guarantee (at least not that I know of) that delegate methods are not called before
EventLoop::run, which means that your example of creating a window and then accessing it inside the method is not guaranteed to work (e.g. the behaviour could change between macOS versions, or different selectors might work differently). - There's a difference between having a delegate method, and not having it, see #1759 (comment), and changing that dynamically will be very confusing.
- We will want to do add at least
with_view_method/set_view_methodas well (see #2120 (comment)), where these issues would be even more pronounced. - This is a minor point, but windows really should be created in
Event::NewEvents(StartCause::Init), and allowing them to be accessed in e.g.application:openFile[s]:is a footgun that I'd like to push people away from. - Implementation-wise I'd also like to avoid creating a new class every time someone adds a new callback, not because of performance concerns, but because I don't think it is maintainable long term (however cool it may be 😁).
This said, I do believe the delegate methods should be modified to take &EventLoopWindowTarget as their first argument, allowing access to the event loop is definitely something we want to allow.
Anyhow, thanks a lot for starting the work on this, and I hope you know the above is not intended to shut down the discussion, it is merely my view on it 😉.
Related to PR, unrelated to discussion: See also the dynamic method resolution features that Objective-C natively supports; though it may not be relevant in our case if we want to allow users to hook into events that winit already handle.
| /// > Unsafe because the caller must ensure that the types match those that are expected when the method is invoked from Objective-C. | ||
| unsafe fn add_application_method<F: DelegateMethod>( | ||
| &mut self, | ||
| sel: Sel, |
There was a problem hiding this comment.
Let's not expose objc as a public dependency; use sel: *const c_void instead.
It may be updated, and then users would have to always use the same version that winit uses. Also, it would now be a breaking change to update our version of objc (or swap it out with something else like my fork, though I realise that's an argument that mostly benefits me 😁).
| } | ||
|
|
||
| pub trait DelegateMethod { | ||
| fn register_method<T>(self, sel: Sel, el: &mut PlatformEventLoop<T>) -> Result<(), String>; |
There was a problem hiding this comment.
This should probably be __-namespaced and #[doc(hidden)]
|
@maroider Had an idea for how we could change things so that all of us can be happy. I won't go into the details but the fundamental idea is that there is a user-defined State object that the event loop owns and passes to each user callback as a parameter, so that things created after the event loop, can still be provided to custom event closures. But I think we gonna have to wait for @maroider to make the proposal officially to discuss it. |
|
Superseded by #3758, where we instead allow the user full control over the application delegate (by making Winit register for notifications instead). |
Implements #2120 for macOS
CHANGELOG.mdif knowledge of this change could be valuable to users