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

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

[Viktor Kharitonovich]

Hello,

On macOS 10.15.7 menu bar behave like a toggle switch. First click is
ignored. Then second click opens menu and everything works as expected. But
another next click on menu bar (not on an item) leads to hiding menu bar
and then again two clicks are needed for open.


In GNU Emacs 27.1 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G95))
of 2020-08-12 built on builder10-14.porkrind.org
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.7

Recent messages:
uncompressing ido.el.gz...done
Searched 0/1 files
Searched 1/1 files
Quit
Mark set
current buffer is now: *unsent posting*
Making completion list... [2 times]
Quit
funcall-interactively: Buffer is read-only: #<buffer *5x5*>
Making completion list...

Configured using:
'configure --with-ns '--enable-locallisppath=/Library/Application
Support/Emacs/${version}/site-lisp:/Library/Application
Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES
THREADS JSON PDUMPER

Important settings:
  value of $LANG: en_BY.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  display-line-numbers-mode: t
  shell-dirtrack-mode: t
  recentf-mode: t
  ido-everywhere: t
  projectile-mode: t
  company-tng-mode: t
  global-company-mode: t
  company-mode: t
  override-global-mode: t
  delete-selection-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/Users/hrls/.emacs.d/elpa/xref-1.0.3/xref hides /Applications/Emacs.app/Contents/Resources/lisp/progmodes/xref
/Users/hrls/.emacs.d/elpa/project-0.5.2/project hides /Applications/Emacs.app/Contents/Resources/lisp/progmodes/project
/Users/hrls/.emacs.d/elpa/flymake-1.0.9/flymake hides /Applications/Emacs.app/Contents/Resources/lisp/progmodes/flymake
/Users/hrls/.emacs.d/elpa/eldoc-1.11.0/eldoc hides /Applications/Emacs.app/Contents/Resources/lisp/emacs-lisp/eldoc

Features:
(shadow sort emacsbug sendmail 5x5 mail-extr ielm cl-print debug
display-line-numbers vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs vc-dir
ewoc vc vc-dispatcher haskell-doc inf-haskell haskell-decl-scan shell
pcomplete haskell haskell-completions haskell-load haskell-commands
highlight-uses-mode haskell-modules haskell-sandbox
haskell-navigate-imports haskell-repl haskell-svg haskell-collapse
hideshow haskell-debug haskell-interactive-mode
haskell-presentation-mode haskell-compile haskell-hoogle haskell-process
haskell-session haskell-mode haskell-cabal haskell-utils
haskell-font-lock haskell-indentation haskell-string
haskell-sort-imports haskell-lexeme haskell-align-imports
haskell-complete-module haskell-ghc-support flymake-proc flymake
warnings dabbrev haskell-customize deeper-blue-theme dichromacy-theme
leuven-theme light-blue-theme manoj-dark-theme misterioso-theme
tango-dark-theme tango-theme tsdh-dark-theme tsdh-light-theme
wheatgrass-theme whiteboard-theme tango-plus-theme recentf tree-widget
flatland-theme cus-theme autoload cus-edit cus-start cus-load wid-edit
lisp-mnt mm-archive message format-spec rfc822 mml mml-sec epa derived
epg gnus-util rmail rmail-loaddefs text-property-search mailabbrev
gmm-utils mailheader mm-decode mm-bodies mm-encode mail-utils misearch
multi-isearch gnutls network-stream url-http mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr url-gw nsm rmc puny url-cache
url-auth url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf url-util mailcap epg-config finder-inf dired-aux
dired-x vc-git diff-mode cargo cargo-process markdown-mode color
noutline outline flycheck time-date pp jka-compr company-oddmuse
company-keywords company-etags etags fileloop generator xref project
company-gtags company-dabbrev-code company-dabbrev company-files
company-clang company-capf company-cmake company-semantic
company-template company-bbdb omnitab my-packages helpful imenu trace
edebug backtrace info-look advice find-func f dash-functional help-fns
radix-tree elisp-refs s dash ido hydra lv projectile grep compile comint
ansi-color ibuf-ext rust-mode rx thingatpt company-tng company pcase
ace-window avy ring exec-path-from-shell diminish delight cl-extra
help-mode use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core edmacro kmacro wm suwayyah server wombat-theme dired
dired-loaddefs ibuffer ibuffer-loaddefs delsel paren info package
easymenu browse-url url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache json subr-x map
url-vars seq byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/ns-win ns-win ucs-normalize mule-util term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite charscript charprop case-table epa-hook jka-cmpr-hook help
simple abbrev obarray cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
threads kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 417887 486947)
(symbols 48 34423 1)
(strings 32 142378 89905)
(string-bytes 1 3608995)
(vectors 16 44990)
(vector-slots 8 604398 45426)
(floats 8 351 405)
(intervals 56 2562 282)
(buffers 1000 43))

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

[Alan Third]

On Fri, Oct 30, 2020 at 08:53:27PM +0300, Viktor Kharitonovich wrote:
> Hello,
> 
> On macOS 10.15.7 menu bar behave like a toggle switch. First click is
> ignored. Then second click opens menu and everything works as expected. But
> another next click on menu bar (not on an item) leads to hiding menu bar
> and then again two clicks are needed for open.
> 
> 
> In GNU Emacs 27.1 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G95))
> of 2020-08-12 built on builder10-14.porkrind.org
> Windowing system distributor 'Apple', version 10.3.1894
> System Description:  Mac OS X 10.15.7

This looks like an emacsformacosx.com build.

IIRC you need to add Ruby to the accessibility permissions group in
the privacy settings.

I'm a little hazy on what needs done here, but if someone who
understands this could write it up for PROBLEMS I'd appreciate it.
-- 
Alan Third

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

[Mattias Engdegård]

> IIRC you need to add Ruby to the accessibility permissions group in
> the privacy settings.

What are the privacy/security implications of doing so?

Since the Mac port reportedly doesn't suffer from this problem, could we learn from it and do the same?

(There should be a previous bug report.)

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

[Alan Third]

On Sat, Oct 31, 2020 at 03:08:14PM +0100, Mattias Engdegård wrote:
> > IIRC you need to add Ruby to the accessibility permissions group in
> > the privacy settings.
> 
> What are the privacy/security implications of doing so?

I have no idea.

> Since the Mac port reportedly doesn't suffer from this problem,
> could we learn from it and do the same?

IIRC the problem is due to the reposting of the menu click event so
the menu can be populated when lisp is running. The Mac port doesn't
have this problem most probably because it's a completely different
architecture but it has the GUI and lisp parts split into two separate
threads which is one way I can see of fixing this.

I'm not keen on doing the complete overhaul required because I'll
likely introduce more bugs than I'll fix, and I'm pretty sure my next
computer isn't going to be a Mac. Introducing lots of bugs then
running doesn't seem like a good approach.

If anyone else has a simpler solution I'd love to hear it.
-- 
Alan Third

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

[Mattias Engdegård]

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

31 okt. 2020 kl. 16.01 skrev Alan Third <alan@idiocy.org>:

> IIRC the problem is due to the reposting of the menu click event so
> the menu can be populated when lisp is running. The Mac port doesn't
> have this problem most probably because it's a completely different
> architecture but it has the GUI and lisp parts split into two separate
> threads which is one way I can see of fixing this.

Last time I looked it seemed that the Mac port actually did synthesise events but used some other means (Carbon?), but I may be wrong.


> 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.

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


[-- Attachment #2: macos-1-click-menu-bar.diff --]
[-- Type: application/octet-stream, Size: 1595 bytes --]

diff --git a/src/nsmenu.m b/src/nsmenu.m
index a286a80da1..4a99df31fb 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -466,7 +466,7 @@
 void
 ns_activate_menubar (struct frame *f)
 {
-#ifdef NS_IMPL_COCOA
+#if 0 && defined NS_IMPL_COCOA
   ns_update_menubar (f, true, nil);
   ns_check_pending_open_menu ();
 #endif
@@ -514,7 +514,7 @@ - (void)setFrame: (struct frame *)f
   frame = f;
 }
 
-#ifdef NS_IMPL_COCOA
+#if 0 && defined NS_IMPL_COCOA
 -(void)trackingNotification:(NSNotification *)notification
 {
   /* Update menu in menuNeedsUpdate only while tracking menus.  */
@@ -573,10 +573,12 @@ - (void)menuNeedsUpdate: (NSMenu *)menu
      From 10.6 on, we could also use -[NSMenu propertiesToUpdate]: In the
      key press case, NSMenuPropertyItemImage (e.g.) won't be set.
   */
+#if 0
   if (trackingMenu == 0)
     return;
+#endif
 /*fprintf (stderr, "Updating menu '%s'\n", [[self title] UTF8String]); NSLog (@"%@\n", event); */
-#ifdef NS_IMPL_GNUSTEP
+#if 1 || defined 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);
diff --git a/src/nsterm.m b/src/nsterm.m
index fa38350a2f..8f5658640f 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -4607,7 +4607,7 @@ in certain situations (rapid incoming events).
 #endif
 
 /* GNUstep does not have cancelTracking.  */
-#ifdef NS_IMPL_COCOA
+#if 0 && defined NS_IMPL_COCOA
 /* Check if menu open should be canceled or continued as normal.  */
 void
 ns_check_menu_open (NSMenu *menu)

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

[Alan Third]

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.

-- 
Alan Third

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

[Alan Third]

[-- 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

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

[Mattias Engdegård]

23 dec. 2020 kl. 21.33 skrev Alan Third <alan@idiocy.org>:

> 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.

Thank you, I tried exactly the same thing. While it works, there are annoying delays in dropping down the menus -- sometimes they come down immediately, but more often than not I get a delay of 100-300 ms. Do you experience them too?

In any case, it's a lot better than the previous double-clutch menus. I rarely use the menus either but partly because they were so annoying; I find them useful for discovering functionality and keys in packages. Getting rid of the delays would be nice, though.

> 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.

Maybe we could make the change conditional on GNUstep?

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

[Alan Third]

On Fri, Dec 25, 2020 at 05:06:15PM +0100, Mattias Engdegård wrote:
> 23 dec. 2020 kl. 21.33 skrev Alan Third <alan@idiocy.org>:
> 
> > 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.
> 
> Thank you, I tried exactly the same thing. While it works, there are
> annoying delays in dropping down the menus -- sometimes they come
> down immediately, but more often than not I get a delay of 100-300
> ms. Do you experience them too?
> 
> In any case, it's a lot better than the previous double-clutch
> menus. I rarely use the menus either but partly because they were so
> annoying; I find them useful for discovering functionality and keys
> in packages. Getting rid of the delays would be nice, though.

I don't see how the delays can be new as the menus would have to be
recalculated under the old code anyway...

It is quite noticeable, though...

I notice this in the Apple docs[1]:

    If populating the menu will take a long time, implement
    numberOfItemsInMenu: and menu:updateItem:atIndex:shouldCancel:
    instead.

I wonder if that's possible for us. ns_update_menubar is a big,
complicated function that I've not been over in depth to work out what
it's doing, so I don't know if it would be practical to break it up
like I suspect using those two methods would require.

> > 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.
> 
> Maybe we could make the change conditional on GNUstep?

Sorry, I was unclear. This is an older problem. I'm not sure when it
was introduced.

[1] https://developer.apple.com/documentation/appkit/nsmenudelegate/1518235-menuneedsupdate?language=objc
-- 
Alan Third

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

[Alan Third]

On Fri, Dec 25, 2020 at 05:26:49PM +0000, Alan Third wrote:
> ns_update_menubar is a big, complicated function that I've not been
> over in depth to work out what it's doing, so I don't know if it
> would be practical to break it up like I suspect using those two
> methods would require.

I've had a look through it, and I've noticed that most of the work
appears to be building a tree of widget_value structs. A quick look at
xmenu.c and w32menu.c leaves me with the impression that they have
almost identical code for that part, but for some reason the NS port
is different (and is full of "FIXME: this is broken" comments).

So I suppose the first thing to do should be to align the NS code with
the other terminals, and see if that improves things.
-- 
Alan Third

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

[Mattias Engdegård]

25 dec. 2020 kl. 20.20 skrev Alan Third <alan@idiocy.org>:
> 
> On Fri, Dec 25, 2020 at 05:26:49PM +0000, Alan Third wrote:
>> ns_update_menubar is a big, complicated function that I've not been
>> over in depth to work out what it's doing, so I don't know if it
>> would be practical to break it up like I suspect using those two
>> methods would require.
> 
> I've had a look through it, and I've noticed that most of the work
> appears to be building a tree of widget_value structs. A quick look at
> xmenu.c and w32menu.c leaves me with the impression that they have
> almost identical code for that part, but for some reason the NS port
> is different (and is full of "FIXME: this is broken" comments).
> 
> So I suppose the first thing to do should be to align the NS code with
> the other terminals, and see if that improves things.

Probably. When the code suffers from one of its slow spells, the time is apparently spent in the calls to parse_single_submenu, and from there... single_keymap_panes, I think. Didn't dig deeper after that.

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

[Alan Third]

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

On Fri, Dec 25, 2020 at 11:28:56PM +0100, Mattias Engdegård wrote:
> 25 dec. 2020 kl. 20.20 skrev Alan Third <alan@idiocy.org>:
> > 
> > On Fri, Dec 25, 2020 at 05:26:49PM +0000, Alan Third wrote:
> >> ns_update_menubar is a big, complicated function that I've not been
> >> over in depth to work out what it's doing, so I don't know if it
> >> would be practical to break it up like I suspect using those two
> >> methods would require.
> > 
> > I've had a look through it, and I've noticed that most of the work
> > appears to be building a tree of widget_value structs. A quick look at
> > xmenu.c and w32menu.c leaves me with the impression that they have
> > almost identical code for that part, but for some reason the NS port
> > is different (and is full of "FIXME: this is broken" comments).
> > 
> > So I suppose the first thing to do should be to align the NS code with
> > the other terminals, and see if that improves things.
> 
> Probably. When the code suffers from one of its slow spells, the
> time is apparently spent in the calls to parse_single_submenu, and
> from there... single_keymap_panes, I think. Didn't dig deeper after
> that.

Does the attached seem any better? It appears to me like the first
click on a menu is slightly faster (but that may be my imagination),
and subsequent menus are instant.
-- 
Alan Third

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

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

* src/nsmenu.m (ns_update_menubar): Copy the parsing code from xmenu.c
and rework the NS specific code around it.
(ns_activate_menubar):
([EmacsMenu trackingNotification:]):
([EmacsMenu menuWillOpen:]):
([EmacsMenu menuDidClose:]): Remove unused functions.
([EmacsMenu menuNeedsUpdate:]): Remove menu tracking code and add code
to check whether an update is required.
(syms_of_nsmenu): Remove 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 | 377 ++++++++++++++++-----------------------------------
 src/nsterm.h |   2 -
 src/nsterm.m | 101 --------------
 3 files changed, 115 insertions(+), 365 deletions(-)

diff --git a/src/nsmenu.m b/src/nsmenu.m
index efad978316..b5d0821323 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -58,9 +58,7 @@
 /* Nonzero means a menu is currently active.  */
 static int popup_activated_flag;
 
-/* Nonzero means we are tracking and updating menus.  */
-static int trackingMenu;
-
+static BOOL needs_deep_update = YES;
 
 /* NOTE: toolbar implementation is at end,
    following complete menu implementation.  */
@@ -98,16 +96,18 @@
     3) deep_p, submenu = non-nil: Update contents of a single submenu.
    -------------------------------------------------------------------------- */
 static void
-ns_update_menubar (struct frame *f, bool deep_p, EmacsMenu *submenu)
+ns_update_menubar (struct frame *f, bool deep_p)
 {
   NSAutoreleasePool *pool;
-  id menu = [NSApp mainMenu];
-  static EmacsMenu *last_submenu = nil;
   BOOL needsSet = NO;
-  bool owfi;
+  id menu = [NSApp mainMenu];
+
   Lisp_Object items;
   widget_value *wv, *first_wv, *prev_wv = 0;
   int i;
+  int *submenu_start, *submenu_end;
+  bool *submenu_top_level_items;
+  int *submenu_n_panes;
 
 #if NSMENUPROFILE
   struct timeb tb;
@@ -141,115 +141,94 @@
   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.  */
-      int n = 0;
-      int *submenu_start, *submenu_end;
-      bool *submenu_top_level_items;
-      int *submenu_n_panes;
+      /* Make a widget-value tree representing the entire menu trees.  */
+
       struct buffer *prev = current_buffer;
       Lisp_Object buffer;
       ptrdiff_t specpdl_count = SPECPDL_INDEX ();
       int previous_menu_items_used = f->menu_bar_items_used;
       Lisp_Object *previous_items
 	= alloca (previous_menu_items_used * sizeof *previous_items);
+      int subitems;
 
-      /* lisp preliminaries */
       buffer = XWINDOW (FRAME_SELECTED_WINDOW (f))->contents;
       specbind (Qinhibit_quit, Qt);
+      /* Don't let the debugger step into this code
+	 because it is not reentrant.  */
       specbind (Qdebug_on_next_call, Qnil);
+
       record_unwind_save_match_data ();
       if (NILP (Voverriding_local_map_menu_flag))
 	{
 	  specbind (Qoverriding_terminal_local_map, Qnil);
 	  specbind (Qoverriding_local_map, Qnil);
 	}
-      set_buffer_internal_1 (XBUFFER (buffer));
 
-      /* TODO: for some reason this is not needed in other terms,
-	 but some menu updates call Info-extract-pointer which causes
-	 abort-on-error if waiting-for-input.  Needs further investigation.  */
-      owfi = waiting_for_input;
-      waiting_for_input = 0;
+      set_buffer_internal_1 (XBUFFER (buffer));
 
-      /* lucid hook and possible reset */
+      /* Run the Lucid hook.  */
       safe_run_hooks (Qactivate_menubar_hook);
+
+      /* If it has changed current-menubar from previous value,
+	 really recompute the menubar from the value.  */
       if (! NILP (Vlucid_menu_bar_dirty_flag))
 	call0 (Qrecompute_lucid_menubar);
       safe_run_hooks (Qmenu_bar_update_hook);
       fset_menu_bar_items (f, menu_bar_items (FRAME_MENU_BAR_ITEMS (f)));
 
-      /* Now ready to go */
       items = FRAME_MENU_BAR_ITEMS (f);
 
-      /* Save the frame's previous menu bar contents data */
+      /* Save the frame's previous menu bar contents data.  */
       if (previous_menu_items_used)
-	memcpy (previous_items, aref_addr (f->menu_bar_vector, 0),
-		previous_menu_items_used * sizeof (Lisp_Object));
+	memcpy (previous_items, xvector_contents (f->menu_bar_vector),
+		previous_menu_items_used * word_size);
 
-      /* parse stage 1: extract from lisp */
+      /* Fill in menu_items with the current menu bar contents.
+	 This can evaluate Lisp code.  */
       save_menu_items ();
 
       menu_items = f->menu_bar_vector;
       menu_items_allocated = VECTORP (menu_items) ? ASIZE (menu_items) : 0;
-      submenu_start = alloca (ASIZE (items) * sizeof *submenu_start);
-      submenu_end = alloca (ASIZE (items) * sizeof *submenu_end);
-      submenu_n_panes = alloca (ASIZE (items) * sizeof *submenu_n_panes);
-      submenu_top_level_items = alloca (ASIZE (items)
+      subitems = ASIZE (items) / 4;
+      submenu_start = alloca ((subitems + 1) * sizeof *submenu_start);
+      submenu_end = alloca (subitems * sizeof *submenu_end);
+      submenu_n_panes = alloca (subitems * sizeof *submenu_n_panes);
+      submenu_top_level_items = alloca (subitems
 					* sizeof *submenu_top_level_items);
       init_menu_items ();
-      for (i = 0; i < ASIZE (items); i += 4)
+      for (i = 0; i < subitems; i++)
 	{
 	  Lisp_Object key, string, maps;
 
-	  key = AREF (items, i);
-	  string = AREF (items, i + 1);
-	  maps = AREF (items, i + 2);
+	  key = AREF (items, 4 * i);
+	  string = AREF (items, 4 * i + 1);
+	  maps = AREF (items, 4 * i + 2);
 	  if (NILP (string))
 	    break;
 
-          /* FIXME: we'd like to only parse the needed submenu, but this
-	     was causing crashes in the _common parsing code: need to make
-	     sure proper initialization done.  */
-	  /* if (submenu && strcmp ([[submenu title] UTF8String], SSDATA (string)))
-               continue; */
-
 	  submenu_start[i] = menu_items_used;
 
 	  menu_items_n_panes = 0;
-	  submenu_top_level_items[i] = parse_single_submenu (key, string, maps);
+	  submenu_top_level_items[i]
+	    = parse_single_submenu (key, string, maps);
 	  submenu_n_panes[i] = menu_items_n_panes;
+
 	  submenu_end[i] = menu_items_used;
-          n++;
 	}
 
+      submenu_start[i] = -1;
       finish_menu_items ();
-      waiting_for_input = owfi;
-
 
-      if (submenu && n == 0)
-        {
-          /* should have found a menu for this one but didn't */
-          fprintf (stderr, "ERROR: did not find lisp menu for submenu '%s'.\n",
-                  [[submenu title] UTF8String]);
-	  discard_menu_items ();
-	  unbind_to (specpdl_count, Qnil);
-          unblock_input ();
-	  return;
-        }
+      /* Convert menu_items into widget_value trees
+	 to display the menu.  This cannot evaluate Lisp code.  */
 
-      /* parse stage 2: insert into lucid 'widget_value' structures
-         [comments in other terms say not to evaluate lisp code here] */
       wv = make_widget_value ("menubar", NULL, true, Qnil);
       wv->button_type = BUTTON_TYPE_NONE;
       first_wv = wv;
 
-      for (i = 0; i < 4*n; i += 4)
+      for (i = 0; submenu_start[i] >= 0; i++)
 	{
 	  menu_items_n_panes = submenu_n_panes[i];
 	  wv = digest_single_submenu (submenu_start[i], submenu_end[i],
@@ -259,158 +238,115 @@
 	  else
 	    first_wv->contents = wv;
 	  /* Don't set wv->name here; GC during the loop might relocate it.  */
-	  wv->enabled = 1;
+	  wv->enabled = true;
 	  wv->button_type = BUTTON_TYPE_NONE;
 	  prev_wv = wv;
 	}
 
       set_buffer_internal_1 (prev);
 
-      /* Compare the new menu items with previous, and leave off if no change.  */
-      /* FIXME: following other terms here, but seems like this should be
-	 done before parse stage 2 above, since its results aren't used.  */
-      if (previous_menu_items_used
-          && (!submenu || (submenu && submenu == last_submenu))
-          && menu_items_used == previous_menu_items_used)
-        {
-          for (i = 0; i < previous_menu_items_used; i++)
-            /* FIXME: this ALWAYS fails on Buffers menu items.. something
-	       about their strings causes them to change every time, so we
-	       double-check failures.  */
-            if (!EQ (previous_items[i], AREF (menu_items, i)))
-              if (!(STRINGP (previous_items[i])
-                    && STRINGP (AREF (menu_items, i))
-                    && !strcmp (SSDATA (previous_items[i]),
-				SSDATA (AREF (menu_items, i)))))
-                  break;
-          if (i == previous_menu_items_used)
-            {
-              /* No change.  */
+      /* If there has been no change in the Lisp-level contents
+	 of the menu bar, skip redisplaying it.  Just exit.  */
 
-#if NSMENUPROFILE
-              ftime (&tb);
-              t += 1000*tb.time+tb.millitm;
-              fprintf (stderr, "NO CHANGE!  CUTTING OUT after %ld msec.\n", t);
-#endif
+      /* Compare the new menu items with the ones computed last time.  */
+      for (i = 0; i < previous_menu_items_used; i++)
+	if (menu_items_used == i
+	    || (!EQ (previous_items[i], AREF (menu_items, i))))
+	  break;
+      if (i == menu_items_used && i == previous_menu_items_used && i != 0)
+	{
+	  /* The menu items have not changed.  Don't bother updating
+	     the menus in any form, since it would be a no-op.  */
+	  free_menubar_widget_value_tree (first_wv);
+	  discard_menu_items ();
+	  unbind_to (specpdl_count, Qnil);
+	  return;
+	}
 
-              free_menubar_widget_value_tree (first_wv);
-              discard_menu_items ();
-              unbind_to (specpdl_count, Qnil);
-              unblock_input ();
-              return;
-            }
-        }
       /* The menu items are different, so store them in the frame.  */
-      /* FIXME: this is not correct for single-submenu case.  */
       fset_menu_bar_vector (f, menu_items);
       f->menu_bar_items_used = menu_items_used;
 
-      /* Calls restore_menu_items, etc., as they were outside.  */
+      /* This undoes save_menu_items.  */
       unbind_to (specpdl_count, Qnil);
 
-      /* Parse stage 2a: now GC cannot happen during the lifetime of the
-         widget_value, so it's safe to store data from a Lisp_String.  */
+      /* Now GC cannot happen during the lifetime of the widget_value,
+	 so it's safe to store data from a Lisp_String.  */
       wv = first_wv->contents;
       for (i = 0; i < ASIZE (items); i += 4)
 	{
 	  Lisp_Object string;
 	  string = AREF (items, i + 1);
 	  if (NILP (string))
-	    break;
-
-	  wv->name = SSDATA (string);
+            break;
+          wv->name = SSDATA (string);
           update_submenu_strings (wv->contents);
-	  wv = wv->next;
+          wv = wv->next;
 	}
 
-      /* Now, update the NS menu; if we have a submenu, use that, otherwise
-         create a new menu for each sub and fill it.  */
-      if (submenu)
-        {
-          const char *submenuTitle = [[submenu title] UTF8String];
-          for (wv = first_wv->contents; wv; wv = wv->next)
-            {
-              if (!strcmp (submenuTitle, wv->name))
-                {
-                  [submenu fillWithWidgetValue: wv->contents];
-                  last_submenu = submenu;
-                  break;
-                }
-            }
-        }
-      else
-        {
-          [menu fillWithWidgetValue: first_wv->contents frame: f];
-        }
-
     }
   else
     {
-      static int n_previous_strings = 0;
-      static char previous_strings[100][10];
-      static struct frame *last_f = NULL;
-      int n;
-      Lisp_Object string;
+      /* Make a widget-value tree containing
+	 just the top level menu bar strings.  */
 
       wv = make_widget_value ("menubar", NULL, true, Qnil);
       wv->button_type = BUTTON_TYPE_NONE;
       first_wv = wv;
 
-      /* Make widget-value tree with just the top level menu bar strings.  */
       items = FRAME_MENU_BAR_ITEMS (f);
-      if (NILP (items))
-        {
-          free_menubar_widget_value_tree (first_wv);
-          unblock_input ();
-          return;
-        }
-
-
-      /* Check if no change: this mechanism is a bit rough, but ready.  */
-      n = ASIZE (items) / 4;
-      if (f == last_f && n_previous_strings == n)
-        {
-          for (i = 0; i<n; i++)
-            {
-	      string = AREF (items, 4*i+1);
-
-              if (EQ (string, make_fixnum (0))) // FIXME: Why???  --Stef
-                continue;
-              if (NILP (string))
-                {
-                  if (previous_strings[i][0])
-                    break;
-                  else
-                    continue;
-                }
-              else if (memcmp (previous_strings[i], SDATA (string),
-			  min (10, SBYTES (string) + 1)))
-                break;
-            }
-
-          if (i == n)
-            {
-              free_menubar_widget_value_tree (first_wv);
-              unblock_input ();
-              return;
-            }
-        }
-
-      [menu clear];
       for (i = 0; i < ASIZE (items); i += 4)
 	{
+	  Lisp_Object string;
+
 	  string = AREF (items, i + 1);
 	  if (NILP (string))
 	    break;
 
-          if (n < 100)
-	    memcpy (previous_strings[i/4], SDATA (string),
-                    min (10, SBYTES (string) + 1));
-
 	  wv = make_widget_value (SSDATA (string), NULL, true, Qnil);
 	  wv->button_type = BUTTON_TYPE_NONE;
+	  /* This prevents lwlib from assuming this
+	     menu item is really supposed to be empty.  */
+	  /* The intptr_t cast avoids a warning.
+	     This value just has to be different from small integers.  */
 	  wv->call_data = (void *) (intptr_t) (-1);
 
+	  if (prev_wv)
+	    prev_wv->next = wv;
+	  else
+	    first_wv->contents = wv;
+	  prev_wv = wv;
+	}
+
+      /* Forget what we thought we knew about what is in the
+	 detailed contents of the menu bar menus.
+	 Changing the top level always destroys the contents.  */
+      f->menu_bar_items_used = 0;
+    }
+
+  /* Now, update the NS menu.  */
+  if (deep_p)
+    {
+      /* This path is typically used when a menu has been clicked.  I
+         think Apple expect us to only update that one menu, however
+         to update one we need to do the hard work of parsing the
+         whole tree, so we may as well update them all.  */
+      int i = 1;
+
+      for (wv = first_wv->contents; wv; wv = wv->next)
+        {
+          /* The contents of wv should match the top level menu.  */
+          EmacsMenu *submenu = (EmacsMenu*)[[menu itemAtIndex:i++] submenu];
+
+          [submenu fillWithWidgetValue: wv->contents frame: f];
+        }
+      needs_deep_update = NO;
+    }
+  else
+    {
+      [menu clear];
+      for (wv = first_wv->contents; wv; wv = wv->next)
+        {
 #ifdef NS_IMPL_COCOA
           /* We'll update the real copy under app menu when time comes.  */
           if (!strcmp ("Services", wv->name))
@@ -420,25 +356,17 @@
             }
           else
 #endif
-          [menu addSubmenuWithTitle: wv->name forFrame: f];
+            [menu addSubmenuWithTitle: wv->name forFrame: f];
+        }
 
-	  if (prev_wv)
-	    prev_wv->next = wv;
-	  else
-	    first_wv->contents = wv;
-	  prev_wv = wv;
-	}
+      /* We've cleared out the contents of the menus, so the next time
+         one is clicked on we'll need to run a deep update.  */
+      needs_deep_update = YES;
+    }
 
-      last_f = f;
-      if (n < 100)
-        n_previous_strings = n;
-      else
-        n_previous_strings = 0;
 
-    }
   free_menubar_widget_value_tree (first_wv);
 
-
 #if NSMENUPROFILE
   ftime (&tb);
   t += 1000*tb.time+tb.millitm;
@@ -460,21 +388,10 @@
 void
 set_frame_menubar (struct frame *f, bool first_time, bool deep_p)
 {
-  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
+  ns_update_menubar (f, deep_p);
 }
 
 
-
-
 /* ==========================================================================
 
     Menu: class implementation
@@ -514,43 +431,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 +438,8 @@ - (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
+  if (needs_deep_update)
+    ns_update_menubar (frame, true);
 }
 
 
@@ -1881,12 +1740,6 @@ - (Lisp_Object)runDialogAt: (NSPoint)p
 void
 syms_of_nsmenu (void)
 {
-#ifndef NS_IMPL_COCOA
-  /* Don't know how to keep track of this in Next/Open/GNUstep.  Always
-     update menus there.  */
-  trackingMenu = 1;
-  PDUMPER_REMEMBER_SCALAR (trackingMenu);
-#endif
   defsubr (&Sns_reset_menu);
   defsubr (&Smenu_or_popup_active_p);
 
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

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

[Mattias Engdegård]

26 dec. 2020 kl. 18.07 skrev Alan Third <alan@idiocy.org>:

> Does the attached seem any better? It appears to me like the first
> click on a menu is slightly faster (but that may be my imagination),
> and subsequent menus are instant.

Not bad at all! Slow spells all gone as far as I can tell. Ship it!

Next up, import the hacks from the Mac port that right-justifies keyboard shortcuts in the menus?

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

[Alan Third]

On Sat, Dec 26, 2020 at 06:42:18PM +0100, Mattias Engdegård wrote:
> 26 dec. 2020 kl. 18.07 skrev Alan Third <alan@idiocy.org>:
> 
> > Does the attached seem any better? It appears to me like the first
> > click on a menu is slightly faster (but that may be my imagination),
> > and subsequent menus are instant.
> 
> Not bad at all! Slow spells all gone as far as I can tell. Ship it!

Cool. I might do some further tidying up. I'll see how I feel.

> Next up, import the hacks from the Mac port that right-justifies
> keyboard shortcuts in the menus?

It doesn't actually right align them, but they're certainly neater.
It's actually almost possible to copy the Mac port code in verbatim,
the NS port's code is based on it after all, but I feel it may be
neater to use a custom view for the NSMenuItems. That would give us
complete control over the layout.

Of course, someone would have to actually do that work...
-- 
Alan Third

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

[Alan Third]

On Sat, Dec 26, 2020 at 09:52:10PM +0000, Alan Third wrote:
> On Sat, Dec 26, 2020 at 06:42:18PM +0100, Mattias Engdegård wrote:
> > 26 dec. 2020 kl. 18.07 skrev Alan Third <alan@idiocy.org>:
> > 
> > > Does the attached seem any better? It appears to me like the first
> > > click on a menu is slightly faster (but that may be my imagination),
> > > and subsequent menus are instant.
> > 
> > Not bad at all! Slow spells all gone as far as I can tell. Ship it!
> 
> Cool. I might do some further tidying up. I'll see how I feel.

I've pushed a modified version of this code to master.

It doesn't work on GNUstep, but GNUstep menus have been broken for a
while anyway, and I don't know what's wrong with them.

-- 
Alan Third

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

[Mattias Engdegård]

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

26 dec. 2020 kl. 22.52 skrev Alan Third <alan@idiocy.org>:

> It's actually almost possible to copy the Mac port code in verbatim,
> the NS port's code is based on it after all, but I feel it may be
> neater to use a custom view for the NSMenuItems. That would give us
> complete control over the layout.

We should, but because I was lazy and impatient, I wrote the attached (terrible) hack just to see what right-justification would look like.

The right margin is slightly ragged because I'm using plain spaces to offset the key binding from the menu text, but I still think it's better than what we have now. I tried using U+2009 THIN SPACE and even U+200A HAIR SPACE instead but somehow the result became worse; no doubt a silly mistake somewhere.

I also removed the special hack for s- bindings because it broke the nice alignment. If we want, we could translate modifiers into the standard symbols (⌘, ⌃, ⇧ and ⌥) depending on these are assigned, or just keep using the Emacs notation. We could also translate <right> into → and so on.


[-- Attachment #2: right-justify-keys.diff --]
[-- Type: application/octet-stream, Size: 4151 bytes --]

diff --git a/src/nsmenu.m b/src/nsmenu.m
index b5d0821323..f985234643 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -498,9 +498,16 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
       keyEq = [self parseKeyEquiv: wv->key];
 #ifdef NS_IMPL_COCOA
       /* macOS mangles modifier strings longer than one character.  */
-      if (keyEquivModMask == 0)
+      if (1 || keyEquivModMask == 0)
         {
-          title = [title stringByAppendingFormat: @" (%@)", keyEq];
+          if (wv->key)
+            {
+              const char *k = wv->key;
+              while (*k == ' ') k++;
+              title = [title stringByAppendingString:
+                               [NSString stringWithUTF8String: k]];
+            }
+          //title = [title stringByAppendingString: keyEq];
           item = [self addItemWithTitle: (NSString *)title
                                  action: @selector (menuDown:)
                           keyEquivalent: @""];
@@ -514,7 +521,7 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
 #ifdef NS_IMPL_COCOA
         }
 #endif
-      [item setKeyEquivalentModifierMask: keyEquivModMask];
+      //[item setKeyEquivalentModifierMask: keyEquivModMask];
 
       [item setEnabled: wv->enabled];
 
@@ -556,14 +563,67 @@ - (void)fillWithWidgetValue: (void *)wvptr
 
 - (void)fillWithWidgetValue: (void *)wvptr
 {
-  widget_value *wv = (widget_value *)wvptr;
+  widget_value *first_wv = (widget_value *)wvptr;
+  NSFont *menuFont = [NSFont menuFontOfSize:0];
+  NSDictionary <NSString *, id> *attributes =
+    [NSDictionary dictionaryWithObject:menuFont forKey:NSFontAttributeName];
+  const char space_str[] = " ";
+  int space_bytes = sizeof space_str - 1;
+  NSSize spaceSize = [[NSString stringWithUTF8String: space_str]
+                       sizeWithAttributes:attributes];
+  CGFloat spaceWidth = spaceSize.width;
+  CGFloat maxNameWidth = 0;
+  CGFloat maxKeyWidth = 0;
+
+  for (widget_value *wv = first_wv; wv != NULL; wv = wv->next)
+    if (!menu_separator_name_p (wv->name))
+      {
+        NSString *name = [NSString stringWithUTF8String: wv->name];
+        NSSize nameSize = [name sizeWithAttributes: attributes];
+        CGFloat nameWidth = nameSize.width;
+        maxNameWidth = MAX(maxNameWidth, nameWidth);
+        if (wv->key)
+          {
+            const char *k = wv->key;
+            while (*k == ' ') k++;
+            NSString *key = [NSString stringWithUTF8String: k];
+            NSSize keySize = [key sizeWithAttributes: attributes];
+            CGFloat keyWidth = keySize.width;
+            maxKeyWidth = MAX(maxKeyWidth, keyWidth);
+          }
+      }
 
   /* clear existing contents */
   [self removeAllItems];
 
   /* add new contents */
-  for (; wv != NULL; wv = wv->next)
+  for (widget_value *wv = first_wv; wv != NULL; wv = wv->next)
     {
+      if (wv->key && !menu_separator_name_p (wv->name))
+        {
+          NSString *name = [NSString stringWithUTF8String: wv->name];
+          NSSize nameSize = [name sizeWithAttributes: attributes];
+          CGFloat nameWidth = nameSize.width;
+          const char *k = wv->key;
+          while (*k == ' ') k++;
+          NSString *key = [NSString stringWithUTF8String: k];
+          NSSize keySize = [key sizeWithAttributes: attributes];
+          CGFloat keyWidth = keySize.width;
+          CGFloat padWidth = (maxNameWidth - nameWidth)
+                             + (maxKeyWidth - keyWidth);
+          int name_len = strlen (wv->name);
+          int extra_spaces = 7;
+          int pad_len = lround(padWidth / spaceWidth + extra_spaces);
+          Lisp_Object s = make_uninit_string (name_len + pad_len * space_bytes);
+          memcpy (SSDATA (s), wv->name, name_len);
+          if (space_bytes == 1)
+            memset (SDATA (s) + name_len, space_str[0], pad_len);
+          else
+            for (int i = 0; i < pad_len; i++)
+              memcpy (SDATA (s) + name_len + i * space_bytes, space_str,
+                      space_bytes);
+          wv->name = SSDATA (s);
+        }
       NSMenuItem *item = [self addItemWithWidgetValue: wv];
 
       if (wv->contents)

NS port menus (was: bug#44333: 27.1; macOS menu bar 2-clicks)

[Alan Third]

I've moved this to Emacs Devel because that bug report is closed now
and it didn't seem like the right place to discuss it.

On Sun, Dec 27, 2020 at 06:14:25PM +0100, Mattias Engdegård wrote:
> 26 dec. 2020 kl. 22.52 skrev Alan Third <alan@idiocy.org>:
> 
> > It's actually almost possible to copy the Mac port code in verbatim,
> > the NS port's code is based on it after all, but I feel it may be
> > neater to use a custom view for the NSMenuItems. That would give us
> > complete control over the layout.
> 
> We should, but because I was lazy and impatient, I wrote the
> attached (terrible) hack just to see what right-justification would
> look like.
> 
> The right margin is slightly ragged because I'm using plain spaces
> to offset the key binding from the menu text, but I still think it's
> better than what we have now. I tried using U+2009 THIN SPACE and
> even U+200A HAIR SPACE instead but somehow the result became worse;
> no doubt a silly mistake somewhere.

It looks pretty good as is! Slightly off, though, as you say.

I'd wondered about using NSAttributedString and NSParagraphStyle to
set a tab stop at a specific pixel point, that should allow perfect
alignment, or, and this is a bit hackier, display the binding in a
monospace font. Then you could use (variable space font) spaces and
tabs to line up the start of the binding text, then use (monospace
font) spaces to align the bindings. That's maybe a bit too much work,
though, and I'm not at all knowledgeable about how NSAttributedStrings
actually work, so maybe it's impossible.

> I also removed the special hack for s- bindings because it broke the
> nice alignment. If we want, we could translate modifiers into the
> standard symbols (⌘, ⌃, ⇧ and ⌥) depending on these are assigned, or
> just keep using the Emacs notation. We could also translate <right>
> into → and so on.

I think whatever we do it's probably best to be consistent, so losing
the special hack for s- bindings sounds good.

-- 
Alan Third

Re: NS port menus (was: bug#44333: 27.1; macOS menu bar 2-clicks)

[Mattias Engdegård]

27 dec. 2020 kl. 19.21 skrev Alan Third <alan@idiocy.org>:

> I've moved this to Emacs Devel because that bug report is closed now
> and it didn't seem like the right place to discuss it.

Agreed; this is now bug#45502.

> I'd wondered about using NSAttributedString and NSParagraphStyle to
> set a tab stop at a specific pixel point, that should allow perfect
> alignment, or, and this is a bit hackier, display the binding in a
> monospace font. Then you could use (variable space font) spaces and
> tabs to line up the start of the binding text, then use (monospace
> font) spaces to align the bindings. That's maybe a bit too much work,
> though, and I'm not at all knowledgeable about how NSAttributedStrings
> actually work, so maybe it's impossible.

It didn't look obvious how to do it, but I could use some help.