From a9fe18f4cb0f872d2d2632071fc6dded1fd67adf Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 25 Sep 2025 15:36:19 +0200 Subject: [PATCH] Revert "gpui: Flash menu in menubar on macOS when action is triggered (#38588)" (#38880) This reverts commit ed7bd5a8ed6cbda563c4e6f1acb66e13f545718b. We noticed this PR causes the editor to hang if you hold down any of the menu item actions like ctrl+z, ctrl+x, etc Release Notes: - Fixed macOS menu item actions hanging the editor when their key combination is held down --- crates/gpui/examples/set_menus.rs | 65 +++++--------------- crates/gpui/src/platform.rs | 1 - crates/gpui/src/platform/linux/platform.rs | 2 - crates/gpui/src/platform/mac/platform.rs | 60 +----------------- crates/gpui/src/platform/test/platform.rs | 4 +- crates/gpui/src/platform/windows/platform.rs | 2 - crates/gpui/src/window.rs | 3 - 7 files changed, 18 insertions(+), 119 deletions(-) diff --git a/crates/gpui/examples/set_menus.rs b/crates/gpui/examples/set_menus.rs index 2488b5c100b1b7b27530ebf3e26143eb0a92b8fd..8a97a8d8a2ce8135b1af0e981a186586c2d5ebb8 100644 --- a/crates/gpui/examples/set_menus.rs +++ b/crates/gpui/examples/set_menus.rs @@ -1,17 +1,13 @@ use gpui::{ - App, Application, Context, FocusHandle, KeyBinding, Menu, MenuItem, PromptLevel, - SystemMenuType, Window, WindowOptions, actions, div, prelude::*, rgb, + App, Application, Context, Menu, MenuItem, SystemMenuType, Window, WindowOptions, actions, div, + prelude::*, rgb, }; -struct SetMenus { - focus_handle: FocusHandle, -} +struct SetMenus; impl Render for SetMenus { fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { div() - .key_context("root") - .track_focus(&self.focus_handle) .flex() .bg(rgb(0x2e7d32)) .size_full() @@ -19,16 +15,6 @@ impl Render for SetMenus { .items_center() .text_xl() .text_color(rgb(0xffffff)) - // Actions can also be registered within elements so they are only active when relevant - .on_action(|_: &Open, window, cx| { - let _ = window.prompt(PromptLevel::Info, "Open action fired", None, &["OK"], cx); - }) - .on_action(|_: &Copy, window, cx| { - let _ = window.prompt(PromptLevel::Info, "Copy action fired", None, &["OK"], cx); - }) - .on_action(|_: &Paste, window, cx| { - let _ = window.prompt(PromptLevel::Info, "Paste action fired", None, &["OK"], cx); - }) .child("Set Menus Example") } } @@ -37,47 +23,24 @@ fn main() { Application::new().run(|cx: &mut App| { // Bring the menu bar to the foreground (so you can see the menu bar) cx.activate(true); - // Bind keys to some menu actions - cx.bind_keys([ - KeyBinding::new("secondary-o", Open, None), - KeyBinding::new("secondary-c", Copy, None), - KeyBinding::new("secondary-v", Paste, None), - ]); // Register the `quit` function so it can be referenced by the `MenuItem::action` in the menu bar cx.on_action(quit); // Add menu items - cx.set_menus(vec![ - Menu { - name: "set_menus".into(), - items: vec![ - MenuItem::os_submenu("Services", SystemMenuType::Services), - MenuItem::separator(), - MenuItem::action("Quit", Quit), - ], - }, - Menu { - name: "File".into(), - items: vec![MenuItem::action("Open", Open)], - }, - Menu { - name: "Edit".into(), - items: vec![ - MenuItem::action("Copy", Copy), - MenuItem::action("Paste", Paste), - ], - }, - ]); - cx.open_window(WindowOptions::default(), |_, cx| { - cx.new(|cx| SetMenus { - focus_handle: cx.focus_handle(), - }) - }) - .unwrap(); + cx.set_menus(vec![Menu { + name: "set_menus".into(), + items: vec![ + MenuItem::os_submenu("Services", SystemMenuType::Services), + MenuItem::separator(), + MenuItem::action("Quit", Quit), + ], + }]); + cx.open_window(WindowOptions::default(), |_, cx| cx.new(|_| SetMenus {})) + .unwrap(); }); } // Associate actions using the `actions!` macro (or `Action` derive macro) -actions!(set_menus, [Quit, Open, Copy, Paste]); +actions!(set_menus, [Quit]); // Define the quit function that is registered with the App fn quit(_: &Quit, cx: &mut App) { diff --git a/crates/gpui/src/platform.rs b/crates/gpui/src/platform.rs index 05b597e9b6936a842ac2cdf7a4784f306ac07eaf..444b60ac154424c423c3cd6a827b22cd7024694f 100644 --- a/crates/gpui/src/platform.rs +++ b/crates/gpui/src/platform.rs @@ -250,7 +250,6 @@ pub(crate) trait Platform: 'static { fn on_app_menu_action(&self, callback: Box); fn on_will_open_app_menu(&self, callback: Box); fn on_validate_app_menu_command(&self, callback: Box bool>); - fn on_action_triggered(&self, action: &dyn Action); fn compositor_name(&self) -> &'static str { "" diff --git a/crates/gpui/src/platform/linux/platform.rs b/crates/gpui/src/platform/linux/platform.rs index a6e22c42da26233a0a83a7e9a1293ef262526170..196e5b65d04125ca90c588212c140d3a63345c2e 100644 --- a/crates/gpui/src/platform/linux/platform.rs +++ b/crates/gpui/src/platform/linux/platform.rs @@ -442,8 +442,6 @@ impl Platform for P { }); } - fn on_action_triggered(&self, _action: &dyn Action) {} - fn app_path(&self) -> Result { // get the path of the executable of the current process let app_path = env::current_exe()?; diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index e0a1d7770d0fea7022960702c250a4bd7d813a20..9909c78c472a17e683380be71b65484800c0fa76 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/crates/gpui/src/platform/mac/platform.rs @@ -177,8 +177,6 @@ pub(crate) struct MacPlatformState { dock_menu: Option, menus: Option>, keyboard_mapper: Rc, - action_menus: Vec<(Box, id)>, - handling_menu_item: bool, } impl Default for MacPlatform { @@ -221,8 +219,6 @@ impl MacPlatform { on_keyboard_layout_change: None, menus: None, keyboard_mapper, - handling_menu_item: false, - action_menus: Vec::new(), })) } @@ -245,7 +241,6 @@ impl MacPlatform { menus: &Vec, delegate: id, actions: &mut Vec>, - action_menus: &mut Vec<(Box, id)>, keymap: &Keymap, ) -> id { unsafe { @@ -263,7 +258,6 @@ impl MacPlatform { item_config, delegate, actions, - action_menus, keymap, )); } @@ -298,7 +292,6 @@ impl MacPlatform { &item_config, delegate, actions, - &mut Vec::new(), keymap, )); } @@ -311,7 +304,6 @@ impl MacPlatform { item: &MenuItem, delegate: id, actions: &mut Vec>, - action_menus: &mut Vec<(Box, id)>, keymap: &Keymap, ) -> id { static DEFAULT_CONTEXT: OnceLock> = OnceLock::new(); @@ -420,7 +412,6 @@ impl MacPlatform { let tag = actions.len() as NSInteger; let _: () = msg_send![item, setTag: tag]; actions.push(action.boxed_clone()); - action_menus.push((action.boxed_clone(), item)); item } MenuItem::Submenu(Menu { name, items }) => { @@ -428,13 +419,7 @@ impl MacPlatform { let submenu = NSMenu::new(nil).autorelease(); submenu.setDelegate_(delegate); for item in items { - submenu.addItem_(Self::create_menu_item( - item, - delegate, - actions, - action_menus, - keymap, - )); + submenu.addItem_(Self::create_menu_item(item, delegate, actions, keymap)); } item.setSubmenu_(submenu); item.setTitle_(ns_string(name)); @@ -903,30 +888,6 @@ impl Platform for MacPlatform { self.0.lock().validate_menu_command = Some(callback); } - fn on_action_triggered(&self, action: &dyn Action) { - let menu_item = { - let mut state = self.0.lock(); - let Some(menu_item) = state - .action_menus - .iter() - .find(|(menu_action, _)| menu_action.partial_eq(action)) - .map(|(_, menu)| *menu) - else { - return; - }; - - state.handling_menu_item = true; - menu_item - }; - - unsafe { - let parent: id = msg_send![menu_item, menu]; - let index: NSInteger = msg_send![parent, indexOfItem: menu_item]; - let _: () = msg_send![parent, performActionForItemAtIndex: index]; - } - self.0.lock().handling_menu_item = false; - } - fn keyboard_layout(&self) -> Box { Box::new(MacKeyboardLayout::new()) } @@ -944,24 +905,15 @@ impl Platform for MacPlatform { } fn set_menus(&self, menus: Vec, keymap: &Keymap) { - let mut action_menus = Vec::new(); unsafe { let app: id = msg_send![APP_CLASS, sharedApplication]; let mut state = self.0.lock(); let actions = &mut state.menu_actions; - let menu = self.create_menu_bar( - &menus, - NSWindow::delegate(app), - actions, - &mut action_menus, - keymap, - ); + let menu = self.create_menu_bar(&menus, NSWindow::delegate(app), actions, keymap); drop(state); app.setMainMenu_(menu); } - let mut state = self.0.lock(); - state.menus = Some(menus.into_iter().map(|menu| menu.owned()).collect()); - state.action_menus = action_menus; + self.0.lock().menus = Some(menus.into_iter().map(|menu| menu.owned()).collect()); } fn get_menus(&self) -> Option> { @@ -1513,12 +1465,6 @@ extern "C" fn handle_menu_item(this: &mut Object, _: Sel, item: id) { unsafe { let platform = get_mac_platform(this); let mut lock = platform.0.lock(); - - // If the menu item is currently being handled, i.e., this action is being triggered as a - // result of a keyboard shortcut causing the menu to flash, don't do anything. - if lock.handling_menu_item { - return; - } if let Some(mut callback) = lock.menu_command.take() { let tag: NSInteger = msg_send![item, tag]; let index = tag as usize; diff --git a/crates/gpui/src/platform/test/platform.rs b/crates/gpui/src/platform/test/platform.rs index 85b84248ff55f6a83d1e99e41d0029ed0b8ce5e7..15b909199fbd53b974e6a140f3223641dc0ac6ae 100644 --- a/crates/gpui/src/platform/test/platform.rs +++ b/crates/gpui/src/platform/test/platform.rs @@ -1,5 +1,5 @@ use crate::{ - Action, AnyWindowHandle, BackgroundExecutor, ClipboardItem, CursorStyle, DevicePixels, + AnyWindowHandle, BackgroundExecutor, ClipboardItem, CursorStyle, DevicePixels, DummyKeyboardMapper, ForegroundExecutor, Keymap, NoopTextSystem, Platform, PlatformDisplay, PlatformKeyboardLayout, PlatformKeyboardMapper, PlatformTextSystem, PromptButton, ScreenCaptureFrame, ScreenCaptureSource, ScreenCaptureStream, SourceMetadata, Task, @@ -378,8 +378,6 @@ impl Platform for TestPlatform { fn on_validate_app_menu_command(&self, _callback: Box bool>) {} - fn on_action_triggered(&self, _action: &dyn Action) {} - fn app_path(&self) -> Result { unimplemented!() } diff --git a/crates/gpui/src/platform/windows/platform.rs b/crates/gpui/src/platform/windows/platform.rs index 0ad343f2d3a590288f9b274f1456d88052850a4b..2eb1862f36a26592e18dc2e44875e08319361cc8 100644 --- a/crates/gpui/src/platform/windows/platform.rs +++ b/crates/gpui/src/platform/windows/platform.rs @@ -536,8 +536,6 @@ impl Platform for WindowsPlatform { .validate_app_menu_command = Some(callback); } - fn on_action_triggered(&self, _action: &dyn Action) {} - fn app_path(&self) -> Result { Ok(std::env::current_exe()?) } diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index e81751f61c38f2098bc00cc2f23b51b415c61555..d9bf27dca1253fa0a5286148ea64a03c3a5bac37 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -4023,8 +4023,6 @@ impl Window { let any_action = action.as_any(); if action_type == any_action.type_id() { cx.propagate_event = false; // Actions stop propagation by default during the bubble phase - cx.platform.on_action_triggered(action); - listener(any_action, DispatchPhase::Bubble, self, cx); if !cx.propagate_event { @@ -4042,7 +4040,6 @@ impl Window { for listener in global_listeners.iter().rev() { cx.propagate_event = false; // Actions stop propagation by default during the bubble phase - cx.platform.on_action_triggered(action); listener(action.as_any(), DispatchPhase::Bubble, cx); if !cx.propagate_event { break;