RFC: Switch to a futures-compatible API about winit HOT 20 CLOSED

@tomaka that API looks fine to me!

Would it be worth making a new window-specific event enum to differentiate between general events and events that apply to a specific window? It might save adding a WindowId field to half of the events. E.g. the existing events might be split up something like this:

pub enum Event {
    MonitorConnected,
    MonitorDisconnected,
    KeyboardInput,
    Awakened,
    Suspended,
    Window { id: WindowId, event: WindowEvent },
}

pub enum WindowEvent {
    Focused,
    Resized,
    Moved,
    Closed,
    DroppedFile,
    MouseMoved,
    MouseEntered,
    MouseLeft,
    Touch,
    TouchpadPressure,
}

So matching on events might look something like this

match event {
    winit::Event::MonitorConnected { ... } => (),
    winit::Event::KeyboardInput { ... } => (),
    winit::Event::Window { id, event } => match event {
        winit::WindowEvent::Resized { ... } if id == window1 => println!("win 1 resized"),
        winit::WindowEvent::Closed if id == window2 => println!("win 2 closed"),
        _ => ()
    },
    _ => (),
}

from winit.

There are two main trade-offs that come to mind with this approach:

• Control Flow - a user may want to stop checking for events after a certain event type is received, or return early using the try! macro following some error. It can be very tedious to handle this when the user is restricted to the scope of a closure. How do you anticipate handling control flow with this design? I'm not sure I'm familiar enough with futures-rs to know how they deal with it.
• Ownership issues - I'm guessing that most of these callbacks will be constrained by FnMut? I can imagine this making it difficult to "share" data between each of the callbacks. It seems like we'd either have to require users wrap their "shared" data in Rc<RefCell<>> or Arc<Mutex<>>, or that they propagate the events yielded by each callback over some channel to a loop that looks like what glutin currently looks like. I put "shared" in quotations as the closures are only actually ever called from the same thread, right?

In the implementation we can decouple the Window object in Rust from the actual window. The actual window could be stored in the EventsLoop for example.

I'm curious why this is a benefit? Is it because you're unsure how to both make the window handle available to the user while also using it to run the events loop? Another way (using glutin's current style) might be to pass the window to the method that yields the next event, something like:

loop {
    let poll = events_loop.poll();
    while let Some(event) = poll.next(&mut window) {
        // Still have access to the window handle
    }
}

I'm not familiar enough with the issues related to the other benefits you mentioned to be able to comment on whether they are worth the trade-offs I mentioned above.

from winit.

I guess the main question is whether it's prudent to plan on releasing a 1.0 of winit based on futures-rs within the next nine months or so

I think that's reasonable, yes, assuming that today's futures-rs seems to cover your needs. We definitely won't be removing any functionality, and at this point the core future and stream abstractions have gotten quite a bit of vetting. While I do expect a 0.2 release in the coming months, we're already using features like deprecation warnings to make that a smooth transition, and I don't expect core abstractions to be changes much, if at all -- with the exception of the just-added Sink trait, which will likely need a bit more design iteration.

from winit.

I'm still a bit hesitant about depending on future, though. I don't want to jump into semver hell by exposing another library's internals. When some person's code will inevitably break after they run cargo update, I will get pinged on IRC to tackle the problem.

I'm tempted to either wait for the private/shared dependencies system that was advertised in a talk recently, or for futures to be merged in the stdlib.

from winit.

I guess I'll ping some people to get some opinions: @ozkriff @mitchmindtree @kvark @fkaa @vberger @glennw @larsbergstrom

from winit.

How would this allow winit to not spawn a background thread on Windows? Rescaling a window causes the callback thread to be blocked until the end of the rescale, a behavior which is currently hidden by the background thread, and as far as I know there isn't any way to stop that behavior. Moving it onto the main thread would move the blocking there.

from winit.

@Osspial Are you sure about that? On Win32 you're supposed to poll and dispatch the messages manually with PeekMessage and DispatchMessage, so I'm not sure how it would block.

@mitchmindtree Don't forget that the closure can borrow its environment.
You could write for example this:

window.set_callback(|ev| {
    match ev {
        Event::Closed => events_loop.stop(),
        _ => ()
    }
});

events_loop.run_until_stopped();

Or, this:

let mut events = RefCell::new(Vec::new());
window.set_callback(|ev| events.borrow_mut().push(ev));

loop {
    events_loop.run_once();
    for event in events.borrow_mut().drain(..) {
        ...
    }
}

It's true that if you want to isolate the window in a struct, then you will need a Rc<RefCell<>> or an Arc<Mutex<>> like this:

pub struct MyApp {
    events: Arc<Mutex<Vec<Events>>>,
    window: Window,
}

impl MyApp {
    pub fn new(events_loop: &EventsLoop) -> MyApp {
        let events = Arc::new(Mutex::new(Vec::new()));
        let events2 = events.clone();
        let window = Window::new(events_loop);
        window.set_callback(move |ev| events2.lock().unwrap().push(ev));
        MyApp { events: events, window: window }
    }
}

However I think this use case doesn't happen often.

I'm curious why this is a benefit?

The benefit of decoupling the window from the events loop is that on MacOS we can run the events loop on the main thread and the window in another thread, and that with X11 we can do everything in a single-threaded fashion by creating one X connection per EventLoop, which will likely lead to less bugs in xlib.

from winit.

@tomaka Yeah. I was able to get winit working without multithreading on Windows with the current API, and everything worked fine except for rescales, which blocked execution. Apparently, Windows enters an internal loop during rescales that does end up calling the user-defined window procedure, but doesn't return from DispatchMessage, effectively blocking program execution until the resize ends, exiting the loop.

from winit.

One interesting problem is about leak safety.
In the example above, if we mem::forget the Window and run the events loops, we can access the objects referenced by the callback without having any actual borrow on them. This is undefined behavior.

Future-rs's tasks system is very complex and avoids this problem by running the callback only if we call a method on either the future itself or the task that holds the future.

from winit.

Looking at this issue again and looking over the benefits you mention, I think they're easily worth the issues I mentioned, and the workarounds you shared look really reasonable anyways. 👍 from me.

from winit.

@tomaka would it make sense to actually make this use futures-rs at this point. According to @aturon Futures should be quite stable at this point. (Also note @alexcrichton's followup on tokio-core stability.

from winit.

@tschneidereit I'd be happy to advise more specifically if you decide to go this direction!

from winit.

@aturon thanks! I guess the main question is whether it's prudent to plan on releasing a 1.0 of winit based on futures-rs within the next nine months or so:

This is essentially the same approach as the future-rs library, except that we're not using future-rs directly because it's not totally stable. I'd like to eventually publish winit 1.0 (in, like, the next nine months or so), and it's unlikely that future-rs publishes its 1.0 before winit.

@tomaka, do I understand correctly that with the right stability guarantees you'd go with futures-rs?

from winit.

@pcwalton, do you think a Futures-/Tokio-based event loop for winit would work well for Servo?

from winit.

At first reaction, this seems like it would be fine for Servo.

The main parts for us are:

• The EventLoopProxy support, which is vital, but sounds like it's just a simple API change.
• I don't work on mac often, but I recall running in to lots of issues with the synchronous resize callback (I don't recall exactly what they were, unfortunately). So long as we retain the functionality to be able to do GL rendering while a resize is ongoing, that should be fine.

from winit.

This is on my to-do list, probably to look at starting Q1 2017.

from winit.

I've been thinking about it, and for me the future library has too many drawbacks:

• Complicated to use.
• You totally lose sense of what's happening. For example good luck knowing where you will land if you happen to panic in a future handler. Since OpenGL operations and some windowing operations are local to a thread, it's problematic. For example trying to call an OpenGL function from a callback could be very dangerous. I think futures/tasks and regular threads are not really compatible.
• The fact that futures always return a Result even though errors can't happen. The poll_events and wait_events functions don't return a Result.
• The tasks system has the consequence that you need to spawn one thread for each event loop that you have in your program. Who wants to have their main thread dedicated entirely to handling windowing messages? With my proposal in the opening post you could use a loop to run each event loop once.

If you except the leakpocalypse-related problem described above, I don't see any good reason to use futures instead of a custom API. On the other hand, it's very easy to wrap the API described in the opening post around futures if one wants to use futures.

from winit.

Maybe an API like this is better:

let events_loop = EventsLoop::new();
let window1 = WindowBuilder::new().build(&events_loop).unwrap();
let window2 = WindowBuilder::new().build(&events_loop).unwrap();

loop {
    events_loop.run_once(|event| {
        match event {
            Event::Resized { window_id, ... } => {
                if window_id == window1.id() { println!("win 1 resized"); }
                else { println!("win 2 resized") }
            },
            Event::MonitorConnected { ... } => {},
            _ => {}
        }
    });
}

The events loop calls a local callback and each event contains an identifier of the window that produced the event (amongst the windows that were registered on the events loop).

I think it would still provide all the advantages described above. We still need to use a callback and not return events by value because of the MacOS resize callback problem, and for emscripten.

Someone would really wants to use futures could easily implement Future/Stream over this API by taking control of the events loop's callback.

from winit.

Perhaps this is of interest: https://github.com/gamazeps/RobotS

from winit.

rust-lang/futures-rs#50

from winit.