windows: Remove the use of DispatcherQueue and fix FileSaveDialog unresponsive issue (#17946)

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 <kenny@kennykerr.ca>

Release Notes:

- N/A

---------

Co-authored-by: Kenny <kenny@kennykerr.ca>
This commit is contained in:
Junkui Zhang 2024-09-18 06:45:08 +08:00 committed by GitHub
parent 56f9e4c7b3
commit fbb402ef12
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 104 additions and 71 deletions

View file

@ -490,7 +490,6 @@ features = [
"implement", "implement",
"Foundation_Numerics", "Foundation_Numerics",
"Storage", "Storage",
"System",
"System_Threading", "System_Threading",
"UI_ViewManagement", "UI_ViewManagement",
"Wdk_System_SystemServices", "Wdk_System_SystemServices",
@ -521,6 +520,7 @@ features = [
"Win32_UI_Input_Ime", "Win32_UI_Input_Ime",
"Win32_UI_Input_KeyboardAndMouse", "Win32_UI_Input_KeyboardAndMouse",
"Win32_UI_Shell", "Win32_UI_Shell",
"Win32_UI_Shell_Common",
"Win32_UI_WindowsAndMessaging", "Win32_UI_WindowsAndMessaging",
] ]

View file

@ -50,7 +50,7 @@ parking = "2.0.0"
parking_lot.workspace = true parking_lot.workspace = true
postage.workspace = true postage.workspace = true
profiling.workspace = true profiling.workspace = true
rand = { optional = true, workspace = true} rand = { optional = true, workspace = true }
raw-window-handle = "0.6" raw-window-handle = "0.6"
refineable.workspace = true refineable.workspace = true
resvg = { version = "0.41.0", default-features = false } resvg = { version = "0.41.0", default-features = false }
@ -110,6 +110,7 @@ blade-graphics.workspace = true
blade-macros.workspace = true blade-macros.workspace = true
blade-util.workspace = true blade-util.workspace = true
bytemuck = "1" bytemuck = "1"
flume = "0.11"
[target.'cfg(target_os = "linux")'.dependencies] [target.'cfg(target_os = "linux")'.dependencies]
as-raw-xcb-connection = "1" as-raw-xcb-connection = "1"
@ -117,7 +118,6 @@ ashpd.workspace = true
calloop = "0.13.0" calloop = "0.13.0"
calloop-wayland-source = "0.3.0" calloop-wayland-source = "0.3.0"
cosmic-text = { git = "https://github.com/pop-os/cosmic-text", rev = "542b20c" } 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-backend = { version = "0.3.3", features = ["client_system", "dlopen"] }
wayland-client = { version = "0.31.2" } wayland-client = { version = "0.31.2" }
wayland-cursor = "0.31.1" wayland-cursor = "0.31.1"

View file

@ -3,51 +3,39 @@ use std::{
time::Duration, time::Duration,
}; };
use anyhow::Context;
use async_task::Runnable; use async_task::Runnable;
use flume::Sender;
use parking::Parker; use parking::Parker;
use parking_lot::Mutex; use parking_lot::Mutex;
use util::ResultExt; use util::ResultExt;
use windows::{ use windows::{
Foundation::TimeSpan, Foundation::TimeSpan,
System::{ System::Threading::{
DispatcherQueue, DispatcherQueueController, DispatcherQueueHandler, ThreadPool, ThreadPoolTimer, TimerElapsedHandler, WorkItemHandler, WorkItemOptions,
Threading::{ WorkItemPriority,
ThreadPool, ThreadPoolTimer, TimerElapsedHandler, WorkItemHandler, WorkItemOptions,
WorkItemPriority,
},
},
Win32::System::WinRT::{
CreateDispatcherQueueController, DispatcherQueueOptions, DQTAT_COM_NONE,
DQTYPE_THREAD_CURRENT,
}, },
Win32::{Foundation::HANDLE, System::Threading::SetEvent},
}; };
use crate::{PlatformDispatcher, TaskLabel}; use crate::{PlatformDispatcher, SafeHandle, TaskLabel};
pub(crate) struct WindowsDispatcher { pub(crate) struct WindowsDispatcher {
controller: DispatcherQueueController, main_sender: Sender<Runnable>,
main_queue: DispatcherQueue, dispatch_event: SafeHandle,
parker: Mutex<Parker>, parker: Mutex<Parker>,
main_thread_id: ThreadId, main_thread_id: ThreadId,
} }
impl WindowsDispatcher { impl WindowsDispatcher {
pub(crate) fn new() -> Self { pub(crate) fn new(main_sender: Sender<Runnable>, dispatch_event: HANDLE) -> Self {
let controller = unsafe { let dispatch_event = dispatch_event.into();
let options = DispatcherQueueOptions {
dwSize: std::mem::size_of::<DispatcherQueueOptions>() as u32,
threadType: DQTYPE_THREAD_CURRENT,
apartmentType: DQTAT_COM_NONE,
};
CreateDispatcherQueueController(options).unwrap()
};
let main_queue = controller.DispatcherQueue().unwrap();
let parker = Mutex::new(Parker::new()); let parker = Mutex::new(Parker::new());
let main_thread_id = current().id(); let main_thread_id = current().id();
WindowsDispatcher { WindowsDispatcher {
controller, main_sender,
main_queue, dispatch_event,
parker, parker,
main_thread_id, 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 { impl PlatformDispatcher for WindowsDispatcher {
fn is_main_thread(&self) -> bool { fn is_main_thread(&self) -> bool {
current().id() == self.main_thread_id current().id() == self.main_thread_id
@ -105,14 +87,11 @@ impl PlatformDispatcher for WindowsDispatcher {
} }
fn dispatch_on_main_thread(&self, runnable: Runnable) { fn dispatch_on_main_thread(&self, runnable: Runnable) {
let handler = { self.main_sender
let mut task_wrapper = Some(runnable); .send(runnable)
DispatcherQueueHandler::new(move || { .context("Dispatch on main thread failed")
task_wrapper.take().unwrap().run(); .log_err();
Ok(()) unsafe { SetEvent(*self.dispatch_event).log_err() };
})
};
self.main_queue.TryEnqueue(&handler).log_err();
} }
fn dispatch_after(&self, duration: Duration, runnable: Runnable) { fn dispatch_after(&self, duration: Duration, runnable: Runnable) {

View file

@ -177,6 +177,9 @@ fn handle_timer_msg(
state_ptr: Rc<WindowsWindowStatePtr>, state_ptr: Rc<WindowsWindowStatePtr>,
) -> Option<isize> { ) -> Option<isize> {
if wparam.0 == SIZE_MOVE_LOOP_TIMER_ID { if wparam.0 == SIZE_MOVE_LOOP_TIMER_ID {
for runnable in state_ptr.main_receiver.drain() {
runnable.run();
}
handle_paint_msg(handle, state_ptr) handle_paint_msg(handle, state_ptr)
} else { } else {
None None

View file

@ -8,6 +8,7 @@ use std::{
use ::util::ResultExt; use ::util::ResultExt;
use anyhow::{anyhow, Context, Result}; use anyhow::{anyhow, Context, Result};
use async_task::Runnable;
use futures::channel::oneshot::{self, Receiver}; use futures::channel::oneshot::{self, Receiver};
use itertools::Itertools; use itertools::Itertools;
use parking_lot::RwLock; use parking_lot::RwLock;
@ -46,6 +47,8 @@ pub(crate) struct WindowsPlatform {
raw_window_handles: RwLock<SmallVec<[HWND; 4]>>, raw_window_handles: RwLock<SmallVec<[HWND; 4]>>,
// The below members will never change throughout the entire lifecycle of the app. // The below members will never change throughout the entire lifecycle of the app.
icon: HICON, icon: HICON,
main_receiver: flume::Receiver<Runnable>,
dispatch_event: HANDLE,
background_executor: BackgroundExecutor, background_executor: BackgroundExecutor,
foreground_executor: ForegroundExecutor, foreground_executor: ForegroundExecutor,
text_system: Arc<DirectWriteTextSystem>, text_system: Arc<DirectWriteTextSystem>,
@ -89,7 +92,9 @@ impl WindowsPlatform {
unsafe { unsafe {
OleInitialize(None).expect("unable to initialize Windows OLE"); OleInitialize(None).expect("unable to initialize Windows OLE");
} }
let dispatcher = Arc::new(WindowsDispatcher::new()); let (main_sender, main_receiver) = flume::unbounded::<Runnable>();
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 background_executor = BackgroundExecutor::new(dispatcher.clone());
let foreground_executor = ForegroundExecutor::new(dispatcher); let foreground_executor = ForegroundExecutor::new(dispatcher);
let bitmap_factory = ManuallyDrop::new(unsafe { let bitmap_factory = ManuallyDrop::new(unsafe {
@ -113,6 +118,8 @@ impl WindowsPlatform {
state, state,
raw_window_handles, raw_window_handles,
icon, icon,
main_receiver,
dispatch_event,
background_executor, background_executor,
foreground_executor, foreground_executor,
text_system, text_system,
@ -176,6 +183,24 @@ impl WindowsPlatform {
lock.is_empty() 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 { impl Platform for WindowsPlatform {
@ -197,16 +222,21 @@ impl Platform for WindowsPlatform {
begin_vsync(*vsync_event); begin_vsync(*vsync_event);
'a: loop { 'a: loop {
let wait_result = unsafe { 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 { match wait_result {
// compositor clock ticked so we should draw a frame // compositor clock ticked so we should draw a frame
WAIT_EVENT(0) => { WAIT_EVENT(0) => self.redraw_all(),
self.redraw_all(); // foreground tasks are dispatched
} WAIT_EVENT(1) => self.run_foreground_tasks(),
// Windows thread messages are posted // Windows thread messages are posted
WAIT_EVENT(1) => { WAIT_EVENT(2) => {
let mut msg = MSG::default(); let mut msg = MSG::default();
unsafe { unsafe {
while PeekMessageW(&mut msg, None, 0, 0, PM_REMOVE).as_bool() { 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); log::error!("Something went wrong while waiting {:?}", wait_result);
@ -319,17 +351,7 @@ impl Platform for WindowsPlatform {
handle: AnyWindowHandle, handle: AnyWindowHandle,
options: WindowParams, options: WindowParams,
) -> Result<Box<dyn PlatformWindow>> { ) -> Result<Box<dyn PlatformWindow>> {
let lock = self.state.borrow(); let window = WindowsWindow::new(handle, options, self.generate_creation_info())?;
let window = WindowsWindow::new(
handle,
options,
self.icon,
self.foreground_executor.clone(),
lock.current_cursor,
self.windows_version,
self.validation_number,
)?;
drop(lock);
let handle = window.get_raw_handle(); let handle = window.get_raw_handle();
self.raw_window_handles.write().push(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<Runnable>,
}
fn open_target(target: &str) { fn open_target(target: &str) {
unsafe { unsafe {
let ret = ShellExecuteW( let ret = ShellExecuteW(
@ -631,22 +662,33 @@ fn file_open_dialog(options: PathPromptOptions) -> Result<Option<Vec<PathBuf>>>
fn file_save_dialog(directory: PathBuf) -> Result<Option<PathBuf>> { fn file_save_dialog(directory: PathBuf) -> Result<Option<PathBuf>> {
let dialog: IFileSaveDialog = unsafe { CoCreateInstance(&FileSaveDialog, None, CLSCTX_ALL)? }; let dialog: IFileSaveDialog = unsafe { CoCreateInstance(&FileSaveDialog, None, CLSCTX_ALL)? };
if let Some(full_path) = directory.canonicalize().log_err() { if !directory.to_string_lossy().is_empty() {
let full_path = full_path.to_string_lossy().to_string(); if let Some(full_path) = directory.canonicalize().log_err() {
if !full_path.is_empty() { let full_path = full_path.to_string_lossy().to_string();
let path_item: IShellItem = if !full_path.is_empty() {
unsafe { SHCreateItemFromParsingName(&HSTRING::from(&full_path), None)? }; let path_item: IShellItem =
unsafe { dialog.SetFolder(&path_item).log_err() }; unsafe { SHCreateItemFromParsingName(&HSTRING::from(&full_path), None)? };
unsafe { dialog.SetFolder(&path_item).log_err() };
}
} }
} }
unsafe { unsafe {
dialog.SetFileTypes(&[Common::COMDLG_FILTERSPEC {
pszName: windows::core::w!("All files"),
pszSpec: windows::core::w!("*.*"),
}])?;
if dialog.Show(None).is_err() { if dialog.Show(None).is_err() {
// User cancelled // User cancelled
return Ok(None); return Ok(None);
} }
} }
let shell_item = unsafe { dialog.GetResult()? }; 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))) Ok(Some(PathBuf::from(file_path_string)))
} }

View file

@ -12,6 +12,7 @@ use std::{
use ::util::ResultExt; use ::util::ResultExt;
use anyhow::{Context, Result}; use anyhow::{Context, Result};
use async_task::Runnable;
use futures::channel::oneshot::{self, Receiver}; use futures::channel::oneshot::{self, Receiver};
use itertools::Itertools; use itertools::Itertools;
use raw_window_handle as rwh; use raw_window_handle as rwh;
@ -63,6 +64,7 @@ pub(crate) struct WindowsWindowStatePtr {
pub(crate) executor: ForegroundExecutor, pub(crate) executor: ForegroundExecutor,
pub(crate) windows_version: WindowsVersion, pub(crate) windows_version: WindowsVersion,
pub(crate) validation_number: usize, pub(crate) validation_number: usize,
pub(crate) main_receiver: flume::Receiver<Runnable>,
} }
impl WindowsWindowState { impl WindowsWindowState {
@ -226,6 +228,7 @@ impl WindowsWindowStatePtr {
executor: context.executor.clone(), executor: context.executor.clone(),
windows_version: context.windows_version, windows_version: context.windows_version,
validation_number: context.validation_number, validation_number: context.validation_number,
main_receiver: context.main_receiver.clone(),
})) }))
} }
} }
@ -253,18 +256,23 @@ struct WindowCreateContext {
current_cursor: HCURSOR, current_cursor: HCURSOR,
windows_version: WindowsVersion, windows_version: WindowsVersion,
validation_number: usize, validation_number: usize,
main_receiver: flume::Receiver<Runnable>,
} }
impl WindowsWindow { impl WindowsWindow {
pub(crate) fn new( pub(crate) fn new(
handle: AnyWindowHandle, handle: AnyWindowHandle,
params: WindowParams, params: WindowParams,
icon: HICON, creation_info: WindowCreationInfo,
executor: ForegroundExecutor,
current_cursor: HCURSOR,
windows_version: WindowsVersion,
validation_number: usize,
) -> Result<Self> { ) -> Result<Self> {
let WindowCreationInfo {
icon,
executor,
current_cursor,
windows_version,
validation_number,
main_receiver,
} = creation_info;
let classname = register_wnd_class(icon); let classname = register_wnd_class(icon);
let hide_title_bar = params let hide_title_bar = params
.titlebar .titlebar
@ -305,6 +313,7 @@ impl WindowsWindow {
current_cursor, current_cursor,
windows_version, windows_version,
validation_number, validation_number,
main_receiver,
}; };
let lpparam = Some(&context as *const _ as *const _); let lpparam = Some(&context as *const _ as *const _);
let creation_result = unsafe { let creation_result = unsafe {