From a70ac1531e684cc2db16415cadde91a9c4ad98cb Mon Sep 17 00:00:00 2001 From: Murarth Date: Fri, 22 Nov 2019 17:11:04 -0700 Subject: [PATCH] X11: Fix window creation hangs when another application is fullscreen (#1248) * X11: Fix window creation hangs when another application is fullscreen Previously, the X11 backend would block until a `VisibilityNotify` event is received when creating a Window that is visible or when calling `set_visible(true)` on a Window that is not currently visible. This could cause winit to hang in situations where the WM does not quickly send this event to the application, such as another window being fullscreen at the time. This behavior existed to prevent an X protocol error caused by setting fullscreen state on an invisible window. This fix instead stores desired fullscreen state when `set_fullscreen` is called (iff the window is not visible or not yet visible) and issues X commands to set fullscreen state when a `VisibilityNotify` event is received through the normal processing of events in the event loop. * Add window_debug example to facilitate testing * Add a CHANGELOG entry * Call `XUnmapWindow` if `VisibilityNotify` is received on an invisible window --- CHANGELOG.md | 1 + examples/timer.rs | 5 +- examples/window_debug.rs | 121 +++++++++++++++++ .../linux/x11/event_processor.rs | 7 + src/platform_impl/linux/x11/window.rs | 124 ++++++++++-------- 5 files changed, 202 insertions(+), 56 deletions(-) create mode 100644 examples/window_debug.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 68850199..e76e35a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - On X11, fix misreporting DPI factor at startup. - On X11, fix events not being reported when using `run_return`. - On X11, fix key modifiers being incorrectly reported. +- On X11, fix window creation hanging when another window is fullscreen. # 0.20.0 Alpha 4 (2019-10-18) diff --git a/examples/timer.rs b/examples/timer.rs index 75bf2831..e1e3f290 100644 --- a/examples/timer.rs +++ b/examples/timer.rs @@ -17,10 +17,7 @@ fn main() { let timer_length = Duration::new(1, 0); event_loop.run(move |event, _, control_flow| { - if let Event::WindowEvent { event, .. } = &event { - // Print only Window events to reduce noise - println!("{:?}", event); - } + println!("{:?}", event); match event { Event::NewEvents(StartCause::Init) => { diff --git a/examples/window_debug.rs b/examples/window_debug.rs new file mode 100644 index 00000000..0ecd37b3 --- /dev/null +++ b/examples/window_debug.rs @@ -0,0 +1,121 @@ +// This example is used by developers to test various window functions. + +use winit::{ + dpi::{LogicalSize, PhysicalSize}, + event::{DeviceEvent, ElementState, Event, KeyboardInput, VirtualKeyCode, WindowEvent}, + event_loop::{ControlFlow, EventLoop}, + window::{Fullscreen, WindowBuilder}, +}; + +fn main() { + let event_loop = EventLoop::new(); + + let window = WindowBuilder::new() + .with_title("A fantastic window!") + .with_inner_size(LogicalSize::from((100, 100))) + .build(&event_loop) + .unwrap(); + + eprintln!("debugging keys:"); + eprintln!(" (E) Enter exclusive fullscreen"); + eprintln!(" (F) Toggle borderless fullscreen"); + #[cfg(waiting_for_set_minimized)] + eprintln!(" (M) Toggle minimized"); + eprintln!(" (Q) Quit event loop"); + eprintln!(" (V) Toggle visibility"); + eprintln!(" (X) Toggle maximized"); + + #[cfg(waiting_for_set_minimized)] + let mut minimized = false; + let mut maximized = false; + let mut visible = true; + + event_loop.run(move |event, _, control_flow| { + *control_flow = ControlFlow::Wait; + + match event { + Event::DeviceEvent { + event: + DeviceEvent::Key(KeyboardInput { + virtual_keycode: Some(key), + state: ElementState::Pressed, + .. + }), + .. + } => match key { + #[cfg(waiting_for_set_minimized)] + VirtualKeyCode::M => { + if minimized { + minimized = !minimized; + window.set_minimized(minimized); + } + } + VirtualKeyCode::V => { + if !visible { + visible = !visible; + window.set_visible(visible); + } + } + _ => (), + }, + Event::WindowEvent { + event: WindowEvent::KeyboardInput { input, .. }, + .. + } => match input { + KeyboardInput { + virtual_keycode: Some(key), + state: ElementState::Pressed, + .. + } => match key { + VirtualKeyCode::E => { + fn area(size: PhysicalSize) -> f64 { + size.width * size.height + } + + let monitor = window.current_monitor(); + if let Some(mode) = monitor.video_modes().max_by(|a, b| { + area(a.size()) + .partial_cmp(&area(b.size())) + .expect("NaN in video mode size") + }) { + window.set_fullscreen(Some(Fullscreen::Exclusive(mode))); + } else { + eprintln!("no video modes available"); + } + } + VirtualKeyCode::F => { + if window.fullscreen().is_some() { + window.set_fullscreen(None); + } else { + let monitor = window.current_monitor(); + window.set_fullscreen(Some(Fullscreen::Borderless(monitor))); + } + } + #[cfg(waiting_for_set_minimized)] + VirtualKeyCode::M => { + minimized = !minimized; + window.set_minimized(minimized); + } + VirtualKeyCode::Q => { + *control_flow = ControlFlow::Exit; + } + VirtualKeyCode::V => { + visible = !visible; + window.set_visible(visible); + } + VirtualKeyCode::X => { + maximized = !maximized; + window.set_maximized(maximized); + } + _ => (), + }, + _ => (), + }, + Event::WindowEvent { + event: WindowEvent::CloseRequested, + window_id, + } if window_id == window.id() => *control_flow = ControlFlow::Exit, + _ => (), + } + }); +} diff --git a/src/platform_impl/linux/x11/event_processor.rs b/src/platform_impl/linux/x11/event_processor.rs index cf613744..9c74e74b 100644 --- a/src/platform_impl/linux/x11/event_processor.rs +++ b/src/platform_impl/linux/x11/event_processor.rs @@ -497,6 +497,13 @@ impl EventProcessor { }); } + ffi::VisibilityNotify => { + let xev: &ffi::XVisibilityEvent = xev.as_ref(); + let xwindow = xev.window; + + self.with_window(xwindow, |window| window.visibility_notify()); + } + ffi::Expose => { let xev: &ffi::XExposeEvent = xev.as_ref(); diff --git a/src/platform_impl/linux/x11/window.rs b/src/platform_impl/linux/x11/window.rs index 8df107bc..f967a100 100644 --- a/src/platform_impl/linux/x11/window.rs +++ b/src/platform_impl/linux/x11/window.rs @@ -28,16 +28,6 @@ use crate::{ use super::{ffi, util, EventLoopWindowTarget, ImeSender, WindowId, XConnection, XError}; -unsafe extern "C" fn visibility_predicate( - _display: *mut ffi::Display, - event: *mut ffi::XEvent, - arg: ffi::XPointer, // We populate this with the window ID (by value) when we call XIfEvent -) -> ffi::Bool { - let event: &ffi::XAnyEvent = (*event).as_ref(); - let window = arg as ffi::Window; - (event.window == window && event.type_ == ffi::VisibilityNotify) as _ -} - #[derive(Debug)] pub struct SharedState { pub cursor_pos: Option<(f64, f64)>, @@ -48,6 +38,8 @@ pub struct SharedState { pub last_monitor: X11MonitorHandle, pub dpi_adjusted: Option<(f64, f64)>, pub fullscreen: Option, + // Set when application calls `set_fullscreen` when window is not visible + pub desired_fullscreen: Option>, // Used to restore position after exiting fullscreen pub restore_position: Option<(i32, i32)>, // Used to restore video mode after exiting fullscreen @@ -55,14 +47,28 @@ pub struct SharedState { pub frame_extents: Option, pub min_inner_size: Option, pub max_inner_size: Option, - pub is_visible: bool, + pub visibility: Visibility, +} + +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum Visibility { + No, + Yes, + // Waiting for VisibilityNotify + YesWait, } impl SharedState { fn new(last_monitor: X11MonitorHandle, is_visible: bool) -> Mutex { + let visibility = if is_visible { + Visibility::YesWait + } else { + Visibility::No + }; + Mutex::new(SharedState { last_monitor, - is_visible, + visibility, cursor_pos: None, size: None, @@ -71,6 +77,7 @@ impl SharedState { inner_position_rel_parent: None, dpi_adjusted: None, fullscreen: None, + desired_fullscreen: None, restore_position: None, desktop_video_mode: None, frame_extents: None, @@ -360,8 +367,6 @@ impl UnownedWindow { unsafe { (xconn.xlib.XMapRaised)(xconn.display, window.xwindow); } //.queue(); - - window.wait_for_visibility_notify(); } // Attempt to make keyboard input repeat detectable @@ -419,8 +424,7 @@ impl UnownedWindow { if window_attrs.fullscreen.is_some() { window .set_fullscreen_inner(window_attrs.fullscreen.clone()) - .unwrap() - .queue(); + .map(|flusher| flusher.queue()); } if window_attrs.always_on_top { window @@ -577,9 +581,13 @@ impl UnownedWindow { fn set_fullscreen_inner(&self, fullscreen: Option) -> Option> { let mut shared_state_lock = self.shared_state.lock(); - if !shared_state_lock.is_visible { + match shared_state_lock.visibility { // Setting fullscreen on a window that is not visible will generate an error. - return None; + Visibility::No | Visibility::YesWait => { + shared_state_lock.desired_fullscreen = Some(fullscreen); + return None; + } + Visibility::Yes => (), } let old_fullscreen = shared_state_lock.fullscreen.clone(); @@ -690,7 +698,12 @@ impl UnownedWindow { #[inline] pub fn fullscreen(&self) -> Option { - self.shared_state.lock().fullscreen.clone() + let shared_state = self.shared_state.lock(); + + shared_state + .desired_fullscreen + .clone() + .unwrap_or_else(|| shared_state.fullscreen.clone()) } #[inline] @@ -703,6 +716,26 @@ impl UnownedWindow { } } + // Called by EventProcessor when a VisibilityNotify event is received + pub(crate) fn visibility_notify(&self) { + let mut shared_state = self.shared_state.lock(); + + match shared_state.visibility { + Visibility::No => unsafe { + (self.xconn.xlib.XUnmapWindow)(self.xconn.display, self.xwindow); + }, + Visibility::Yes => (), + Visibility::YesWait => { + shared_state.visibility = Visibility::Yes; + + if let Some(fullscreen) = shared_state.desired_fullscreen.take() { + drop(shared_state); + self.set_fullscreen(fullscreen); + } + } + } + } + #[inline] pub fn current_monitor(&self) -> X11MonitorHandle { self.shared_state.lock().last_monitor.clone() @@ -838,44 +871,31 @@ impl UnownedWindow { #[inline] pub fn set_visible(&self, visible: bool) { - let is_visible = self.shared_state.lock().is_visible; + let mut shared_state = self.shared_state.lock(); - if visible == is_visible { - return; + match (visible, shared_state.visibility) { + (true, Visibility::Yes) | (true, Visibility::YesWait) | (false, Visibility::No) => { + return + } + _ => (), } - match visible { - true => unsafe { + if visible { + unsafe { (self.xconn.xlib.XMapRaised)(self.xconn.display, self.xwindow); - self.xconn - .flush_requests() - .expect("Failed to call XMapRaised"); - - // Some X requests may generate an error if the window is not - // visible, so we must wait until the window becomes visible. - self.wait_for_visibility_notify(); - }, - false => unsafe { + } + self.xconn + .flush_requests() + .expect("Failed to call XMapRaised"); + shared_state.visibility = Visibility::YesWait; + } else { + unsafe { (self.xconn.xlib.XUnmapWindow)(self.xconn.display, self.xwindow); - self.xconn - .flush_requests() - .expect("Failed to call XUnmapWindow"); - }, - } - - self.shared_state.lock().is_visible = visible; - } - - fn wait_for_visibility_notify(&self) { - unsafe { - let mut event = MaybeUninit::uninit(); - - (self.xconn.xlib.XIfEvent)( - self.xconn.display, - event.as_mut_ptr(), - Some(visibility_predicate), - self.xwindow as _, - ); + } + self.xconn + .flush_requests() + .expect("Failed to call XUnmapWindow"); + shared_state.visibility = Visibility::No; } }