bug#44333: 27.1; macOS menu bar 2-clicks

[Alan Third <alan@idiocy.org>]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On Sun, Nov 01, 2020 at 05:28:43PM +0000, Alan Third wrote:
> On Sun, Nov 01, 2020 at 11:50:32AM +0100, Mattias Engdegård wrote:
> > > If anyone else has a simpler solution I'd love to hear it.
> > 
> > Maybe we are overthinking it. I don't think the Cocoa event loop
> > really is running in a different thread. Anecdotal evidence
> > (printf!) indicates that it isn't.
> 
> On the Mac port? It certainly isn't on the NS port... I don't think
> the Mac port even really uses the Cocoa event loop, although I could
> easily be wrong.
> 
> > (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32864#38 has a
> > previous investigation into the matter.)
> > 
> > I just did the simplest thing possible and it works (with a slight
> > latency at times, but perhaps tolerable). I'd like to know what's
> > wrong with running Lisp in the event loop. It does appear safe to
> > me.
> 
> We used to have a *lot* of crashes that were related to the event loop
> and lisp and so on, but if you're confident it isn't a problem feel
> free to propose a patch and we can give it a go on master.

OK, I've had a go at this and tried removing EVERYTHING related to the
delayed menu stuff and, as you say, it seems fine, but I barely ever
use the menus, so I could be missing something.

For the record, the menu code (or maybe it's the toolbar, but I
suspect the menus) kills GNUstep builds stone dead as soon as you try
typing anything. It looks from the debugger like Emacs is still
running, but it just won't update the screen. Turning off the menus
appears to fix it.
-- 
Alan Third

[-- Attachment #2: 0001-Remove-NS-menu-synthesized-events-bug-44333.patch --]
[-- Type: text/plain, Size: 9688 bytes --]

From 0e07430da3ad68bdc375aa1ac2688c2eadbbe88f Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Wed, 23 Dec 2020 20:12:02 +0000
Subject: [PATCH] Remove NS menu synthesized events (bug#44333)

* src/nsmenu.m (ns_update_menubar): Remove GNUstep special case that's
no longer needed.
(ns_activate_menubar):
([EmacsMenu trackingNotification:]):
([EmacsMenu menuWillOpen:]):
([EmacsMenu menuDidClose:]): Remove unused functions.
([EmacsMenu menuNeedsUpdate:]): Remove menu tracking code.
* src/nsterm.m (ns_check_menu_open):
(ns_check_pending_open_menu):
(ns_create_terminal): Remove unused functions.
(ns_term_init): Get rid of menu tracking.
---
 src/nsmenu.m |  78 ---------------------------------------
 src/nsterm.h |   2 -
 src/nsterm.m | 101 ---------------------------------------------------
 3 files changed, 181 deletions(-)

diff --git a/src/nsmenu.m b/src/nsmenu.m
index efad978316..d72f9c61be 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -58,10 +58,6 @@
 /* Nonzero means a menu is currently active.  */
 static int popup_activated_flag;
 
-/* Nonzero means we are tracking and updating menus.  */
-static int trackingMenu;
-
-
 /* NOTE: toolbar implementation is at end,
    following complete menu implementation.  */
 
@@ -141,10 +137,6 @@
   t = -(1000*tb.time+tb.millitm);
 #endif
 
-#ifdef NS_IMPL_GNUSTEP
-  deep_p = 1; /* until GNUstep NSMenu implements the Panther delegation model */
-#endif
-
   if (deep_p)
     {
       /* Fully parse one or more of the submenus.  */
@@ -463,17 +455,6 @@
   ns_update_menubar (f, deep_p, nil);
 }
 
-void
-ns_activate_menubar (struct frame *f)
-{
-#ifdef NS_IMPL_COCOA
-  ns_update_menubar (f, true, nil);
-  ns_check_pending_open_menu ();
-#endif
-}
-
-
-
 
 /* ==========================================================================
 
@@ -514,43 +495,6 @@ - (void)setFrame: (struct frame *)f
   frame = f;
 }
 
-#ifdef NS_IMPL_COCOA
--(void)trackingNotification:(NSNotification *)notification
-{
-  /* Update menu in menuNeedsUpdate only while tracking menus.  */
-  trackingMenu = ([notification name] == NSMenuDidBeginTrackingNotification
-                  ? 1 : 0);
-  if (! trackingMenu) ns_check_menu_open (nil);
-}
-
-- (void)menuWillOpen:(NSMenu *)menu
-{
-  ++trackingMenu;
-
-#if MAC_OS_X_VERSION_MIN_REQUIRED < 1070
-  // On 10.6 we get repeated calls, only the one for NSSystemDefined is "real".
-  if (
-#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
-      NSAppKitVersionNumber < NSAppKitVersionNumber10_7 &&
-#endif
-      [[NSApp currentEvent] type] != NSEventTypeSystemDefined)
-    return;
-#endif
-
-  /* When dragging from one menu to another, we get willOpen followed by didClose,
-     i.e. trackingMenu == 3 in willOpen and then 2 after didClose.
-     We have updated all menus, so avoid doing it when trackingMenu == 3.  */
-  if (trackingMenu == 2)
-    ns_check_menu_open (menu);
-}
-
-- (void)menuDidClose:(NSMenu *)menu
-{
-  --trackingMenu;
-}
-
-#endif /* NS_IMPL_COCOA */
-
 /* Delegate method called when a submenu is being opened: run a 'deep' call
    to set_frame_menubar.  */
 - (void)menuNeedsUpdate: (NSMenu *)menu
@@ -558,29 +502,7 @@ - (void)menuNeedsUpdate: (NSMenu *)menu
   if (!FRAME_LIVE_P (frame))
     return;
 
-  /* Cocoa/Carbon will request update on every keystroke
-     via IsMenuKeyEvent -> CheckMenusForKeyEvent.  These are not needed
-     since key equivalents are handled through emacs.
-     On Leopard, even keystroke events generate SystemDefined event.
-     Third-party applications that enhance mouse / trackpad
-     interaction, or also VNC/Remote Desktop will send events
-     of type AppDefined rather than SysDefined.
-     Menus will fail to show up if they haven't been initialized.
-     AppDefined events may lack timing data.
-
-     Thus, we rely on the didBeginTrackingNotification notification
-     as above to indicate the need for updates.
-     From 10.6 on, we could also use -[NSMenu propertiesToUpdate]: In the
-     key press case, NSMenuPropertyItemImage (e.g.) won't be set.
-  */
-  if (trackingMenu == 0)
-    return;
-/*fprintf (stderr, "Updating menu '%s'\n", [[self title] UTF8String]); NSLog (@"%@\n", event); */
-#ifdef NS_IMPL_GNUSTEP
-  /* Don't know how to do this for anything other than Mac OS X 10.5 and later.
-     This is wrong, as it might run Lisp code in the event loop.  */
   ns_update_menubar (frame, true, self);
-#endif
 }
 
 
diff --git a/src/nsterm.h b/src/nsterm.h
index 94472ec107..0b7e27c00f 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -1130,8 +1130,6 @@ ns_query_color (void *col, Emacs_Color *color_def, bool setPixel);
 extern NSColor *ns_lookup_indexed_color (unsigned long idx, struct frame *f);
 extern unsigned long ns_index_color (NSColor *color, struct frame *f);
 extern const char *ns_get_pending_menu_title (void);
-extern void ns_check_menu_open (NSMenu *menu);
-extern void ns_check_pending_open_menu (void);
 #endif
 
 /* Implemented in nsfns, published in nsterm.  */
diff --git a/src/nsterm.m b/src/nsterm.m
index 2a117a0780..161677484f 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -310,24 +310,6 @@ - (NSColor *)colorUsingDefaultColorSpace
   NULL, 0, 0
 };
 
-#ifdef NS_IMPL_COCOA
-/*
- * State for pending menu activation:
- * MENU_NONE     Normal state
- * MENU_PENDING  A menu has been clicked on, but has been canceled so we can
- *               run lisp to update the menu.
- * MENU_OPENING  Menu is up to date, and the click event is redone so the menu
- *               will open.
- */
-#define MENU_NONE 0
-#define MENU_PENDING 1
-#define MENU_OPENING 2
-static int menu_will_open_state = MENU_NONE;
-
-/* Saved position for menu click.  */
-static CGPoint menu_mouse_point;
-#endif
-
 /* Convert modifiers in a NeXTstep event to emacs style modifiers.  */
 #define NS_FUNCTION_KEY_MASK 0x800000
 #define NSLeftControlKeyMask    (0x000001 | NSEventModifierFlagControl)
@@ -4607,79 +4589,6 @@ in certain situations (rapid incoming events).
 }
 #endif
 
-/* GNUstep does not have cancelTracking.  */
-#ifdef NS_IMPL_COCOA
-/* Check if menu open should be canceled or continued as normal.  */
-void
-ns_check_menu_open (NSMenu *menu)
-{
-  /* Click in menu bar?  */
-  NSArray *a = [[NSApp mainMenu] itemArray];
-  int i;
-  BOOL found = NO;
-
-  if (menu == nil) // Menu tracking ended.
-    {
-      if (menu_will_open_state == MENU_OPENING)
-        menu_will_open_state = MENU_NONE;
-      return;
-    }
-
-  for (i = 0; ! found && i < [a count]; i++)
-    found = menu == [[a objectAtIndex:i] submenu];
-  if (found)
-    {
-      if (menu_will_open_state == MENU_NONE && emacs_event)
-        {
-          NSEvent *theEvent = [NSApp currentEvent];
-          struct frame *emacsframe = SELECTED_FRAME ();
-
-          /* On macOS, the following can cause an event loop when the
-             Spotlight for Help search field is populated.  Avoid this by
-             not postponing mouse drag and non-user-generated mouse down
-             events (Bug#31371).  */
-          if (([theEvent type] == NSEventTypeLeftMouseDown)
-              && [theEvent eventNumber])
-            {
-              [menu cancelTracking];
-              menu_will_open_state = MENU_PENDING;
-              emacs_event->kind = MENU_BAR_ACTIVATE_EVENT;
-              EV_TRAILER (theEvent);
-
-              CGEventRef ourEvent = CGEventCreate (NULL);
-              menu_mouse_point = CGEventGetLocation (ourEvent);
-              CFRelease (ourEvent);
-            }
-        }
-      else if (menu_will_open_state == MENU_OPENING)
-        {
-          menu_will_open_state = MENU_NONE;
-        }
-    }
-}
-
-/* Redo saved menu click if state is MENU_PENDING.  */
-void
-ns_check_pending_open_menu ()
-{
-  if (menu_will_open_state == MENU_PENDING)
-    {
-      CGEventSourceRef source
-        = CGEventSourceCreate (kCGEventSourceStateHIDSystemState);
-
-      CGEventRef event = CGEventCreateMouseEvent (source,
-                                                  kCGEventLeftMouseDown,
-                                                  menu_mouse_point,
-                                                  kCGMouseButtonLeft);
-      CGEventSetType (event, kCGEventLeftMouseDown);
-      CGEventPost (kCGHIDEventTap, event);
-      CFRelease (event);
-      CFRelease (source);
-
-      menu_will_open_state = MENU_OPENING;
-    }
-}
-#endif /* NS_IMPL_COCOA */
 
 static int
 ns_read_socket (struct terminal *terminal, struct input_event *hold_quit)
@@ -5416,7 +5325,6 @@ static Lisp_Object ns_new_font (struct frame *f, Lisp_Object font_object,
   terminal->set_new_font_hook = ns_new_font;
   terminal->implicit_set_name_hook = ns_implicitly_set_name;
   terminal->menu_show_hook = ns_menu_show;
-  terminal->activate_menubar_hook = ns_activate_menubar;
   terminal->popup_dialog_hook = ns_popup_dialog;
   terminal->set_vertical_scroll_bar_hook = ns_set_vertical_scroll_bar;
   terminal->set_horizontal_scroll_bar_hook = ns_set_horizontal_scroll_bar;
@@ -5661,15 +5569,6 @@ Needs to be here because ns_initialize_display_info () uses AppKit classes.
     [NSApp setServicesMenu: svcsMenu];
     /* Needed at least on Cocoa, to get dock menu to show windows */
     [NSApp setWindowsMenu: [[NSMenu alloc] init]];
-
-    [[NSNotificationCenter defaultCenter]
-      addObserver: mainMenu
-         selector: @selector (trackingNotification:)
-             name: NSMenuDidBeginTrackingNotification object: mainMenu];
-    [[NSNotificationCenter defaultCenter]
-      addObserver: mainMenu
-         selector: @selector (trackingNotification:)
-             name: NSMenuDidEndTrackingNotification object: mainMenu];
   }
 #endif /* macOS menu setup */
 
-- 
2.29.2