diff --git a/CHANGELOG.md b/CHANGELOG.md index 57237ac8..d0370646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +- Fixed thread-safety issues in several `Window` functions on Windows. - On MacOS, the key state for modifiers key events is now properly set. - On iOS, the view is now set correctly. This makes it possible to render things (instead of being stuck on a black screen), and touch events work again. - Added NetBSD support. diff --git a/src/platform/windows/events_loop.rs b/src/platform/windows/events_loop.rs index ca9a8f05..bed52abc 100644 --- a/src/platform/windows/events_loop.rs +++ b/src/platform/windows/events_loop.rs @@ -58,6 +58,7 @@ use platform::platform::dpi::{ get_hwnd_scale_factor, }; use platform::platform::event::{handle_extended_keys, process_key_params, vkey_to_winit_vkey}; +use platform::platform::icon::WinIcon; use platform::platform::raw_input::{get_raw_input_data, get_raw_mouse_button_state}; use platform::platform::window::adjust_size; @@ -94,6 +95,13 @@ pub struct WindowState { // This is different from the value in `SavedWindowInfo`! That one represents the DPI saved upon entering // fullscreen. This will always be the most recent DPI for the window. pub dpi_factor: f64, + pub fullscreen: Option<::MonitorId>, + pub window_icon: Option, + pub taskbar_icon: Option, + pub decorations: bool, + pub always_on_top: bool, + pub maximized: bool, + pub resizable: bool, } impl WindowState { @@ -326,6 +334,11 @@ impl EventsLoopProxy { /// /// The `Inserted` can be used to inject a `WindowState` for the callback to use. The state is /// removed automatically if the callback receives a `WM_CLOSE` message for the window. + /// + /// Note that if you are using this to change some property of a window and updating + /// `WindowState` then you should call this within the lock of `WindowState`. Otherwise the + /// events may be sent to the other thread in different order to the one in which you set + /// `WindowState`, leaving them out of sync. pub fn execute_in_thread(&self, function: F) where F: FnMut(Inserter) + Send + 'static, diff --git a/src/platform/windows/icon.rs b/src/platform/windows/icon.rs index b21131b5..6ea1e990 100644 --- a/src/platform/windows/icon.rs +++ b/src/platform/windows/icon.rs @@ -22,11 +22,13 @@ pub enum IconType { Big = winuser::ICON_BIG as isize, } -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct WinIcon { pub handle: HICON, } +unsafe impl Send for WinIcon {} + impl WinIcon { #[allow(dead_code)] pub fn from_path>(path: P) -> Result { diff --git a/src/platform/windows/window.rs b/src/platform/windows/window.rs index 0d7227d9..d961fc38 100644 --- a/src/platform/windows/window.rs +++ b/src/platform/windows/window.rs @@ -1,8 +1,8 @@ #![cfg(target_os = "windows")] -use std::cell::{Cell, RefCell}; -use std::ffi::OsStr; use std::{io, mem, ptr}; +use std::cell::Cell; +use std::ffi::OsStr; use std::os::windows::ffi::OsStrExt; use std::sync::{Arc, Mutex}; use std::sync::mpsc::channel; @@ -11,7 +11,7 @@ use winapi::ctypes::c_int; use winapi::shared::minwindef::{BOOL, DWORD, FALSE, LPARAM, TRUE, UINT, WORD, WPARAM}; use winapi::shared::windef::{HDC, HWND, LPPOINT, POINT, RECT}; use winapi::um::{combaseapi, dwmapi, libloaderapi, winuser}; -use winapi::um::objbase::{COINIT_MULTITHREADED}; +use winapi::um::objbase::COINIT_MULTITHREADED; use winapi::um::shobjidl_core::{CLSID_TaskbarList, ITaskbarList2}; use winapi::um::winnt::{LONG, LPCWSTR}; @@ -27,7 +27,8 @@ use { }; use platform::platform::{Cursor, PlatformSpecificWindowBuilderAttributes, WindowId}; use platform::platform::dpi::{dpi_to_scale_factor, get_window_dpi, get_window_scale_factor}; -use platform::platform::events_loop::{self, DESTROY_MSG_ID, EventsLoop, INITIAL_DPI_MSG_ID}; +use platform::platform::events_loop::{self, EventsLoop, DESTROY_MSG_ID, INITIAL_DPI_MSG_ID}; +use platform::platform::events_loop::WindowState; use platform::platform::icon::{self, IconType, WinIcon}; use platform::platform::monitor::get_available_monitors; use platform::platform::raw_input::register_all_mice_and_keyboards_for_raw_input; @@ -40,25 +41,13 @@ pub struct Window { /// Main handle for the window. window: WindowWrapper, - decorations: Cell, - maximized: Cell, - resizable: Cell, - fullscreen: RefCell>, - always_on_top: Cell, - /// The current window state. - window_state: Arc>, - - window_icon: Cell>, - taskbar_icon: Cell>, + window_state: Arc>, // The events loop proxy. events_loop_proxy: events_loop::EventsLoopProxy, } -unsafe impl Send for Window {} -unsafe impl Sync for Window {} - // https://blogs.msdn.microsoft.com/oldnewthing/20131017-00/?p=2903 // The idea here is that we use the Adjust­Window­Rect­Ex function to calculate how much additional // non-client area gets added due to the styles we passed. To make the math simple, @@ -283,32 +272,25 @@ impl Window { #[inline] pub fn set_resizable(&self, resizable: bool) { - if resizable == self.resizable.get() { - return; - } - if self.fullscreen.borrow().is_some() { + let mut window_state = self.window_state.lock().unwrap(); + if mem::replace(&mut window_state.resizable, resizable) != resizable { // If we're in fullscreen, update stored configuration but don't apply anything. - self.resizable.replace(resizable); - return; - } + if window_state.fullscreen.is_none() { + let mut style = unsafe { + winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE) + }; - let mut style = unsafe { - winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE) - }; - if resizable { - style |= WS_RESIZABLE as LONG; - } else { - style &= !WS_RESIZABLE as LONG; - } + if resizable { + style |= WS_RESIZABLE as LONG; + } else { + style &= !WS_RESIZABLE as LONG; + } - unsafe { - winuser::SetWindowLongW( - self.window.0, - winuser::GWL_STYLE, - style as _, - ); - }; - self.resizable.replace(resizable); + unsafe { + winuser::SetWindowLongW(self.window.0, winuser::GWL_STYLE, style as _); + }; + } + } } /// Returns the `hwnd` of this window. @@ -391,15 +373,12 @@ impl Window { #[inline] pub fn grab_cursor(&self, grab: bool) -> Result<(), String> { let currently_grabbed = unsafe { self.cursor_is_grabbed() }?; - let window_state = Arc::clone(&self.window_state); - { - let window_state_lock = window_state.lock().unwrap(); - if currently_grabbed == grab - && grab == window_state_lock.cursor_grabbed { - return Ok(()); - } + let window_state_lock = self.window_state.lock().unwrap(); + if currently_grabbed == grab && grab == window_state_lock.cursor_grabbed { + return Ok(()); } let window = self.window.clone(); + let window_state = Arc::clone(&self.window_state); let (tx, rx) = channel(); self.events_loop_proxy.execute_in_thread(move |_| { let result = unsafe { Self::grab_cursor_inner(&window, grab) }; @@ -408,6 +387,7 @@ impl Window { } let _ = tx.send(result); }); + drop(window_state_lock); rx.recv().unwrap() } @@ -421,18 +401,17 @@ impl Window { #[inline] pub fn hide_cursor(&self, hide: bool) { - let window_state = Arc::clone(&self.window_state); - { - let window_state_lock = window_state.lock().unwrap(); - // We don't want to increment/decrement the display count more than once! - if hide == window_state_lock.cursor_hidden { return; } - } + let window_state_lock = self.window_state.lock().unwrap(); + // We don't want to increment/decrement the display count more than once! + if hide == window_state_lock.cursor_hidden { return; } let (tx, rx) = channel(); + let window_state = Arc::clone(&self.window_state); self.events_loop_proxy.execute_in_thread(move |_| { unsafe { Self::hide_cursor_inner(hide) }; window_state.lock().unwrap().cursor_hidden = hide; let _ = tx.send(()); }); + drop(window_state_lock); rx.recv().unwrap() } @@ -468,9 +447,12 @@ impl Window { #[inline] pub fn set_maximized(&self, maximized: bool) { - self.maximized.replace(maximized); + let mut window_state = self.window_state.lock().unwrap(); + window_state.maximized = true; // We only maximize if we're not in fullscreen. - if self.fullscreen.borrow().is_some() { return; } + if window_state.fullscreen.is_none() { + return; + } let window = self.window.clone(); unsafe { @@ -488,10 +470,8 @@ impl Window { } } - unsafe fn set_fullscreen_style(&self) -> (LONG, LONG) { - let mut window_state = self.window_state.lock().unwrap(); - - if self.fullscreen.borrow().is_none() || window_state.saved_window_info.is_none() { + unsafe fn set_fullscreen_style(&self, window_state: &mut WindowState) -> (LONG, LONG) { + if window_state.fullscreen.is_none() || window_state.saved_window_info.is_none() { let rect = util::get_window_rect(self.window.0).expect("`GetWindowRect` failed"); let dpi_factor = Some(self.get_hidpi_factor()); window_state.saved_window_info = Some(events_loop::SavedWindowInfo { @@ -507,16 +487,14 @@ impl Window { let mut placement: winuser::WINDOWPLACEMENT = mem::zeroed(); placement.length = mem::size_of::() as u32; winuser::GetWindowPlacement(self.window.0, &mut placement); - self.maximized.replace(placement.showCmd == (winuser::SW_SHOWMAXIMIZED as u32)); + window_state.maximized = placement.showCmd == (winuser::SW_SHOWMAXIMIZED as u32); let saved_window_info = window_state.saved_window_info.as_ref().unwrap(); (saved_window_info.style, saved_window_info.ex_style) } - unsafe fn restore_saved_window(&self) { + unsafe fn restore_saved_window(&self, window_state_lock: &mut WindowState) { let (rect, mut style, ex_style) = { - let mut window_state_lock = self.window_state.lock().unwrap(); - // 'saved_window_info' can be None if the window has never been // in fullscreen mode before this method gets called. if window_state_lock.saved_window_info.is_none() { @@ -537,8 +515,8 @@ impl Window { let window = self.window.clone(); let window_state = Arc::clone(&self.window_state); - let maximized = self.maximized.get(); - let resizable = self.resizable.get(); + let resizable = window_state_lock.resizable; + let maximized = window_state_lock.maximized; // We're restoring the window to its size and position from before being fullscreened. // `ShowWindow` resizes the window, so it must be called from the main thread. @@ -585,6 +563,7 @@ impl Window { #[inline] pub fn set_fullscreen(&self, monitor: Option) { + let mut window_state_lock = self.window_state.lock().unwrap(); unsafe { match &monitor { &Some(RootMonitorId { ref inner }) => { @@ -593,8 +572,7 @@ impl Window { let window = self.window.clone(); let window_state = Arc::clone(&self.window_state); - let (style, ex_style) = self.set_fullscreen_style(); - + let (style, ex_style) = self.set_fullscreen_style(&mut window_state_lock); self.events_loop_proxy.execute_in_thread(move |_| { let _ = Self::grab_cursor_inner(&window, false); @@ -634,27 +612,23 @@ impl Window { }); } &None => { - self.restore_saved_window(); + self.restore_saved_window(&mut window_state_lock); } } } - self.fullscreen.replace(monitor); + window_state_lock.fullscreen = monitor; } #[inline] pub fn set_decorations(&self, decorations: bool) { - if self.decorations.get() == decorations { - return; - } - + let mut window_state = self.window_state.lock().unwrap(); + if mem::replace(&mut window_state.decorations, decorations) != decorations { let style_flags = (winuser::WS_CAPTION | winuser::WS_THICKFRAME) as LONG; let ex_style_flags = (winuser::WS_EX_WINDOWEDGE) as LONG; - // if we are in fullscreen mode, we only change the saved window info - if self.fullscreen.borrow().is_some() { - { - let mut window_state = self.window_state.lock().unwrap(); + // if we are in fullscreen mode, we only change the saved window info + if window_state.fullscreen.is_some() { let saved = window_state.saved_window_info.as_mut().unwrap(); unsafe { @@ -677,81 +651,73 @@ impl Window { saved.ex_style as _, ); } - } - - self.decorations.replace(decorations); - return; - } - - unsafe { - let mut rect: RECT = mem::zeroed(); - winuser::GetWindowRect(self.window.0, &mut rect); - - let mut style = winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE); - let mut ex_style = winuser::GetWindowLongW(self.window.0, winuser::GWL_EXSTYLE); - unjust_window_rect(&mut rect, style as _, ex_style as _); - - if decorations { - style = style | style_flags; - ex_style = ex_style | ex_style_flags; } else { - style = style & !style_flags; - ex_style = ex_style & !ex_style_flags; + unsafe { + let mut rect: RECT = mem::zeroed(); + winuser::GetWindowRect(self.window.0, &mut rect); + + let mut style = winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE); + let mut ex_style = winuser::GetWindowLongW(self.window.0, winuser::GWL_EXSTYLE); + unjust_window_rect(&mut rect, style as _, ex_style as _); + + if decorations { + style = style | style_flags; + ex_style = ex_style | ex_style_flags; + } else { + style = style & !style_flags; + ex_style = ex_style & !ex_style_flags; + } + + let window = self.window.clone(); + + self.events_loop_proxy.execute_in_thread(move |_| { + winuser::SetWindowLongW(window.0, winuser::GWL_STYLE, style); + winuser::SetWindowLongW(window.0, winuser::GWL_EXSTYLE, ex_style); + winuser::AdjustWindowRectEx(&mut rect, style as _, 0, ex_style as _); + + winuser::SetWindowPos( + window.0, + ptr::null_mut(), + rect.left, + rect.top, + rect.right - rect.left, + rect.bottom - rect.top, + winuser::SWP_ASYNCWINDOWPOS + | winuser::SWP_NOZORDER + | winuser::SWP_NOACTIVATE + | winuser::SWP_FRAMECHANGED, + ); + }); + } } - - let window = self.window.clone(); - - self.events_loop_proxy.execute_in_thread(move |_| { - winuser::SetWindowLongW(window.0, winuser::GWL_STYLE, style); - winuser::SetWindowLongW(window.0, winuser::GWL_EXSTYLE, ex_style); - winuser::AdjustWindowRectEx(&mut rect, style as _, 0, ex_style as _); - - winuser::SetWindowPos( - window.0, - ptr::null_mut(), - rect.left, - rect.top, - rect.right - rect.left, - rect.bottom - rect.top, - winuser::SWP_ASYNCWINDOWPOS - | winuser::SWP_NOZORDER - | winuser::SWP_NOACTIVATE - | winuser::SWP_FRAMECHANGED, - ); - }); } - - self.decorations.replace(decorations); } #[inline] pub fn set_always_on_top(&self, always_on_top: bool) { - if self.always_on_top.get() == always_on_top { - return; + let mut window_state = self.window_state.lock().unwrap(); + if mem::replace(&mut window_state.always_on_top, always_on_top) != always_on_top { + let window = self.window.clone(); + self.events_loop_proxy.execute_in_thread(move |_| { + let insert_after = if always_on_top { + winuser::HWND_TOPMOST + } else { + winuser::HWND_NOTOPMOST + }; + unsafe { + winuser::SetWindowPos( + window.0, + insert_after, + 0, + 0, + 0, + 0, + winuser::SWP_ASYNCWINDOWPOS | winuser::SWP_NOMOVE | winuser::SWP_NOSIZE, + ); + winuser::UpdateWindow(window.0); + } + }); } - - let window = self.window.clone(); - self.events_loop_proxy.execute_in_thread(move |_| { - let insert_after = if always_on_top { - winuser::HWND_TOPMOST - } else { - winuser::HWND_NOTOPMOST - }; - unsafe { - winuser::SetWindowPos( - window.0, - insert_after, - 0, - 0, - 0, - 0, - winuser::SWP_ASYNCWINDOWPOS | winuser::SWP_NOMOVE | winuser::SWP_NOSIZE, - ); - winuser::UpdateWindow(window.0); - } - }); - - self.always_on_top.replace(always_on_top); } #[inline] @@ -771,7 +737,7 @@ impl Window { } else { icon::unset_for_window(self.window.0, IconType::Small); } - self.window_icon.replace(window_icon); + self.window_state.lock().unwrap().window_icon = window_icon; } #[inline] @@ -784,7 +750,7 @@ impl Window { } else { icon::unset_for_window(self.window.0, IconType::Big); } - self.taskbar_icon.replace(taskbar_icon); + self.window_state.lock().unwrap().taskbar_icon = taskbar_icon; } #[inline] @@ -809,15 +775,25 @@ impl Drop for Window { #[derive(Clone)] pub struct WindowWrapper(HWND, HDC); -// Send is not implemented for HWND and HDC, we have to wrap it and implement it manually. +// Send and Sync are not implemented for HWND and HDC, we have to wrap it and implement them manually. // For more info see: // https://github.com/retep998/winapi-rs/issues/360 // https://github.com/retep998/winapi-rs/issues/396 +unsafe impl Sync for WindowWrapper {} unsafe impl Send for WindowWrapper {} -pub unsafe fn adjust_size(physical_size: PhysicalSize, style: DWORD, ex_style: DWORD) -> (LONG, LONG) { +pub unsafe fn adjust_size( + physical_size: PhysicalSize, + style: DWORD, + ex_style: DWORD, +) -> (LONG, LONG) { let (width, height): (u32, u32) = physical_size.into(); - let mut rect = RECT { left: 0, right: width as LONG, top: 0, bottom: height as LONG }; + let mut rect = RECT { + left: 0, + right: width as LONG, + top: 0, + bottom: height as LONG, + }; winuser::AdjustWindowRectEx(&mut rect, style, 0, ex_style); (rect.right - rect.left, rect.bottom - rect.top) } @@ -1029,6 +1005,13 @@ unsafe fn init( mouse_in_window: false, saved_window_info: None, dpi_factor, + fullscreen: attributes.fullscreen.clone(), + window_icon, + taskbar_icon, + decorations: attributes.decorations, + maximized: attributes.maximized, + resizable: attributes.resizable, + always_on_top: attributes.always_on_top, }; // Creating a mutex to track the current window state Arc::new(Mutex::new(window_state)) @@ -1048,14 +1031,7 @@ unsafe fn init( let win = Window { window: real_window, - window_state: window_state, - decorations: Cell::new(attributes.decorations), - maximized: Cell::new(attributes.maximized.clone()), - resizable: Cell::new(attributes.resizable.clone()), - fullscreen: RefCell::new(attributes.fullscreen.clone()), - always_on_top: Cell::new(attributes.always_on_top), - window_icon: Cell::new(window_icon), - taskbar_icon: Cell::new(taskbar_icon), + window_state, events_loop_proxy, }; @@ -1112,7 +1088,6 @@ unsafe fn register_window_class( class_name } - struct ComInitialized(*mut ()); impl Drop for ComInitialized { fn drop(&mut self) {