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

Support handling any event on macOS #2141

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ArturKovacs
Copy link
Contributor

Implements #2120 for macOS

  • 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

@ArturKovacs ArturKovacs marked this pull request as draft January 8, 2022 18:55
@ArturKovacs
Copy link
Contributor Author

@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
https://github.com/rust-windowing/winit/pull/2141/files#diff-e84f3d0ef4443d7dac0b6443d49905fc86537ac06a4b3e1cbf5a79988962f223R217-R231

@maroider
Copy link
Member

maroider commented Jan 9, 2022

The idea is that I always declare a new delegate class when a new callback is added.

Can you base this off of the EventLoopBuilder added in #2137 instead, and then create a single delegate class for downstream callbacks?

@ArturKovacs
Copy link
Contributor Author

The idea is that I always declare a new delegate class when a new callback is added.

Can you base this off of the EventLoopBuilder added in #2137 instead, and then create a single delegate class for downstream callbacks?

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 madsmtm added the C - needs discussion Direction must be ironed out label Jan 10, 2022
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'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.

  1. with_application_method communicates the correct intent. I would not want users to think that they can dynamically call set_application_method inside EventLoop::run and 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).
  2. 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).
  3. There's a difference between having a delegate method, and not having it, see Allow listening to file open events on macos #1759 (comment), and changing that dynamically will be very confusing.
  4. We will want to do add at least with_view_method/set_view_method as well (see Platform-specific event callbacks #2120 (comment)), where these issues would be even more pronounced.
  5. 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.
  6. 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,
Copy link
Member

Choose a reason for hiding this comment

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

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 😁).

@@ -179,6 +191,112 @@ impl WindowBuilderExtMacOS for WindowBuilder {
}
}

pub trait DelegateMethod {
fn register_method<T>(self, sel: Sel, el: &mut PlatformEventLoop<T>) -> Result<(), String>;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be __-namespaced and #[doc(hidden)]

@ArturKovacs
Copy link
Contributor Author

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

@madsmtm
Copy link
Member

madsmtm commented Jun 24, 2024

Superseded by #3758, where we instead allow the user full control over the application delegate (by making Winit register for notifications instead).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out DS - macos
Development

Successfully merging this pull request may close these issues.

None yet

3 participants