bug#45502: [PATCH] Prettier key bindings in NS menu entries

["Mattias Engdegård" <mattiase@acm.org>]

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

28 dec. 2020 kl. 19.36 skrev Alan Third <alan@idiocy.org>:

> I'm not sure either, but I guess the tabstop thing would look
> something like: [...]

Actually seems to work! (In your place I would feign a complete lack of surprise.)
Resulting patch attached.

28 dec. 2020 kl. 23.46 skrev Daniel Martín <mardani29@yahoo.es>:

> Thanks for the patch! It crashed Emacs when I tried to open the Gnus
> menu bar (the Gnus menu bar is an extreme case with lots of bindings).

Confirmed, but that is unrelated to my patch. Alan, will you have a look?

> I'm not sure if left-alignment or right-alignment would be better.  To
> improve visuals, Apple seems to align with respect to the ⌘ symbol, but
> that doesn't fit Emacs well because there's no single modifier that is
> used in almost every keybinding (some use Control, some Meta).  Also,
> it's not uncommon in Emacs to have keybindings that are a couple of
> keymaps deep.

Right; it's easy to use either left or right alignment for the bindings. (I think we all agree that they should be kept in a separate column to the right of the menu strings in either case.) I'm going to experiment with translating modifiers and keys to the standard symbols. Not sure how to deal with modifiers that are unavailable, such as s-k when no Super modifier is available.

It might be a good idea to normalise how key bindings are displayed on all platforms to some extent. For example, <C-return> is better written C-<return> or C-RET, <M-backspace> is better as M-<backspace> or M-DEL (although not necessarily exactly the same thing, it's a bit muddy).

Of course <> are terrible as angle brackets; we typically want ‹› or ⟨⟩ depending on context and purpose.


[-- Attachment #2: 0001-Right-justify-keys-in-NS-menu-entries-bug-45502.patch --]
[-- Type: application/octet-stream, Size: 7473 bytes --]

From 3ad8a068e7743d947377087429b204996c2a650c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 28 Dec 2020 15:24:08 +0100
Subject: [PATCH] Right-justify keys in NS menu entries (bug#45502)

Each menu entry now has the key binding right-aligned, as an attempt
to improve readability.  Previously the keys were given in brackets
immediately following the menu string.

* src/nsmenu.m ([EmacsMenu parseKeyEquiv:]): Remove.
(skipspc): New helper function.
([EmacsMenu addItemWithWidgetValue:]): Add attributes argument.
Use attributed title string.  Don't special-case Super bindings.
([EmacsMenu fillWithWidgetValue:]): Compute maximum width.  Prepare
attributes for title.
---
 src/nsmenu.m | 103 ++++++++++++++++++++++++++++-----------------------
 src/nsterm.h |   5 +--
 2 files changed, 58 insertions(+), 50 deletions(-)

diff --git a/src/nsmenu.m b/src/nsmenu.m
index 23699627b1..967388f3bb 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -448,33 +448,16 @@ - (BOOL)performKeyEquivalent: (NSEvent *)theEvent
 }
 
 
-/* Parse a widget_value's key rep (examples: 's-p', 's-S', '(C-x C-s)', '<f13>')
-   into an accelerator string.  We are only able to display a single character
-   for an accelerator, together with an optional modifier combination.  (Under
-   Carbon more control was possible, but in Cocoa multi-char strings passed to
-   NSMenuItem get ignored.  For now we try to display a super-single letter
-   combo, and return the others as strings to be appended to the item title.
-   (This is signaled by setting keyEquivModMask to 0 for now.) */
--(NSString *)parseKeyEquiv: (const char *)key
+static const char *
+skipspc (const char *s)
 {
-  const char *tpos = key;
-  keyEquivModMask = NSEventModifierFlagCommand;
-
-  if (!key || !*key)
-    return @"";
-
-  while (*tpos == ' ' || *tpos == '(')
-    tpos++;
-  if ((*tpos == 's') && (*(tpos+1) == '-'))
-    {
-      return [NSString stringWithFormat: @"%c", tpos[2]];
-    }
-  keyEquivModMask = 0; /* signal */
-  return [NSString stringWithUTF8String: tpos];
+  while (*s == ' ')
+    s++;
+  return s;
 }
 
-
 - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
+                            attributes: (NSDictionary *)attributes
 {
   NSMenuItem *item;
   widget_value *wv = (widget_value *)wvptr;
@@ -482,36 +465,28 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
   if (menu_separator_name_p (wv->name))
     {
       item = [NSMenuItem separatorItem];
-      [self addItem: item];
     }
   else
     {
-      NSString *title, *keyEq;
-      title = [NSString stringWithUTF8String: wv->name];
+      NSString *title = [NSString stringWithUTF8String: wv->name];
       if (title == nil)
         title = @"< ? >";  /* (get out in the open so we know about it) */
 
-      keyEq = [self parseKeyEquiv: wv->key];
-#ifdef NS_IMPL_COCOA
-      /* macOS mangles modifier strings longer than one character.  */
-      if (keyEquivModMask == 0)
+      /* Cocoa only permits a single key (with modifiers) as
+         keyEquivalent, so we stick with the standard Emacs notation
+         for all key bindings for the sake of uniformity. */
+      if (wv->key)
         {
-          title = [title stringByAppendingFormat: @" (%@)", keyEq];
-          item = [self addItemWithTitle: (NSString *)title
-                                 action: @selector (menuDown:)
-                          keyEquivalent: @""];
+          NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+          title = [title stringByAppendingFormat: @"\t%@", key];
         }
-      else
-        {
-#endif
-          item = [self addItemWithTitle: (NSString *)title
-                                 action: @selector (menuDown:)
-                          keyEquivalent: keyEq];
-#ifdef NS_IMPL_COCOA
-        }
-#endif
-      [item setKeyEquivalentModifierMask: keyEquivModMask];
 
+      NSAttributedString *atitle = [[NSAttributedString alloc]
+                                         initWithString: title
+                                             attributes: attributes];
+      item = [[NSMenuItem alloc] init];
+      [item setAction: @selector (menuDown:)];
+      [item setAttributedTitle: atitle];
       [item setEnabled: wv->enabled];
 
       /* Draw radio buttons and tickboxes.  */
@@ -524,6 +499,7 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
       [item setTag: (NSInteger)wv->call_data];
     }
 
+  [self addItem: item];
   return item;
 }
 
@@ -548,15 +524,48 @@ -(void)removeAllItems
 
 - (void)fillWithWidgetValue: (void *)wvptr
 {
-  widget_value *wv = (widget_value *)wvptr;
+  widget_value *first_wv = (widget_value *)wvptr;
+  NSFont *menuFont = [NSFont menuFontOfSize:0];
+  NSDictionary *font_attribs = @{NSFontAttributeName: menuFont};
+  CGFloat maxNameWidth = 0;
+  CGFloat maxKeyWidth = 0;
+
+  /* Determine the maximum width of all menu items. */
+  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: font_attribs];
+        maxNameWidth = MAX(maxNameWidth, nameSize.width);
+        if (wv->key)
+          {
+            NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+            NSSize keySize = [key sizeWithAttributes: font_attribs];
+            maxKeyWidth = MAX(maxKeyWidth, keySize.width);
+          }
+      }
+
+  /* Put some space between the names and keys. */
+  CGFloat maxWidth = maxNameWidth + maxKeyWidth + 40;
+
+  /* Set a right-aligned tab stop at the maximum width, so that the
+     key will appear immediately to the left of it. */
+  NSTextTab *tab =
+    [[NSTextTab alloc] initWithTextAlignment: NSTextAlignmentRight
+                                    location: maxWidth
+                                     options: @{}];
+  NSMutableParagraphStyle *pstyle = [[NSMutableParagraphStyle alloc] init];
+  [pstyle setTabStops: @[tab]];
+  NSDictionary *attributes = @{NSParagraphStyleAttributeName: pstyle};
 
   /* 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)
     {
-      NSMenuItem *item = [self addItemWithWidgetValue: wv];
+      NSMenuItem *item = [self addItemWithWidgetValue: wv
+                                           attributes: attributes];
 
       if (wv->contents)
         {
diff --git a/src/nsterm.h b/src/nsterm.h
index b7b4d3b047..f1d5acde2e 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -515,13 +515,12 @@ #define NS_DRAW_TO_BUFFER 1
 
 @interface EmacsMenu : NSMenu  <NSMenuDelegate>
 {
-  unsigned long keyEquivModMask;
   BOOL needsUpdate;
 }
 
 - (void)menuNeedsUpdate: (NSMenu *)menu; /* (delegate method) */
-- (NSString *)parseKeyEquiv: (const char *)key;
-- (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr;
+- (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
+                            attributes: (NSDictionary *)attributes;
 - (void)fillWithWidgetValue: (void *)wvptr;
 - (EmacsMenu *)addSubmenuWithTitle: (const char *)title;
 - (void) removeAllItems;
-- 
2.21.1 (Apple Git-122.3)