Switch to a futures-compatible API

[tomaka]

Implements #20

What does this do?

Instead of polling and waiting for events on the window directly, you first have to register your window on an events loop.

This events loop can then be polled (with poll_events()) or waited upon (with run_forever()), just like in the current API.

See the new window example for an example usage.

There are several advantages to this approach:

• We can use the events loop to signal system-wide events that are not specific to a given window (eg. a new monitor was connected). The current Event enum has been renamed to WindowEvent, and a new enum named Event contains all the possible "global" events.
• Creating multiple windows no longer requires creating one thread per window. This was especially problematic on Mac OS where it is forbidden to have windows in a non-primary thread. (EDIT: it may still be necessary to spawn an additional thread in winit's internals, but it's no longer necessary from the API point of view).
• The fact that we pass a callback to the events loop (as opposed to an iterator) means that we can finally correctly handle resize events on OS X. The resize callback has therefore been removed.
• The WindowProxy system has been removed.
• The fact that was pass a callback also means that we can add a lifetime parameter to the Event struct if necessary (eg. to pass strings as &str instead of String).

Implementation strategy

In order to make the transition smoother and avoid having to rewrite all the implementation, I created a macro named gen_api_transition! which creates types that expose the new API, and internally uses the old API.

For the moment each backend invokes this macro in order to make itself compatible with the new API.
In the future, each backend should be rewritten to directly implement the new API without going through this macro. Once everything has been transferred, the macro should be removed.

The macro doesn't attempt to register a resize callback, because it was too complex. However it should be easy to pass callback events to the events loop if we modify the OS X backend.

A necessary change was that the frontend now uses a type named Window2 instead of Window. This should also be reverted once possible.

Close #20

[tomaka]

Ping @glennw @jdm @ozkriff @mitchmindtree @kvark @retep998 @vberger (hopefully not forgetting someone)

[tomaka]

Note: do not forget to test the Android build before merging.
EDIT: ok that was easy, done already.

[elinorbgr]

Just a thought, would it be the proper place to add a note that on some platforms, no events will be generated before something is drawn on the window (this is the case of wayland)? This can totally be done in a followup PR btw, it's just that reading this here made me think of it.

[elinorbgr]

I like this new API. 👍

As it matches pretty well the wayland event flow, it'll allow me to remove a large part of the current runtime overhead of the wayland backend.

Just, to make sure I got it correctly. From the point of view of the backends, doing the proper migration means implementing an EventLoop struct with this API:

impl EventLoop {
    pub fn new() -> EventLoop { ... }
    pub fn poll_events<F>(&self, cb: F) where F: FnMut(::Event) { ... }
    pub fn run_forever<F>(&self, cb: F) where F: FnMut(::Event) { ... }
}

and update the window internals to dispatch the events through the event loop. Is it correct?

Secondary question: is it supposed to be valid for an user to have more than one EventLoop, and manage them independently (possibly from different threads)?

[tomaka]

Is it correct?

Yes, the backends are supposed to be exactly like the public API.

Secondary question: is it supposed to be valid for an user to have more than one EventLoop, and manage them independently (possibly from different threads)?

Yes, is it a problem?

[elinorbgr]

Not at all, but this has consequences on the implementation, so I wanted to be sure we are on the same page.

[wolfiestyle]

It seems weird to me to provide a run_forever instead of something like this:

pub fn wait_event<F>(&self, cb: F) where F: FnOnce(::Event) { ... }

..or just return the Event directly:

pub fn wait_event(&self) -> Event { ... }

Does it have to include the loop?
The run_forever version could be implemented on top of those as a convenience.

[tomaka]

@Darkstalker There's the problem of OS X which interrupts your main thread while your window is being resized, which means that the only way to do something is through the callback handler.

[tomaka]

Let's do that final comment period thing for this design.
I'm probably a bit overprecocious here, but I don't want to change an "important" library's design on a whim.

[jwilm]

@tomaka thanks for inviting the FCP. Is the expectation with this implementation that drawing should happen on a secondary thread?

[tomaka]

@jwilm No. Today you usually do something like this:

loop {
    for ev in window.poll_events() {
        match ev { ... }
    }

    draw();
}

With the new API it would be something like this:

loop {
    events_loop.poll_events(|ev| {
        match ev { ... }
    });

    draw();
}

[jwilm]

@tomaka oops, I missed the poll_events() mention at the top of the post. Thanks for the clarification there.

My other concern regards removal of the WindowProxy system. Is there another way to wakeup the run_forever loop? I currently depend on that functionality to multiplex window events with reading from a file descriptor on another thread.

[tomaka]

Is there another way to wakeup the run_forever loop?

Yes, events_loop.interrupt() can be called from a separate thread and will wake up one instance of run_forever (similar to a condvar for example).

[jwilm]

@tomaka perfect. This all sounds good!

[mitchmindtree]

Could we move this variant to the Event enum? Also perhaps the Awakened variant (although I noticed it looks like you're planning on removing this).

I'm wondering if also the ReceivedCharacter and KeyboardInput variants should be moved there also? Or perhaps they should stay in the WindowEvent enum and only be delivered to the currently focused window?

[tomaka]

Ah yes, the problem with Suspended and Awakened is that I need to change the backends' code if I remove them.

As for the keyboard events, the windows compositor usually sends the event only to the currently focused window, so sending it to a specific window is how it's designed.

[mitchmindtree]

the problem with Suspended and Awakened is that I need to change the backends' code if I remove them.

I guess it's probably OK to leave for a follow-up PR then. You could do it on another branch and wait for contributors from each backend to do PRs to that branch if you can't do each backend yourself.

As for the keyboard events, the windows compositor usually sends the event only to the currently focused window, so sending it to a specific window is how it's designed.

Now that you mention it, this also seems to be the way macOS windows work too.

[mitchmindtree]

Perhaps this should be it's own Id type defined in the window module? That way we could also document it, specifying that every window will have a unique identifier associated with it for the duration of its lifetime? Maybe this is fine though, I'm not sure.

[mitchmindtree]

This variant could probably just be Window as we already know that it is an Event via the enum name, e.g. Event::Window. Otherwise we end up repeating "Event" twice.

[tomaka]

A window is not an event though.

[mitchmindtree]

We already know that it is an Event though, it is the name of the type.

[mitchmindtree]

Not a big deal, just a suggestion.

[mitchmindtree]

It seems like the Window's poll_events and wait_events methods and their associated Iterators should also be removed as a part of this PR?

[tomaka]

There's no poll_events and wait_events anymore in the public API, so I'm not sure what you mean.
EDIT: Oh nevermind, looks like the changes didn't make it to github.

[mitchmindtree]

It looks like they are still exposed here and here, unless I am mistaken?

[mitchmindtree]

There's a window_proxy_send.rs module in the tests directory that could probably be removed.

[mitchmindtree]

@tomaka should it be possible for a user to call poll_events or run_forever inside another call to poll_events or run_forever? E.g.

events_loop.run_forever(|event| {
    events_loop.poll_events(|event| {
        // ..etc
    })
})

The API as it is allows it, however this has some implications for the macOS implementation, as the macOS implementation must be able to share the user-given callback to each of the window delegate's callbacks (for resize, etc). I think I can allow this by keeping the user-given callbacks in a stack (edit: actually I think I can just use the call stack), but just thought I'd check that allowing this is intentional first.

Edit: Also, if this kind of nesting is to be allowed, should the interrupt method only interrupt the inner-most loop? Note that currently this PR makes it so that all loops on the stack are interrupted and not just the top-most one. To change the behaviour so that only the inner-most loop is interrupted, you could call self.interrupted.store(false, ::std::sync::atomic::Ordering::Relaxed); before breaking in the gen_api_transition! run_forever method.

[mitchmindtree]

A minor point: the following might be more ergonomic and suitable method names:

• events_loop.poll_events(...) -> events_loop.poll(...)
• events_loop.run_forever(...) -> events_loop.wait(...)

I think these are more concise while still making it obvious that we are polling or waiting for events. I think using wait is more suitable than run_forever as:

• It is consistent with the old wait_events iterator and will make it more obvious to people switching over from the old wait_events API that this is method achieves the same behaviour.
• The loop doesn't really run "forever" if it is interrupted.

Sorry for the late bike-shedding! Only just getting time to properly look over all this over the past couple days.

[tomaka]

@tomaka should it be possible for a user to call poll_events or run_forever inside another call to poll_events or run_forever? E.g.

Maybe it's best to simply panic if the user does so.

I think using wait is more suitable than run_forever as:

The reason why I don't really like wait is that it makes you think that it will wait for an event or a set of events, and will automatically stop waiting afterwards.
Just run would be appropriate though I guess.

[elinorbgr]

About the nesting, my wayland lib has a similar pattern, but the methods require &mut self on the event-loop. If there a reason this is not the case here?

With the wayland backend, nesting run_forever and/or poll_events calls will result in a deadlock.

[tomaka]

About the nesting, my wayland lib has a similar pattern, but the methods require &mut self on the event-loop. If there a reason this is not the case here?

If it required &mut self you couldn't call interrupt anymore.

[elinorbgr]

Oh, right., forgot about that.

I've solved this issue in wayland-client using an EventLoopHandle struct, which is given as a reference argument to the closure by the event loop. This handle providing limited control to the event loop, allowing only operations that are safe to do from within a callback. Might be an option here?

[tomaka]

Well, the other reason is that ideally the events loop should be sharable amongst threads (even if that means that a mutex is used internally) unless there's a good reason not to.
I also like the simplicity of having just a EventsLoop and no weird thing like the WindowProxy was.

I suggest that the code just panics if run_forever or poll_events is called while inside a callback.
This check could even be performed in the public code so that the behavior is the same amongst backends.

[tomaka]

Ok, let's merge this.

The next steps are to write a PR that panics if we call a callback from within a callback, and to update the backends.