From fbb402ef12b0e61bc02db7a1715d0a36df1b94a7 Mon Sep 17 00:00:00 2001 From: Junkui Zhang <364772080@qq.com> Date: Wed, 18 Sep 2024 06:45:08 +0800 Subject: [PATCH] windows: Remove the use of `DispatcherQueue` and fix `FileSaveDialog` unresponsive issue (#17946) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #17069, closes #12410 With the help of @kennykerr (Creator of C++/WinRT and the crate `windows-rs`, Engineer on the Windows team at Microsoft) and @riverar (Windows Development expert), we discovered that this bug only occurs when an IME with a candidate window, such as Microsoft Pinyin IME, is active. In this case, the `FileSaveDialog` becomes unresponsive—while the dialog itself appears to be functioning, it doesn't accept any mouse or keyboard input. After a period of debugging and testing, I found that this issue only arises when using `DispatcherQueue` to dispatch runnables on the UI thread. After @kennykerr’s further investigation, Kenny identified that this is a bug with `DispatcherQueue`, and he recommended to avoid using `DispatcherQueue`. Given the uncertainty about whether Microsoft will address this bug in the foreseeable future, I have removed the use of `DispatcherQueue`. Co-authored-by: Kenny Release Notes: - N/A --------- Co-authored-by: Kenny --- Cargo.toml | 2 +- crates/gpui/Cargo.toml | 4 +- .../gpui/src/platform/windows/dispatcher.rs | 57 ++++-------- crates/gpui/src/platform/windows/events.rs | 3 + crates/gpui/src/platform/windows/platform.rs | 90 ++++++++++++++----- crates/gpui/src/platform/windows/window.rs | 19 ++-- 6 files changed, 104 insertions(+), 71 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0b392e02eb..ec3138179b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -490,7 +490,6 @@ features = [ "implement", "Foundation_Numerics", "Storage", - "System", "System_Threading", "UI_ViewManagement", "Wdk_System_SystemServices", @@ -521,6 +520,7 @@ features = [ "Win32_UI_Input_Ime", "Win32_UI_Input_KeyboardAndMouse", "Win32_UI_Shell", + "Win32_UI_Shell_Common", "Win32_UI_WindowsAndMessaging", ] diff --git a/crates/gpui/Cargo.toml b/crates/gpui/Cargo.toml index 09b546fc32..d0d75b73e9 100644 --- a/crates/gpui/Cargo.toml +++ b/crates/gpui/Cargo.toml @@ -50,7 +50,7 @@ parking = "2.0.0" parking_lot.workspace = true postage.workspace = true profiling.workspace = true -rand = { optional = true, workspace = true} +rand = { optional = true, workspace = true } raw-window-handle = "0.6" refineable.workspace = true resvg = { version = "0.41.0", default-features = false } @@ -110,6 +110,7 @@ blade-graphics.workspace = true blade-macros.workspace = true blade-util.workspace = true bytemuck = "1" +flume = "0.11" [target.'cfg(target_os = "linux")'.dependencies] as-raw-xcb-connection = "1" @@ -117,7 +118,6 @@ ashpd.workspace = true calloop = "0.13.0" calloop-wayland-source = "0.3.0" cosmic-text = { git = "https://github.com/pop-os/cosmic-text", rev = "542b20c" } -flume = "0.11" wayland-backend = { version = "0.3.3", features = ["client_system", "dlopen"] } wayland-client = { version = "0.31.2" } wayland-cursor = "0.31.1" diff --git a/crates/gpui/src/platform/windows/dispatcher.rs b/crates/gpui/src/platform/windows/dispatcher.rs index abe40d2c2e..575e844051 100644 --- a/crates/gpui/src/platform/windows/dispatcher.rs +++ b/crates/gpui/src/platform/windows/dispatcher.rs @@ -3,51 +3,39 @@ use std::{ time::Duration, }; +use anyhow::Context; use async_task::Runnable; +use flume::Sender; use parking::Parker; use parking_lot::Mutex; use util::ResultExt; use windows::{ Foundation::TimeSpan, - System::{ - DispatcherQueue, DispatcherQueueController, DispatcherQueueHandler, - Threading::{ - ThreadPool, ThreadPoolTimer, TimerElapsedHandler, WorkItemHandler, WorkItemOptions, - WorkItemPriority, - }, - }, - Win32::System::WinRT::{ - CreateDispatcherQueueController, DispatcherQueueOptions, DQTAT_COM_NONE, - DQTYPE_THREAD_CURRENT, + System::Threading::{ + ThreadPool, ThreadPoolTimer, TimerElapsedHandler, WorkItemHandler, WorkItemOptions, + WorkItemPriority, }, + Win32::{Foundation::HANDLE, System::Threading::SetEvent}, }; -use crate::{PlatformDispatcher, TaskLabel}; +use crate::{PlatformDispatcher, SafeHandle, TaskLabel}; pub(crate) struct WindowsDispatcher { - controller: DispatcherQueueController, - main_queue: DispatcherQueue, + main_sender: Sender, + dispatch_event: SafeHandle, parker: Mutex, main_thread_id: ThreadId, } impl WindowsDispatcher { - pub(crate) fn new() -> Self { - let controller = unsafe { - let options = DispatcherQueueOptions { - dwSize: std::mem::size_of::() as u32, - threadType: DQTYPE_THREAD_CURRENT, - apartmentType: DQTAT_COM_NONE, - }; - CreateDispatcherQueueController(options).unwrap() - }; - let main_queue = controller.DispatcherQueue().unwrap(); + pub(crate) fn new(main_sender: Sender, dispatch_event: HANDLE) -> Self { + let dispatch_event = dispatch_event.into(); let parker = Mutex::new(Parker::new()); let main_thread_id = current().id(); WindowsDispatcher { - controller, - main_queue, + main_sender, + dispatch_event, parker, main_thread_id, } @@ -86,12 +74,6 @@ impl WindowsDispatcher { } } -impl Drop for WindowsDispatcher { - fn drop(&mut self) { - self.controller.ShutdownQueueAsync().log_err(); - } -} - impl PlatformDispatcher for WindowsDispatcher { fn is_main_thread(&self) -> bool { current().id() == self.main_thread_id @@ -105,14 +87,11 @@ impl PlatformDispatcher for WindowsDispatcher { } fn dispatch_on_main_thread(&self, runnable: Runnable) { - let handler = { - let mut task_wrapper = Some(runnable); - DispatcherQueueHandler::new(move || { - task_wrapper.take().unwrap().run(); - Ok(()) - }) - }; - self.main_queue.TryEnqueue(&handler).log_err(); + self.main_sender + .send(runnable) + .context("Dispatch on main thread failed") + .log_err(); + unsafe { SetEvent(*self.dispatch_event).log_err() }; } fn dispatch_after(&self, duration: Duration, runnable: Runnable) { diff --git a/crates/gpui/src/platform/windows/events.rs b/crates/gpui/src/platform/windows/events.rs index 0d55142ae9..b62f51f6d9 100644 --- a/crates/gpui/src/platform/windows/events.rs +++ b/crates/gpui/src/platform/windows/events.rs @@ -177,6 +177,9 @@ fn handle_timer_msg( state_ptr: Rc, ) -> Option { if wparam.0 == SIZE_MOVE_LOOP_TIMER_ID { + for runnable in state_ptr.main_receiver.drain() { + runnable.run(); + } handle_paint_msg(handle, state_ptr) } else { None diff --git a/crates/gpui/src/platform/windows/platform.rs b/crates/gpui/src/platform/windows/platform.rs index 934d9336d2..d9f08c2247 100644 --- a/crates/gpui/src/platform/windows/platform.rs +++ b/crates/gpui/src/platform/windows/platform.rs @@ -8,6 +8,7 @@ use std::{ use ::util::ResultExt; use anyhow::{anyhow, Context, Result}; +use async_task::Runnable; use futures::channel::oneshot::{self, Receiver}; use itertools::Itertools; use parking_lot::RwLock; @@ -46,6 +47,8 @@ pub(crate) struct WindowsPlatform { raw_window_handles: RwLock>, // The below members will never change throughout the entire lifecycle of the app. icon: HICON, + main_receiver: flume::Receiver, + dispatch_event: HANDLE, background_executor: BackgroundExecutor, foreground_executor: ForegroundExecutor, text_system: Arc, @@ -89,7 +92,9 @@ impl WindowsPlatform { unsafe { OleInitialize(None).expect("unable to initialize Windows OLE"); } - let dispatcher = Arc::new(WindowsDispatcher::new()); + let (main_sender, main_receiver) = flume::unbounded::(); + let dispatch_event = unsafe { CreateEventW(None, false, false, None) }.unwrap(); + let dispatcher = Arc::new(WindowsDispatcher::new(main_sender, dispatch_event)); let background_executor = BackgroundExecutor::new(dispatcher.clone()); let foreground_executor = ForegroundExecutor::new(dispatcher); let bitmap_factory = ManuallyDrop::new(unsafe { @@ -113,6 +118,8 @@ impl WindowsPlatform { state, raw_window_handles, icon, + main_receiver, + dispatch_event, background_executor, foreground_executor, text_system, @@ -176,6 +183,24 @@ impl WindowsPlatform { lock.is_empty() } + + #[inline] + fn run_foreground_tasks(&self) { + for runnable in self.main_receiver.drain() { + runnable.run(); + } + } + + fn generate_creation_info(&self) -> WindowCreationInfo { + WindowCreationInfo { + icon: self.icon, + executor: self.foreground_executor.clone(), + current_cursor: self.state.borrow().current_cursor, + windows_version: self.windows_version, + validation_number: self.validation_number, + main_receiver: self.main_receiver.clone(), + } + } } impl Platform for WindowsPlatform { @@ -197,16 +222,21 @@ impl Platform for WindowsPlatform { begin_vsync(*vsync_event); 'a: loop { let wait_result = unsafe { - MsgWaitForMultipleObjects(Some(&[*vsync_event]), false, INFINITE, QS_ALLINPUT) + MsgWaitForMultipleObjects( + Some(&[*vsync_event, self.dispatch_event]), + false, + INFINITE, + QS_ALLINPUT, + ) }; match wait_result { // compositor clock ticked so we should draw a frame - WAIT_EVENT(0) => { - self.redraw_all(); - } + WAIT_EVENT(0) => self.redraw_all(), + // foreground tasks are dispatched + WAIT_EVENT(1) => self.run_foreground_tasks(), // Windows thread messages are posted - WAIT_EVENT(1) => { + WAIT_EVENT(2) => { let mut msg = MSG::default(); unsafe { while PeekMessageW(&mut msg, None, 0, 0, PM_REMOVE).as_bool() { @@ -230,6 +260,8 @@ impl Platform for WindowsPlatform { } } } + // foreground tasks may have been queued in the message handlers + self.run_foreground_tasks(); } _ => { log::error!("Something went wrong while waiting {:?}", wait_result); @@ -319,17 +351,7 @@ impl Platform for WindowsPlatform { handle: AnyWindowHandle, options: WindowParams, ) -> Result> { - let lock = self.state.borrow(); - let window = WindowsWindow::new( - handle, - options, - self.icon, - self.foreground_executor.clone(), - lock.current_cursor, - self.windows_version, - self.validation_number, - )?; - drop(lock); + let window = WindowsWindow::new(handle, options, self.generate_creation_info())?; let handle = window.get_raw_handle(); self.raw_window_handles.write().push(handle); @@ -558,6 +580,15 @@ impl Drop for WindowsPlatform { } } +pub(crate) struct WindowCreationInfo { + pub(crate) icon: HICON, + pub(crate) executor: ForegroundExecutor, + pub(crate) current_cursor: HCURSOR, + pub(crate) windows_version: WindowsVersion, + pub(crate) validation_number: usize, + pub(crate) main_receiver: flume::Receiver, +} + fn open_target(target: &str) { unsafe { let ret = ShellExecuteW( @@ -631,22 +662,33 @@ fn file_open_dialog(options: PathPromptOptions) -> Result>> fn file_save_dialog(directory: PathBuf) -> Result> { let dialog: IFileSaveDialog = unsafe { CoCreateInstance(&FileSaveDialog, None, CLSCTX_ALL)? }; - if let Some(full_path) = directory.canonicalize().log_err() { - let full_path = full_path.to_string_lossy().to_string(); - if !full_path.is_empty() { - let path_item: IShellItem = - unsafe { SHCreateItemFromParsingName(&HSTRING::from(&full_path), None)? }; - unsafe { dialog.SetFolder(&path_item).log_err() }; + if !directory.to_string_lossy().is_empty() { + if let Some(full_path) = directory.canonicalize().log_err() { + let full_path = full_path.to_string_lossy().to_string(); + if !full_path.is_empty() { + let path_item: IShellItem = + unsafe { SHCreateItemFromParsingName(&HSTRING::from(&full_path), None)? }; + unsafe { dialog.SetFolder(&path_item).log_err() }; + } } } unsafe { + dialog.SetFileTypes(&[Common::COMDLG_FILTERSPEC { + pszName: windows::core::w!("All files"), + pszSpec: windows::core::w!("*.*"), + }])?; if dialog.Show(None).is_err() { // User cancelled return Ok(None); } } let shell_item = unsafe { dialog.GetResult()? }; - let file_path_string = unsafe { shell_item.GetDisplayName(SIGDN_FILESYSPATH)?.to_string()? }; + let file_path_string = unsafe { + let pwstr = shell_item.GetDisplayName(SIGDN_FILESYSPATH)?; + let string = pwstr.to_string()?; + CoTaskMemFree(Some(pwstr.0 as _)); + string + }; Ok(Some(PathBuf::from(file_path_string))) } diff --git a/crates/gpui/src/platform/windows/window.rs b/crates/gpui/src/platform/windows/window.rs index 1a059491a2..e2cfb38afd 100644 --- a/crates/gpui/src/platform/windows/window.rs +++ b/crates/gpui/src/platform/windows/window.rs @@ -12,6 +12,7 @@ use std::{ use ::util::ResultExt; use anyhow::{Context, Result}; +use async_task::Runnable; use futures::channel::oneshot::{self, Receiver}; use itertools::Itertools; use raw_window_handle as rwh; @@ -63,6 +64,7 @@ pub(crate) struct WindowsWindowStatePtr { pub(crate) executor: ForegroundExecutor, pub(crate) windows_version: WindowsVersion, pub(crate) validation_number: usize, + pub(crate) main_receiver: flume::Receiver, } impl WindowsWindowState { @@ -226,6 +228,7 @@ impl WindowsWindowStatePtr { executor: context.executor.clone(), windows_version: context.windows_version, validation_number: context.validation_number, + main_receiver: context.main_receiver.clone(), })) } } @@ -253,18 +256,23 @@ struct WindowCreateContext { current_cursor: HCURSOR, windows_version: WindowsVersion, validation_number: usize, + main_receiver: flume::Receiver, } impl WindowsWindow { pub(crate) fn new( handle: AnyWindowHandle, params: WindowParams, - icon: HICON, - executor: ForegroundExecutor, - current_cursor: HCURSOR, - windows_version: WindowsVersion, - validation_number: usize, + creation_info: WindowCreationInfo, ) -> Result { + let WindowCreationInfo { + icon, + executor, + current_cursor, + windows_version, + validation_number, + main_receiver, + } = creation_info; let classname = register_wnd_class(icon); let hide_title_bar = params .titlebar @@ -305,6 +313,7 @@ impl WindowsWindow { current_cursor, windows_version, validation_number, + main_receiver, }; let lpparam = Some(&context as *const _ as *const _); let creation_result = unsafe {