diff --git a/.changes/windows-wrap-command-ids.md b/.changes/windows-wrap-command-ids.md new file mode 100644 index 00000000..339adfca --- /dev/null +++ b/.changes/windows-wrap-command-ids.md @@ -0,0 +1,5 @@ +--- +"muda": patch +--- + +On Windows, Fix menu items going dead after a process has created about 64.5k menus and items. diff --git a/src/platform_impl/windows/mod.rs b/src/platform_impl/windows/mod.rs index 727a900e..4d37a4ac 100644 --- a/src/platform_impl/windows/mod.rs +++ b/src/platform_impl/windows/mod.rs @@ -9,7 +9,7 @@ mod util; pub(crate) use self::icon::WinIcon as PlatformIcon; use self::{ dark_menu_bar::{WM_UAHDRAWMENU, WM_UAHDRAWMENUITEM}, - util::Owned, + util::{CounterU16, Owned}, }; use crate::{ @@ -34,8 +34,30 @@ use windows_sys::Win32::{ /// Type alias for a window handle (HWND) in Windows. type Hwnd = isize; -/// Internal command ids. Used for the `WM_COMMAND` message to identify which menu item was clicked. -static COUNTER: Counter = Counter::new_with_start(1000); +/// The first identifier Win32 reserves for window menu (system) commands. +const FIRST_SYSTEM_COMMAND_ID: u16 = SC_SIZE as u16; + +/// Command ids, used for the `WM_COMMAND` message to identify which menu item was clicked. Only +/// handed out to items `WM_COMMAND` can report. +/// +/// The high end stops short of [`FIRST_SYSTEM_COMMAND_ID`]. A `WM_COMMAND` normally never carries +/// an `SC_*` code, since the window menu reports through `WM_SYSCOMMAND` instead, but a host that +/// tracks that menu itself with `TrackPopupMenu` does receive one as a `WM_COMMAND`, on the very +/// window our subclass is watching. So we stay below the line to avoid conflicts with system commands. +/// +/// The low end is only a courtesy: it keeps us off zero, which Win32 spends on a dismissed +/// `TrackPopupMenu` and on every separator, see https://learn.microsoft.com/en-us/windows/win32/menurc/wm-command#menus +static COMMAND_IDS: CounterU16 = CounterU16::new_with_range(1000, FIRST_SYSTEM_COMMAND_ID - 1); + +/// The id a submenu holds in place of a command id, Win32 addresses a submenu by the container it +/// owns, and never reports one as a command. [`COMMAND_IDS`] starts above zero, so this can never +/// collide with the id of an item that does take commands. +const NO_COMMAND_ID: u16 = 0; + +/// Identities of root menus, used to key the accelerator table each item shares with the menus it +/// is attached to. These never reach Win32, so unlike command ids they must not wrap: two live +/// menus sharing an identity would corrupt that bookkeeping. +static MENU_IDS: Counter = Counter::new_with_start(1); #[cfg(feature = "snapshot")] pub(crate) fn dispatch_on_main_thread(f: F) @@ -57,16 +79,17 @@ pub(crate) fn run_predefined_action(_item_type: &crate::items::PredefinedMenuIte /// and also by all submenus and items of the menu so that they can add and remove their own accelerators. struct AcceleratorTable { handle: HACCEL, - entries: HashMap, + /// Keyed by the command id in each entry's `ACCEL::cmd`. + entries: HashMap, } impl AcceleratorTable { - fn insert(&mut self, id: u32, accel: ACCEL) { + fn insert(&mut self, id: u16, accel: ACCEL) { self.entries.insert(id, accel); self.rebuild(); } - fn remove(&mut self, id: u32) { + fn remove(&mut self, id: u16) { if self.entries.remove(&id).is_some() { self.rebuild(); } @@ -121,6 +144,7 @@ struct ParentMenu { /// A root menu bar that can be attached to a window. pub(crate) struct PlatformMenu { + /// Identity in the accelerator tables of this menu's items. Not a command id. id: u32, hmenu: HMENU, hpopupmenu: HMENU, @@ -185,7 +209,8 @@ impl Drop for PlatformMenu { /// A menu item that can be attached to a menu or submenu. pub(crate) struct PlatformMenuItem { - id: u32, + /// The command id `WM_COMMAND` reports a click with, or [`NO_COMMAND_ID`] for a submenu. + command_id: u16, /// What a click does. click: MenuItemAction, parents: Vec, @@ -233,16 +258,15 @@ impl Drop for PlatformMenuItem { } // 5. Remove the item from all accelerator tables it is in. - let id = self.id(); for entry in self.accelerator_tables.values() { - entry.table.borrow_mut().remove(id) + entry.table.borrow_mut().remove(self.command_id) } } } impl PlatformMenu { pub fn new() -> Self { - let id = COUNTER.next(); + let id = MENU_IDS.next(); Self { id, hmenu: unsafe { CreateMenu() }, @@ -419,7 +443,7 @@ impl PlatformMenu { )]) } - fn find_by_id(&self, id: u32) -> Option>> { + fn find_by_id(&self, id: u16) -> Option>> { find_by_id(id, &self.children) } @@ -433,7 +457,7 @@ impl PlatformMenu { impl PlatformMenuItem { pub fn new(click: MenuItemAction) -> Self { Self { - id: COUNTER.next(), + command_id: COMMAND_IDS.next(), click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -448,7 +472,8 @@ impl PlatformMenuItem { pub fn new_submenu(click: MenuItemAction) -> Self { Self { - id: COUNTER.next(), + // A submenu is never the target of a `WM_COMMAND`, so it takes no command id. + command_id: NO_COMMAND_ID, click, parents: Vec::new(), accelerator_tables: HashMap::new(), @@ -467,7 +492,7 @@ impl PlatformMenuItem { /// what `MF_POPUP` inserts, so it is what `MF_BYCOMMAND` matches. fn id(&self) -> u32 { if self.hmenu.is_null() { - self.id + u32::from(self.command_id) } else { self.hmenu as u32 } @@ -571,10 +596,8 @@ impl PlatformMenuItem { text: &str, accelerator: Option<&MenuAccelerator>, ) -> crate::Result<()> { - let id = self.id(); - let accel = accelerator - .map(|accelerator| accelerator.to_accel(id as _)) + .map(|accelerator| accelerator.to_accel(self.command_id)) .transpose()?; self.set_text(text, accelerator); @@ -584,8 +607,8 @@ impl PlatformMenuItem { let mut table = entry.table.borrow_mut(); match accel { - Some(accel) => table.insert(id, accel), - None => table.remove(id), + Some(accel) => table.insert(self.command_id, accel), + None => table.remove(self.command_id), } } @@ -673,7 +696,7 @@ impl PlatformMenuItem { unregister_accelerator_tables_from_subtree(&mut child, &self.accelerator_tables); } - fn find_by_id(&self, id: u32) -> Option>> { + fn find_by_id(&self, id: u16) -> Option>> { find_by_id(id, self.children.as_deref().unwrap_or_default()) } @@ -749,7 +772,7 @@ fn attach_item( let accel = args .accelerator .as_ref() - .map(|accelerator| accelerator.to_accel(id as _)) + .map(|accelerator| accelerator.to_accel(child.command_id)) .transpose()?; child.accel = accel; @@ -810,7 +833,6 @@ fn register_accelerator_tables_for_subtree( child: &mut PlatformMenuItem, tables: &HashMap, ) { - let id = child.id(); let accel = child.accel; for (&menu_id, parent_entry) in tables { @@ -822,6 +844,7 @@ fn register_accelerator_tables_for_subtree( if entry.count == 0 { if let Some(accel) = accel { + let id = child.command_id; parent_entry.table.borrow_mut().insert(id, accel); } } @@ -840,15 +863,13 @@ fn unregister_accelerator_tables_from_subtree( child: &mut PlatformMenuItem, tables: &HashMap, ) { - let id = child.id(); - for (menu_id, parent_entry) in tables { if let Some(entry) = child.accelerator_tables.get_mut(menu_id) { entry.count = entry.count.saturating_sub(parent_entry.count); if entry.count == 0 { let entry = child.accelerator_tables.remove(menu_id).unwrap(); - entry.table.borrow_mut().remove(id); + entry.table.borrow_mut().remove(child.command_id); } } } @@ -880,14 +901,19 @@ fn forget_one_parent(hmenu: HMENU, parents: &mut Vec) { } } -/// Finds a menu item by its ID in the specified children and their descendants. +/// Finds a menu item by its command id in the specified children and their descendants. fn find_by_id( - id: u32, + id: u16, children: &[Rc>], ) -> Option>> { + // Skip, see [`NO_COMMAND_ID`] for more details. + if id == NO_COMMAND_ID { + return None; + } + for child in children { let item = child.borrow(); - if item.id == id { + if item.command_id == id { return Some(child.clone()); } @@ -901,7 +927,7 @@ fn find_by_id( } /// Shows a context menu at the specified position, or at the current cursor position if no position is specified. -unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option) -> Option { +unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option) -> Option { let pt = if let Some(pos) = position { let dpi = util::hwnd_dpi(hwnd); let scale_factor = util::dpi_to_scale_factor(dpi); @@ -930,6 +956,8 @@ unsafe fn show_context_menu(hwnd: HWND, hmenu: HMENU, position: Option std::ptr::null(), ); + // Zero is the documented return for a menu the user dismissed, and `TPM_RETURNCMD` hands back + // the full stored identifier, so anything a command id cannot hold is not one of ours. (result > 0).then_some(result.try_into().ok()).flatten() } @@ -990,7 +1018,7 @@ unsafe extern "system" fn menu_subclass_proc( } WM_COMMAND => { - let id = util::LOWORD(wparam as _) as u32; + let id = util::LOWORD(wparam as _); let item = match uidsubclass { MENU_SUBCLASS_ID => { diff --git a/src/platform_impl/windows/util.rs b/src/platform_impl/windows/util.rs index aa891635..b5d1319b 100644 --- a/src/platform_impl/windows/util.rs +++ b/src/platform_impl/windows/util.rs @@ -2,7 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-License-Identifier: MIT -use std::sync::LazyLock; +use std::sync::{ + atomic::{AtomicU16, Ordering}, + LazyLock, +}; use windows_sys::{ core::HRESULT, Win32::{ @@ -74,6 +77,42 @@ impl Free for HBITMAP { } } +/// A counter similar to [crate::util::Counter] but wraps around instead of leaving its range. +pub struct CounterU16 { + start: u16, + end: u16, + current: AtomicU16, +} + +impl CounterU16 { + /// Counts over `start..=end`, inclusive of both. + pub const fn new_with_range(start: u16, end: u16) -> Self { + assert!(start < end, "a counter needs a range of at least two ids"); + + Self { + start, + end, + current: AtomicU16::new(start), + } + } + + pub fn next(&self) -> u16 { + // TODO: remove this deprecated allowance once we raise MSRV to 1.95 + // where we can use `try_update` instead. + #[allow(deprecated)] + let previous = self + .current + .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |id| { + Some(if id == self.end { self.start } else { id + 1 }) + }); + + // The update closure always returns `Some`, so both arms hold the previous value. + match previous { + Ok(id) | Err(id) => id, + } + } +} + pub fn encode_wide>(string: S) -> Vec { std::os::windows::prelude::OsStrExt::encode_wide(string.as_ref()) .chain(std::iter::once(0))