Revert "gpui: Flash menu in menubar on macOS when action is triggered (#38588)" (#38880)

Lukas Wirth created

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

Change summary

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(-)

Detailed changes

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<Self>) -> 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) {

crates/gpui/src/platform.rs 🔗

@@ -250,7 +250,6 @@ pub(crate) trait Platform: 'static {
     fn on_app_menu_action(&self, callback: Box<dyn FnMut(&dyn Action)>);
     fn on_will_open_app_menu(&self, callback: Box<dyn FnMut()>);
     fn on_validate_app_menu_command(&self, callback: Box<dyn FnMut(&dyn Action) -> bool>);
-    fn on_action_triggered(&self, action: &dyn Action);
 
     fn compositor_name(&self) -> &'static str {
         ""

crates/gpui/src/platform/linux/platform.rs 🔗

@@ -442,8 +442,6 @@ impl<P: LinuxClient + 'static> Platform for P {
         });
     }
 
-    fn on_action_triggered(&self, _action: &dyn Action) {}
-
     fn app_path(&self) -> Result<PathBuf> {
         // get the path of the executable of the current process
         let app_path = env::current_exe()?;

crates/gpui/src/platform/mac/platform.rs 🔗

@@ -177,8 +177,6 @@ pub(crate) struct MacPlatformState {
     dock_menu: Option<id>,
     menus: Option<Vec<OwnedMenu>>,
     keyboard_mapper: Rc<MacKeyboardMapper>,
-    action_menus: Vec<(Box<dyn Action>, 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<Menu>,
         delegate: id,
         actions: &mut Vec<Box<dyn Action>>,
-        action_menus: &mut Vec<(Box<dyn Action>, 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<Box<dyn Action>>,
-        action_menus: &mut Vec<(Box<dyn Action>, id)>,
         keymap: &Keymap,
     ) -> id {
         static DEFAULT_CONTEXT: OnceLock<Vec<KeyContext>> = 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<dyn PlatformKeyboardLayout> {
         Box::new(MacKeyboardLayout::new())
     }
@@ -944,24 +905,15 @@ impl Platform for MacPlatform {
     }
 
     fn set_menus(&self, menus: Vec<Menu>, 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<Vec<OwnedMenu>> {
@@ -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;

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<dyn FnMut(&dyn crate::Action) -> bool>) {}
 
-    fn on_action_triggered(&self, _action: &dyn Action) {}
-
     fn app_path(&self) -> Result<std::path::PathBuf> {
         unimplemented!()
     }

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<PathBuf> {
         Ok(std::env::current_exe()?)
     }

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;