x11: Destroy dropped windows; handle WM_DELETE_WINDOW

[francesca64]

Fixes #79 #259 #414 #421

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage, offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was previously contained in Window's Drop implementation, along with destroying the Window. Not only does this mean that dropped windows are closed, but also that clicking the × button on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly permenanently) frozen after its invocation. That was caused specifically by the mutex locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave this way, this is nonetheless a significant improvement. The previous behavior led to inconsistent state, panics, and event loop breakage, along with not actually destroying the window.

This additionally makes the assumption that users don't need Focused or CursorLeft events for the destroyed window, as Closed is adequate to indicate unfocus, and users may not expect to receive events for closed/dropped windows. In my testing, those specific events were sent immediately after the window was destroyed, though this sort of behavior could be WM-specific. I've opted to explicitly suppress those events in the case of the window no longer existing.

[Ralith]

It's by design that clicking the X doesn't automatically close the window. It's supposed to issue a request, which the application can respond to as appropriate. For example, an application might want to prompt the user to save their work.

[francesca64]

@Ralith I agree with you 100% that winit should work that way. However, I'm not convinced winit currently codifies that idea.

For one thing, in the absence of #13, the current API doesn't even enable us to manually close windows (aside from dropping, but I don't see that documented as a solution, and I imagine we want the canonical solution to be more explicit). The multiwindow example seems to suggest that Closed indicates a window's already been closed, and the documentation isn't explicit about the meaning.

Now, I'm not too familiar with the other backends, but I tried to investigate this. On Windows, it looks like the window's state is removed at the same time as sending Closed and it's stated here that the DefWindowProcW call from above results in the window ultimately being destroyed. This is more clear on OS X, where find_and_remove_window is used to remove the window at the same time as sending Closed. Of course, I haven't tested the behavior of these, so I could fortunately be wrong in how I'm interpreting this.

Either way, I think there's a definite need for winit to take a more explicit stance on what the intended behavior is. I designed this PR the way I did to make it more consistent with existing expectations and the other backends, with the mindset that larger API concerns could be ironed out in future work. However, if you're of the mindset that this would be introducing a de facto regression, well, I can't dispute that. I can get to work on making a new PR that addresses these concerns.

[Ralith]

I can't speak to the state of the other backends, and I'm not in a position to unilaterally reject this approach. Consistency is good, and incremental improvements are worth making.

I would, however, like to note for posterity that "drop = close" is natural from an RAII perspective, and in the overwhelmingly common case of one-window applications that terminate then a close is requested, it Just Works.

[potocpav]

This fixes a panic that I incounter on multiple windows created sequentially:

extern crate glium;

fn spawn() {
    let mut events_loop = glium::glutin::EventsLoop::new();
    let window = glium::glutin::WindowBuilder::new();
    let context = glium::glutin::ContextBuilder::new();
    let display = glium::Display::new(window, context, &events_loop).unwrap();

    events_loop.run_forever(|event| {
        if let glium::glutin::Event::WindowEvent { event: glium::glutin::WindowEvent::Closed, .. } = event {
            return glium::glutin::ControlFlow::Break;
        }
        glium::glutin::ControlFlow::Continue
    });
}

fn main() {
    spawn();
    spawn();
}

After closing the first win, this produces on my X11 setup:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:335:21
(...)
   9: <core::option::Option<T>>::unwrap
             at /checkout/src/libcore/macros.rs:20
  10: winit::platform::platform::x11::EventsLoop::process_event
             at /home/pavel/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/winit-0.11.2/src/platform/linux/x11/mod.rs:719
  11: winit::platform::platform::x11::EventsLoop::run_forever
             at /home/pavel/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/winit-0.11.2/src/platform/linux/x11/mod.rs:188
  12: winit::platform::platform::EventsLoop::run_forever
             at /home/pavel/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/winit-0.11.2/src/platform/linux/mod.rs:399
  13: winit::EventsLoop::run_forever
             at /home/pavel/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/winit-0.11.2/src/lib.rs:231
  14: multi_win::spawn
             at src/main.rs:10
  15: multi_win::main
             at src/main.rs:20

[francesca64]

I've verified that this is consistent with the behavior on Windows. I haven't had the opportunity to test on OS X, but the code there is easier to interpret, so I'd be surprised if it's not the case there as well.

Considering the difficulty of making multi-platform design changes, combined with the significance of the existing problems in the X11 backend, I'm still in favor of this being merged. I'll also create an issue concerning winit's concept of Closed.

[gui1117]

I have an issue using it in vulkano:
I add

[patch.crates-io]
winit = { git = "https://github.com/francesca64/winit", branch = "x11-drop-window" }

to the root Cargo.toml and then if I run triangle example and close the window then it does close the window but doesn't end the processus.

[tomaka]

I have an issue using it in vulkano:

I'm not sure vulkano is a good example of this, as it stores the window in an Arc which may not be dropped when you want.