Platform-specific event callbacks

[maroider]

Issues like #1878, #1928, and #1759 have made it clear that there is a need to hook into platform-specific events that winit itself doesn't (and arguably shouldn't) handle. Ideally, we'd give users the ability to hook into these events with minimal effort, and in a way that doesn't interfere with winit (although some would perhaps like to do that as well).

The simplest solution I can think of to this is to add an EventLoopBuilder struct where you can register platform-specific callback functions before the event loop is ever entered. This will make it so that these callbacks don't miss any events, like the file opening events in #1759.

Granted, this doesn't lend itself to being a particularly ergonomic API, as any data shared between the platform-specific callbacks and the platform-independent on would have to be behind something like an Rc<RefCell<T>> or an Arc<Mutex<T>>. I think there may exist a solution where sharing such state is easier, but I don't want to spend much energy on that ATM.

Another issue is that this will (by necessity) make some projects (like AccessKit) end up depending directly on winit or have to provide an adapter crate. Maybe it could be possible to pull a raw-window-handle and create some cross-windowing-library interface for this, but I don't think it's ultimately going to be all that feasible.

The Windows backend for this should be fairly trivial, so I'll go and implement that soon-ish, unless there are any major concerns with this proposal.

This is on some level related to #2010, but this concerns the public API, rather than the internal one.

[maroider]

cc @mwcampbell @fschutt

[maroider]

The minimal API for Windows would look something like this:

impl WindowsEventLoopBuilder for EventLoopBuilder {
    pub fn with_callback<F>(&mut self, msg_mask: u32, callback: F)
    where
        F: FnMut(*mut c_void, u32, usize, usize) -> usize,
    {
    }
}

Although something should probably be done to let end users correlate the WindowId with the HWND (the *mut c_void in this case) more easily.

[mwcampbell]

Thanks @maroider for working on this.

I'll settle for having a separate accesskit_winit crate rather than having any of my existing crates depend directly on winit.

I assume the *mut c_void parameter to the callback is an HWND. If so, that looks fine.

[ArturKovacs]

The API you proposed for Windows looks totally reasonable. Maybe it would be better to pass in the WindowId instead of the HWND and provide a platform specific extension trait for getting the HWND from the WindowId.

I would be happy to implement this for macos.

[mwcampbell]

I like the idea of using WindowId. Among other benefits, it's hashable, so my code will be able to look up its own struct associated with the window.

[ArturKovacs]

I started implementing something that would allow this on macOS. It can be found at https://github.com/ArturKovacs/winit/tree/macos-any-event

The difficulty is that one has to define methods in the application delegate or NSView for the event that one is interested in.

To allow the user to do this, we define a new application delegate class during runtime when the user wants to register a new event handler.

The API is fairly simple though:

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

// Implement DelegateMethod for a bunch of closure types

pub trait EventLoopExtMacOS {

    /// Adds a new callback method for the application delegate.
    fn add_application_method<F: DelegateMethod>(&mut self, sel: Sel, method: F) -> Result<(), String>;
}


/////////////////////////////////////
// main.rs

use winit::{
    platform::macos::{EventLoopExtMacOS, objc::{self, sel, sel_impl}}
};

fn main() {
    //...
    let mut event_loop = EventLoop::new();

    event_loop.add_application_method(
        sel!(applicationDidChangeOcclusionState:),
        Box::new(|_notification: *mut objc::runtime::Object| {
            println!("The occlusion state has changed!");
        }) as Box<dyn Fn(_)>
    ).unwrap();

    //...
}

[madsmtm]

I agree that this is something that we need, would the intention then be to move e.g. #1759 to a separate crate that depends on winit?

I haven't really thought deeply about it but I think I agree with @ArturKovacs for the macOS approach, though with the addition that we move the initialization of APP_DELEGATE_CLASS to EventLoop::new / EventLoopBuilder::build, so that we don't need to register multiple application delegates. Also, add_application_method is definitely unsafe!

[maroider]

I agree that this is something that we need, would the intention then be to move e.g. #1759 to a separate crate that depends on winit?

I've had thoughts along the lines of compiling commonly-wanted functionality that doesn't quite make the cut for inclusion in winit proper into something like a winit_extras crate, which could include file open events on macOS, as well as file dialogs, message boxes, and platform-native menu bars.

[mwcampbell]

This all sounds good. For what it's worth, AccessKit for Mac will need to override some methods on the NSView object. I'm not yet sure which ones.

[ArturKovacs]

though with the addition that we move the initialization of APP_DELEGATE_CLASS to EventLoop::new / EventLoopBuilder::build, so that we don't need to register multiple application delegates.

If I understand correctly, this will suffer from the same problem that we discovered with the "file open callback" PR. In short the problem is that it's going to make it very difficult to refer to instances of Window. It's described here in more detail in my original comment that I copy-pasted below for readablility (source #1759 (comment))

I hit a problem. The window needs an EventLoop when it's constructed. So new_with_file_open_callback must be called before the window exists, which leads to a very awkward solution if one desires to use anything depending on the event loop in the callback. The code below is the nicest approach I could come up with, and it's a mess. The Mutex<Option<>> part of the type wouldn't be needed if this could be called after the window was created, so that the closure could directly take a clone of Arc<Window>.

let window_ref: Arc<Mutex<Option<Window>>> = Default::default();
let event_loop;
{
    let window_ref = window_ref.clone();
    event_loop = EventLoop::<()>::new_with_file_open_callback(move |paths: Vec<PathBuf>| {
        let guard = window_ref.lock().unwrap();
        let window = match &*guard {
            Some(window) => window,
            None => return FileOpenResult::Failure,
        };
        for path in paths.iter() {
            open_file(window, path.as_ref());
        }
        FileOpenResult::Success
    });
}

Probably the best is to have a set_file_open_callback on the EventLoop.

---

Also, add_application_method is definitely unsafe!

Why? As far as I understand only those functions need to be unsafe where the function makes assumptions that aren't guaranteed to be true and that the function doesn't check. As far as I can tell no such assumption is made by this function.

[madsmtm]

Ah, right, the callback has to actually be usable... Maybe we could somehow inject the current event loop as a parameter to the closure, but let's defer that discussion to when we get an actual PR up n' running.

Why? As far as I understand only those functions need to be unsafe where the function makes assumptions that aren't guaranteed to be true and that the function doesn't check. As far as I can tell no such assumption is made by this function.

Because this is effectively equivalent to ClassDecl::add_method, and hence inherits the (un)safety of that. To paraphrase; there is no restriction on the types that the closure takes.

In your example, the closure takes _notification: *mut objc::runtime::Object, but one could just as well have written _notification: bool, and that would be UB.

[kchibisov]

I'm not sure that it could be done on Wayland at all. The thing is that there's no thing like getting extra events that winit can't handle, since winit asks only for specific events and handle all of them. As for extra events, well, you basically create one more event queue downstream and add things to it and since winit owns the main queue (which dispatches events to your queue) winit will wakeup naturally.

But it's not like there's a strong need for that on Wayland as well, so it shouldn't block on anything, since it's platform-specific.

[maroider]

This is an indirect response to this comment

So this whole platform-specific callback thing has a major usability issue as-is: it's kind of annoying to share state between the platform-specific callback and the main event loop callback. I did initially have a more comprehensive proposal in mind, but in the interest of moving this along, I've chosen to only focus on the simplest one instead.

The basic idea is to add another type parameter S to EventLoop and EventLoopBuilder representing said shared state, which all callbacks then receive as &mut S (might have to be a &S if we determine that passing in &mut S can't be done).

pub struct EventLoopBuilder<T, S=()> { .. }

impl<T> EventLoopBuilder<T, ()> {
    // `init_fn` should be called as late as possible when
    // initializing the event loop, but early enough that
    // any platform-specific callbacks can receive `&mut S`.
    // This lets users stuff a plain `Window` into whichever
    // type they specify as `S`, without having to resort to
    // `Option<T>` or similar constructs.
    pub fn with_custom_state<S, F>(self, init_fn: F) -> EventLoopBuilder<T, S>
    where
	F: FnOnce(&EventLoopWindowTarget) -> S
    { .. }
}

impl<T, S> EventLoopBuilder<T, S> {
	pub fn build(self) -> EventLoop<T, S> { .. }
}

impl<T, S> EventLoopBuilderExtWindows for EventLoopBuilder<T, S> {
    // Passing `&mut S` here may be suspect for a couple of reasons:
    //   1. `WndProc` is re-entrant
    //   2. Users may want to have their callback called while we're inside
    //      `CreateWindowExW` (since `CreateWindowExW` dispatches some rather
    //      important events before returning), but this becomes a bit of a
    //      conundrum if `S` is supposed to contain their one and only window.
    // Issue no. 1 should be easy to mitigate with existing checks used on the
    // main callback.
    pub fn with_callback<F>(self, msg_mask: u32, callback: F)
    where
        F: FnMut(&mut S, WindowId, u32, usize,   usize) -> usize,
    { .... }
}

[mwcampbell]

FWIW, initializing accessibility inside the call to CreateWindowExW only seems to be necessary if the window is created with the WS_VISIBLE style already set. If the window is created hidden, one can initialize accessibility outside of the window creation proper, then show the window, and it seems that there's no inconsistency observable by the screen reader or other assistive technology.

[maroider]

The HWND can be obtained through the WM_NCCREATE and WM_CREATE messages, which are sent during the call to CreateWindowExW

Is there some paramter or struct field I've missed somehow? I can only find a field for the "parent/owner" HWND. It appears that I'm being a bit of an idiot. The HWND should be passed directly to our wndproc.

[mwcampbell]

Well, the HWND is the first parameter to the window proc itself. Combine that with the lpCreateParams field in CREATESTURCTW, which corresponds to the lpParam parameter to CreateWindowExW, and you can associate that message with a particular call to CreateWindowExW.

Note that winit already handles both WM_NCCREATE and WM_CREATE. The WM_NCCREATE handler passes the HWND to InitData::on_nccreate.

[Liamolucko]

I've come up with a half-baked proposal for a slightly higher-level API for this.

Basically, allow registering EventListeners, a trait which allows registering custom callbacks which produce events that get forwarded to the main event loop. Implementations of this trait then more-or-less act as plugins for winit.

A rough sketch of the API (I don't know most of the platform-specific details):

pub trait EventListener {
    // Whatever event type is produced by this event listener.
    type Event;

    fn register_macos_callbacks(registrar: MacOSEventRegistrar<Self>) {}
    fn register_windows_callbacks(registrar: WindowsEventRegistrar<Self>) {}
    // other platforms...
}

// Based on https://github.com/rust-windowing/winit/issues/2120#issuecomment-1003594325.
impl<T: EventListener> MacOSEventRegistrar<T> {
    pub fn add_application_method(
        sel: Sel,
        // The handler gets a mutable reference to the `EventListener`,
        // so that it can keep state, and is passed a callback to send an event to the event loop.
        // (Note that this uses a callback instead of just returning events so that things like
        // `ScaleFactorChanged` with lifetimes will work.)
        handler: impl FnMut(&mut T, /* actual callback arguments somehow */, impl FnMut(T::Event)),
    ) {
        /* ... */
    }
}

impl<T> EventLoopBuilder<T> {
    fn add_event_listener<L>(listener: L)
    where
        L: EventListener,
        // Most of the time, this would probably produce `UserEvent`s.
        L::Event: Into<Event<T>>,
    {
        /* ... */
    }
}

// Example event listener:
struct CountHides {
    hides: u32,
}

struct HideEvent(u32);

impl From<HideEvent> for Event<HideEvent> {
    fn from(e: HideEvent) -> Self {
        Event::UserEvent(e)
    }
}

impl EventListener for CountHides {
    type Event = HideEvent;

    fn register_macos_callbacks(registrar: MacOSEventRegistrar<Self>) {
        registrar.add_application_method(sel!(applicationDidHide:), |this, _, callback| {
            this.hides += 1;
            callback(HideEvent(this.hides));
        });
    }
}

Then, all of the builtin callbacks could be switched to enabled-by-default EventListeners, allowing them to be disabled for cases where having the event handlers present causes unwanted side effects (mentioned in #2010).

[rib]

I wonder if instead of adding a separate callback (which makes it awkward to handle cross talk between handling platform vs common events) we could instead look at adding a general Event::Platform(PlatformEvent) (that can be delivered to the existing callback) that gives each backend some freedom to expose backend-specific events.

On Android for example it would be good to be able to emit an event for inset changes (such as when a soft keyboard is shown), or forward LowMemory, ConfigChanged or SaveState events.

This wouldn't be a general purpose, extensible mechanism for integrating things like file descriptors with the event loop but would make it possible for backends to expose window system and life cycle events that are important for a given platform or backend, when it doesn't really make sense to force a generic, platform-agnostic abstraction.

[kchibisov]

I wonder if instead of adding a separate callback (which makes it awkward to handle cross talk between handling platform vs common events) we could instead look at adding a general Event::Platform(PlatformEvent) (that can be delivered to the existing callback) that gives each backend some freedom to expose backend-specific events.

I agree with this, we don't need callbacks at all, winit is not a callback driven and adding callbacks is just a mess. What we want is a separate event for such things.

[ArturKovacs]

In an attempt to move this forward, I'm going to try to summarize the arguments for using callbacks and their counter-arguments.

There are too many kinds of events on some platforms for winit to handle

This only seems to be true for Windows - but on Windows, we can completely sidestep this problem by simply sending WindowEvent::Platform({msg: u32, lparam: usize, wparam: usize}) for every event under the sun.

Some events need a response from the application

The solution is to call the user callback synchronously (The user callback must return before the original event handler returns. An example of this is the Resized event on Windows) In some cases this response could be communicated back to winit through a mutable reference (like it was done for ScaleFactorChanged)

On macOS, the behavior is different depending on whether a callback is registered or not

This can be dealt with by different means (for example by having functions like allow_open_files(allowed: bool) that controls wether the AppDelegate even has the handler method)

---

All-in-all I'm in favour of whichever approach - it would just be nice to see this move forward. Let's have a vote for the fun of it:

• All in favour of Event variants, react with 🚀
• All if favour of callback functions, react with 🎉
• Either works for me, just let's move forward 😄

[mwcampbell]

My rationale for event variants: The existing winit event handler callback is the one place in a typical winit-based application that has access to mutable application state, without having to use reference counting and interior mutability. As such, this makes it easier to integrate custom event handling that may modify an application's state, such as lazily and synchronously initializing an accessibility tree, into an existing architecture.

In addition to covering custom events on Windows (with an out parameter for the LRESULT), event variants could also be stretched to cover arbitrary selectors on the NSView on macOS. The application could register its desire to handle certain selectors, and then messages to those selectors could be delivered synchronously as event variants, again with an out parameter for the result.

Use of event variants for both custom Win32 messages and custom NSView selectors would allow a no-compromise integration of accessibility providers such as AccessKit, complete with lazy and synchronous initialization of accessibility support without contorting the architecture to allow a separate callback to modify application state, without requiring winit to be directly coupled to AccessKit. For that reason alone, I strongly recommend proceeding with this approach.

[kchibisov]

I would suggest custom per platform events, since only windows/x11 has a common source of all events. On Wayland everything is very strict. But for custom events I can just move their any arbitrary stuff into it without bringing such event into other platform.

[mwcampbell]

Unfortunately, I think I've identified a fatal flaw in the idea of extending winit's existing event handling to cover platform-specific events, particularly those that require synchronous handling with a result, such as the Windows WM_GETOBJECT message (used for accessibility). The problem is that a window can be created and shown, possibly triggering such events, before the application has provided its event callback to winit. The only way I can see of fixing that would be to require that applications not create their first window until the event loop has started (which would have to be indicated by a new event such as Event::LoopStarted). But that's probably drastic enough that it won't be seriously considered.

[rib]

One solution for this is to have events that effectively have an out parameter of some kind.

The event handling in android-activity (used in the Android backend) is similar to Winits and we have a few situations where it's critical that the event is handled synchronously (e.g. notifying of surface invalidation and requesting that the application save state) and when requesting that the application saves state that also requires data to be provided synchronously via an out parameter.

It might not be a typical case (since it may be a large blob of data being saved vs a primitive return value) but maybe it can serve as one point of reference to consider here:

https://github.com/rust-mobile/android-activity/blob/6942637c3ccfdb2d02af97fdf174c1aa273c16a8/android-activity/src/lib.rs#L236
(Sorry the rustdoc hasn't been properly updated from the original C code that uses malloc)

Example usage:
https://github.com/rust-mobile/android-activity/blob/6942637c3ccfdb2d02af97fdf174c1aa273c16a8/examples/na-mainloop/src/lib.rs#L29

[madsmtm]

require that applications not create their first window until the event loop has started (which would have to be indicated by a new event such as Event::LoopStarted).

This causes a lot of issues on macOS as well, so I don't think it would be so far-fetched to require at some point.

[rib]

require that applications not create their first window until the event loop has started (which would have to be indicated by a new event such as Event::LoopStarted).

This causes a lot of issues on macOS as well, so I don't think it would be so far-fetched to require at some point.

One of the reasons behind making all backends emit a Resumed event was to address this too. (Considering that you cant e.g. create a rendering surface on Android before you have a window and it also doesn't make sense to do that while paused either. It wasn't possible to write portable code that could account for this that worked on mobile and desktop systems until we emitted a Resumed event consistently)

The Resumed event should probably be delayed if necessary for any platform that is currently emitting it before it's possible to create their first window.

[madsmtm]

This will be affected quite vastly by #2903, if we go through with that design

[daxpedda]

Web event callbacks can entirely be done externally, no Winit support required. See Window::prevent_default().

[kchibisov]

See #3852 , this shows how to add them in the current winit, once #3851 is added, it's kind of solved for backend writes, since they can expose all of that without forcing anything into other backends.

[kchibisov]

Also, since this PR is talking about a generic sink for events, it could also be exposed via such interface on platforms where it's all supported and it won't be blocked by common API design.