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

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

[Mattias Engdegård]

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

The NS port shows key bindings in a rather cluttered way, with the key in brackets directly after the menu entry. The Mac port of Emacs is much neater with the bindings all aligned at a common tab position. We could do the same, but having done some experiments I actually prefer a right-alignment of the keys. Proof-of-concept patch attached.

The alignment is made by padding with spaces, and then with hair spaces for extra precision; the result is not perfect but probably better than what we have now. If I get some time, I might do an experiment with more precise formatting.


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

diff --git a/src/nsmenu.m b/src/nsmenu.m
index 3f0cd0c6ed..6c96f5f123 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -450,32 +450,14 @@ - (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
 {
   NSMenuItem *item;
@@ -488,31 +470,20 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
     }
   else
     {
-      NSString *title, *keyEq;
+      NSString *title;
       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)
-        {
-          title = [title stringByAppendingFormat: @" (%@)", keyEq];
-          item = [self addItemWithTitle: (NSString *)title
-                                 action: @selector (menuDown:)
-                          keyEquivalent: @""];
-        }
-      else
-        {
-#endif
-          item = [self addItemWithTitle: (NSString *)title
-                                 action: @selector (menuDown:)
-                          keyEquivalent: keyEq];
-#ifdef NS_IMPL_COCOA
-        }
-#endif
-      [item setKeyEquivalentModifierMask: keyEquivModMask];
+      /* 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 stringByAppendingString:
+                         [NSString stringWithUTF8String: skipspc (wv->key)]];
+      item = [self addItemWithTitle: (NSString *)title
+                             action: @selector (menuDown:)
+                      keyEquivalent: @""];
 
       [item setEnabled: wv->enabled];
 
@@ -557,14 +528,69 @@ -(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 <NSString *, id> *attributes =
+    [NSDictionary dictionaryWithObject:menuFont forKey:NSFontAttributeName];
+  NSSize spaceSize = [@" " sizeWithAttributes:attributes];
+  CGFloat spaceWidth = spaceSize.width;
+  const char hair_str[] = "\u200a";  // HAIR SPACE
+  int hair_bytes = sizeof hair_str - 1;
+  NSSize hairSize = [[NSString stringWithUTF8String: hair_str]
+                       sizeWithAttributes:attributes];
+  CGFloat hairWidth = spaceSize.width;
+  CGFloat maxNameWidth = 0;
+  CGFloat maxKeyWidth = 0;
+
+  /* Determine maximum width of 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: attributes];
+        CGFloat nameWidth = nameSize.width;
+        maxNameWidth = MAX(maxNameWidth, nameWidth);
+        if (wv->key)
+          {
+            NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+            NSSize keySize = [key sizeWithAttributes: attributes];
+            CGFloat keyWidth = keySize.width;
+            maxKeyWidth = MAX(maxKeyWidth, keyWidth);
+          }
+      }
+  CGFloat maxWidth = maxNameWidth + maxKeyWidth;
 
   /* 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))
+        {
+          /* Modify name to include padding to right-justify key binding. */
+          NSString *name = [NSString stringWithUTF8String: wv->name];
+          NSSize nameSize = [name sizeWithAttributes: attributes];
+          CGFloat nameWidth = nameSize.width;
+          NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+          NSSize keySize = [key sizeWithAttributes: attributes];
+          CGFloat keyWidth = keySize.width;
+          CGFloat padWidth = maxWidth - (nameWidth + keyWidth);
+          /* Use plain spaces as far as possible, and hair spaces for
+             the rest. */
+          int extra_spaces = 4;
+          int nspaces = padWidth / spaceWidth + extra_spaces;
+          int nhairs = fmod(padWidth, spaceWidth) / hairWidth + 0.5;
+          int name_len = strlen (wv->name);
+          Lisp_Object s = make_uninit_string (name_len + nspaces
+                                              + nhairs * hair_bytes);
+          memcpy (SSDATA (s), wv->name, name_len);
+          memset (SDATA (s) + name_len, ' ', nspaces);
+          for (int i = 0; i < nhairs; i++)
+            memcpy (SDATA (s) + name_len + nspaces + i * hair_bytes, hair_str,
+                    hair_bytes);
+          wv->name = SSDATA (s);
+        }
       NSMenuItem *item = [self addItemWithWidgetValue: wv];
 
       if (wv->contents)
diff --git a/src/nsterm.h b/src/nsterm.h
index b7b4d3b047..c4873b6082 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -515,7 +515,6 @@ #define NS_DRAW_TO_BUFFER 1
 
 @interface EmacsMenu : NSMenu  <NSMenuDelegate>
 {
-  unsigned long keyEquivModMask;
   BOOL needsUpdate;
 }
 

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

[Alan Third]

On Mon, Dec 28, 2020 at 03:23:25PM +0100, Mattias Engdegård wrote:
> The NS port shows key bindings in a rather cluttered way, with the
> key in brackets directly after the menu entry. The Mac port of Emacs
> is much neater with the bindings all aligned at a common tab
> position. We could do the same, but having done some experiments I
> actually prefer a right-alignment of the keys. Proof-of-concept
> patch attached.
> 
> The alignment is made by padding with spaces, and then with hair
> spaces for extra precision; the result is not perfect but probably
> better than what we have now. If I get some time, I might do an
> experiment with more precise formatting.
> 


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

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

NSMutableParagraphStyle *p = [NSMutableParagraphStyle
                               defaultParagraphStyle];
NSArray *tabstops = [NSArray withObject:[[[NSTextTab alloc]
                        initWithTextAlignment:NSTextAlignmentRight
                                     location:50 // This is probably in points.
                                      options:nil];
[p setTabStops:tabstops];

NSAttributedString *s = [[NSAttributedString alloc]
                          initWithString:yourString
                              attributes:someDictionaryObjectIncludingTheParagraphStyle];

[releaseAllTheAllocedObjects];

I can never remember how to make dictionaries, so I'll leave that to
you to work out. ;)

Hopefully then all you have to do is set the string as the title and it will
be drawn with the correct tabstop. Maybe NSMenuItems are special and
it won't work, though.
-- 
Alan Third

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

[Mattias Engdegård]

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

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

[Alan Third]

On Tue, Dec 29, 2020 at 01:02:21PM +0100, Mattias Engdegård wrote:
> 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.)

No feigning required. 😉

> Resulting patch attached.

Looks good. As I understand it GNUstep doesn't need this fix, so
please make it Cocoa only.

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

There was a work-around for some problem related to waiting_for_input
in the old menu code that was lost when I copied the code from
xmenu.c, so I've put it back in and pushed to master. It looks like it
fixes the crash here.

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

There's also the problem that we allow setting the left and right
modifiers separately, so option or command may not match both keys.

-- 
Alan Third

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

[Mattias Engdegård]

29 dec. 2020 kl. 14.53 skrev Alan Third <alan@idiocy.org>:

> Looks good. As I understand it GNUstep doesn't need this fix, so
> please make it Cocoa only.

Will do, but what do you mean by not needing it -- does it do any harm, given that GNUstep menus do not appear to be working anyway? I have no GNUstep system to try it on so I don't know if it will even build.

> There was a work-around for some problem related to waiting_for_input
> in the old menu code that was lost when I copied the code from
> xmenu.c, so I've put it back in and pushed to master. It looks like it
> fixes the crash here.

Thank you, that did it.

> There's also the problem that we allow setting the left and right
> modifiers separately, so option or command may not match both keys.

That's true. Maybe a hybrid like M-↓ or C-⌫ is still fine though since the symbols are very recognisable -- I'll give it a go.

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

[Alan Third]

On Tue, Dec 29, 2020 at 03:41:03PM +0100, Mattias Engdegård wrote:
> 29 dec. 2020 kl. 14.53 skrev Alan Third <alan@idiocy.org>:
> 
> > Looks good. As I understand it GNUstep doesn't need this fix, so
> > please make it Cocoa only.
> 
> Will do, but what do you mean by not needing it -- does it do any
> harm, given that GNUstep menus do not appear to be working anyway? I
> have no GNUstep system to try it on so I don't know if it will even
> build.

As far as I'm aware GNUstep is able to display the bindings correctly
no matter how many characters they have, so we don't need the
work-around. And I'm hoping that we'll be able to get GNUstep menus
working again in the future.

I think the problem is something to do with drawing the frame rather
than a problem with building the menus. But I could be wrong.

> > There's also the problem that we allow setting the left and right
> > modifiers separately, so option or command may not match both keys.
> 
> That's true. Maybe a hybrid like M-↓ or C-⌫ is still fine though
> since the symbols are very recognisable -- I'll give it a go.

Yes, I think so.
-- 
Alan Third

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

[Mattias Engdegård]

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

29 dec. 2020 kl. 16.50 skrev Alan Third <alan@idiocy.org>:

> As far as I'm aware GNUstep is able to display the bindings correctly
> no matter how many characters they have, so we don't need the
> work-around. And I'm hoping that we'll be able to get GNUstep menus
> working again in the future.

All right, I've done what I think you meant and pushed to master; please tell me if I'm on the wrong track.

Here is a proof-of-concept patch for using fancy symbols instead of <right> etc.
Not production quality because (1) it's obviously ugly code, (2) I'm unsure about the PgUp/PgDn/Home/End symbols and (3) it only works with modifiers if the patch in bug#45536 is applied (which of course I think it should), but at least it gives a feeling for whether it's a good thing to do or not.

There is also the question whether to follow the platform convention of always using upper case letters, with an explicit Shift modifier if necessary.


[-- Attachment #2: key-symbols.diff --]
[-- Type: application/octet-stream, Size: 4796 bytes --]

diff --git a/src/nsmenu.m b/src/nsmenu.m
index d5321dcdc6..36566e9a11 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -552,9 +552,76 @@ - (void)fillWithWidgetValue: (void *)wvptr
         maxNameWidth = MAX(maxNameWidth, nameSize.width);
         if (wv->key)
           {
-            NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+            NSMutableString *key =
+              [NSMutableString stringWithUTF8String: skipspc (wv->key)];
+            [key replaceOccurrencesOfString: @"<backspace>"
+                                 withString: @"⌫"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"DEL"
+                                 withString: @"⌫"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<deletechar>"
+                                 withString: @"⌦"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<return>"
+                                 withString: @"↩"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"RET"
+                                 withString: @"↩"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<left>"
+                                 withString: @"←"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<right>"
+                                 withString: @"→"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<up>"
+                                 withString: @"↑"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<down>"
+                                 withString: @"↓"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<prior>"
+                                 withString: @"⇞"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<next>"
+                                 withString: @"⇟"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<home>"
+                                 withString: @"↖"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<end>"
+                                 withString: @"↘"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<tab>"
+                                 withString: @"⇥"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"TAB"
+                                 withString: @"⇥"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
+            [key replaceOccurrencesOfString: @"<backtab>"
+                                 withString: @"⇤"
+                                    options: NSLiteralSearch
+                                      range: NSMakeRange (0, [key length])];
             NSSize keySize = [key sizeWithAttributes: font_attribs];
             maxKeyWidth = MAX(maxKeyWidth, keySize.width);
+            Lisp_Object newkey = build_string ([key UTF8String]);
+            wv->key = SSDATA (newkey);
           }
       }
 

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

[Alan Third]

On Tue, Dec 29, 2020 at 06:34:20PM +0100, Mattias Engdegård wrote:
> 29 dec. 2020 kl. 16.50 skrev Alan Third <alan@idiocy.org>:
> 
> > As far as I'm aware GNUstep is able to display the bindings correctly
> > no matter how many characters they have, so we don't need the
> > work-around. And I'm hoping that we'll be able to get GNUstep menus
> > working again in the future.
> 
> All right, I've done what I think you meant and pushed to master;
> please tell me if I'm on the wrong track.

I've pushed a small change. I also set the alloc'd objects to
autorelease. We need to do that because when they're assigned to the
NSMenuItem it will retain them, and because we alloc'd them we already
have them retained once.

At least I hope that's right. For some reason I've found the
retain/release cycle really hard to understand, but I think I'm
getting there now.

> Here is a proof-of-concept patch for using fancy symbols instead of
> <right> etc.
>
> Not production quality because (1) it's obviously ugly code, (2) I'm
> unsure about the PgUp/PgDn/Home/End symbols and (3) it only works
> with modifiers if the patch in bug#45536 is applied (which of course
> I think it should), but at least it gives a feeling for whether it's
> a good thing to do or not.

I have no opinion on this, really. Although that is some ugly code! ;)

I have no ideas for how to improve it, though.

> There is also the question whether to follow the platform convention
> of always using upper case letters, with an explicit Shift modifier
> if necessary.

I think that might be confusing. It doesn't matter with most macOS
apps because they don't differentiate between shortcuts with upper and
lower case characters, but we do, and I feel as a seasoned Emacs user
I'd see C-A and think that was actually C-S-a when it might be C-a.

-- 
Alan Third

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

[Mattias Engdegård]

29 dec. 2020 kl. 22.24 skrev Alan Third <alan@idiocy.org>:

> I've pushed a small change. I also set the alloc'd objects to
> autorelease. We need to do that because when they're assigned to the
> NSMenuItem it will retain them, and because we alloc'd them we already
> have them retained once.
> 
> At least I hope that's right. For some reason I've found the
> retain/release cycle really hard to understand, but I think I'm
> getting there now.

Thanks! Is this the standard way of doing it? The objects that you marked autorelease (tab and pstyle) are only used for the extent of that method; I take it you prefer autorelease to sending them 'release' at the end of the method?

Presumably 'atitle' should be sent autorelease (or release) as well?

> I have no opinion on this, really. Although that is some ugly code! ;)

Oh yes, it should at least be table-driven in some way. 

> I think that might be confusing. It doesn't matter with most macOS
> apps because they don't differentiate between shortcuts with upper and
> lower case characters, but we do, and I feel as a seasoned Emacs user
> I'd see C-A and think that was actually C-S-a when it might be C-a.

Agreed. If we were to follow the macOS conventions fully then it would make sense to use ⌘K for M-k (if Command is Meta), but otherwise it's more likely to be confusing.

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

[Alan Third]

On Tue, Dec 29, 2020 at 11:53:59PM +0100, Mattias Engdegård wrote:
> 29 dec. 2020 kl. 22.24 skrev Alan Third <alan@idiocy.org>:
> 
> > I've pushed a small change. I also set the alloc'd objects to
> > autorelease. We need to do that because when they're assigned to the
> > NSMenuItem it will retain them, and because we alloc'd them we already
> > have them retained once.
> > 
> > At least I hope that's right. For some reason I've found the
> > retain/release cycle really hard to understand, but I think I'm
> > getting there now.
> 
> Thanks! Is this the standard way of doing it? The objects that you
> marked autorelease (tab and pstyle) are only used for the extent of
> that method; I take it you prefer autorelease to sending them
> 'release' at the end of the method?

I think it's maybe visually a little neater to use autorelease, but
either way works and I would probably do it the other way in different
circumstances. Feel free to change it if you want.

> Presumably 'atitle' should be sent autorelease (or release) as well?

Yes, I missed that one. And actually, I think the alloc'd NSMenuItem
on line 484 will need released too. It should probably be autoreleased
because it's returned to the calling function, and the caller can then
decide whether to retain it or not (it doesn't in this case).

-- 
Alan Third

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

[Mattias Engdegård]

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

30 dec. 2020 kl. 00.49 skrev Alan Third <alan@idiocy.org>:

> I think it's maybe visually a little neater to use autorelease, but
> either way works and I would probably do it the other way in different
> circumstances. Feel free to change it if you want.

Thanks, I'm sticking to autorelease for uniformity here; slightly delayed deallocation is no worse than GC after all.

>> Presumably 'atitle' should be sent autorelease (or release) as well?
> 
> Yes, I missed that one. And actually, I think the alloc'd NSMenuItem
> on line 484 will need released too. It should probably be autoreleased
> because it's returned to the calling function, and the caller can then
> decide whether to retain it or not (it doesn't in this case).

Yes, fixed.

(By the way, 'chording' isn't quite the same as a multi-key sequence; chords are rather simultaneous presses, like C-M-x, no? To continue a musical metaphor, perhaps an Emacs key sequence is an arpeggio?)

Here is a slightly less ugly variant of the symbol substitution patch. Maybe we should apply it and see if there are any complaints, or if we turn against it ourselves later on?


[-- Attachment #2: 0001-Use-standard-key-symbols-in-NS-menu-entries.patch --]
[-- Type: application/octet-stream, Size: 3757 bytes --]

From 7ccf892b25d04d94bdcb76dfb6a000028e7b3c2a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 30 Dec 2020 13:06:47 +0100
Subject: [PATCH] Use standard key symbols in NS menu entries

* src/nsmenu.m (skipspc): Remove.
(key_symbols, prettify_key): New.
([EmacsMenu fillWithWidgetValue:]): Call prettify_key.
---
 src/nsmenu.m | 70 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/src/nsmenu.m b/src/nsmenu.m
index 34912705e2..c124bf500d 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -458,14 +458,6 @@ - (BOOL)performKeyEquivalent: (NSEvent *)theEvent
 }
 
 
-static const char *
-skipspc (const char *s)
-{
-  while (*s == ' ')
-    s++;
-  return s;
-}
-
 - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
                             attributes: (NSDictionary *)attributes
 {
@@ -485,7 +477,7 @@ - (NSMenuItem *)addItemWithWidgetValue: (void *)wvptr
       item = [[[NSMenuItem alloc] init] autorelease];
       if (wv->key)
         {
-          NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+          NSString *key = [NSString stringWithUTF8String: wv->key];
 #ifdef NS_IMPL_COCOA
           /* Cocoa only permits a single key (with modifiers) as
              keyEquivalent, so we put them in the title string
@@ -537,6 +529,63 @@ -(void)removeAllItems
 }
 
 
+typedef struct {
+  const char *from, *to;
+} subst_t;
+
+/* Standard keyboard symbols used in menus. */
+static const subst_t key_symbols[] = {
+  {"<backspace>",  "⌫"},
+  {"DEL",          "⌫"},
+  {"<deletechar>", "⌦"},
+  {"<return>",     "↩"},
+  {"RET",          "↩"},
+  {"<left>",       "←"},
+  {"<right>",      "→"},
+  {"<up>",         "↑"},
+  {"<down>",       "↓"},
+  {"<prior>",      "⇞"},
+  {"<next>",       "⇟"},
+  {"<home>",       "↖"},
+  {"<end>",        "↘"},
+  {"<tab>",        "⇥"},
+  {"TAB",          "⇥"},
+  {"<backtab>",    "⇤"},
+};
+
+/* Transform the key sequence KEY into something prettier by
+   substituting keyboard symbols. */
+static char *
+prettify_key (const char *key)
+{
+  while (*key == ' ') key++;
+
+  int len = strlen (key);
+  char *buf = xmalloc (len + 1);
+  memcpy (buf, key, len + 1);
+  for (int i = 0; i < ARRAYELTS (key_symbols); i++)
+    {
+      ptrdiff_t fromlen = strlen (key_symbols[i].from);
+      char *p = buf;
+      while (p < buf + len)
+        {
+          char *match = memmem (buf, len, key_symbols[i].from, fromlen);
+          if (!match)
+            break;
+          ptrdiff_t tolen = strlen (key_symbols[i].to);
+          eassert (tolen <= fromlen);
+          memcpy (match, key_symbols[i].to, tolen);
+          memmove (match + tolen, match + fromlen,
+                   len - (match + fromlen - buf) + 1);
+          len -= fromlen - tolen;
+          p = match + tolen;
+        }
+    }
+  Lisp_Object result = build_string (buf);
+  xfree (buf);
+  return SSDATA (result);
+}
+
 - (void)fillWithWidgetValue: (void *)wvptr
 {
   widget_value *first_wv = (widget_value *)wvptr;
@@ -560,7 +609,8 @@ - (void)fillWithWidgetValue: (void *)wvptr
         maxNameWidth = MAX(maxNameWidth, nameSize.width);
         if (wv->key)
           {
-            NSString *key = [NSString stringWithUTF8String: skipspc (wv->key)];
+            wv->key = prettify_key (wv->key);
+            NSString *key = [NSString stringWithUTF8String: wv->key];
             NSSize keySize = [key sizeWithAttributes: font_attribs];
             maxKeyWidth = MAX(maxKeyWidth, keySize.width);
           }
-- 
2.21.1 (Apple Git-122.3)

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

[Alan Third]

On Wed, Dec 30, 2020 at 01:19:48PM +0100, Mattias Engdegård wrote:
> 30 dec. 2020 kl. 00.49 skrev Alan Third <alan@idiocy.org>:
> 
> (By the way, 'chording' isn't quite the same as a multi-key
> sequence; chords are rather simultaneous presses, like C-M-x, no? To
> continue a musical metaphor, perhaps an Emacs key sequence is an
> arpeggio?)

Good point. I thought about it for a while and wasn't really sure what
to write, so my comment wasn't very good even ignoring that. :)

> Here is a slightly less ugly variant of the symbol substitution
> patch. Maybe we should apply it and see if there are any complaints,
> or if we turn against it ourselves later on?

It looks good to me, so I see no reason not to apply it and see if
anyone complains.

Oh, I noticed that the Mark menu in dired has the Help menu search
entry. I guess I've missed something somewhere. I wondered if that was
set somewhere but couldn't see any reference to it so assumed macOS
was doing something clever when it saw a "Help" top level submenu...

BTW, this isn't a comment on your code, I'm just curious. I notice you
use C functions within Obj C classes where I would probably have just
created another method. Is that just because they're dealing with C
code exclusively or something else?

-- 
Alan Third

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

[Alan Third]

On Wed, Dec 30, 2020 at 12:46:52PM +0000, Alan Third wrote:
> Oh, I noticed that the Mark menu in dired has the Help menu search
> entry. I guess I've missed something somewhere. I wondered if that was
> set somewhere but couldn't see any reference to it so assumed macOS
> was doing something clever when it saw a "Help" top level submenu...

Ah, got it!

modified   src/nsmenu.m
@@ -365,6 +365,9 @@
       else
         submenu = [menu addSubmenuWithTitle: wv->name];
 
+      if ([[submenu title] isEqualToString:@"Help"])
+        [NSApp setHelpMenu:submenu];
+
       if (deep_p)
         [submenu fillWithWidgetValue: wv->contents];


-- 
Alan Third

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

[Mattias Engdegård]

30 dec. 2020 kl. 14.09 skrev Alan Third <alan@idiocy.org>:

> Ah, got it!

Good catch! I think we've squeezed this bug dry by now; closing.

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

[Mattias Engdegård]

30 dec. 2020 kl. 13.46 skrev Alan Third <alan@idiocy.org>:

> It looks good to me, so I see no reason not to apply it and see if
> anyone complains.

Good, will do.

> Oh, I noticed that the Mark menu in dired has the Help menu search
> entry. I guess I've missed something somewhere. I wondered if that was
> set somewhere but couldn't see any reference to it so assumed macOS
> was doing something clever when it saw a "Help" top level submenu...

It's on the menu with the same index as Help normally has (because Dired adds multiple menus to the bar); the same effect can be seen in M-x shell. Perhaps we manage to confuse the Cocoa automatics somehow?

> BTW, this isn't a comment on your code, I'm just curious. I notice you
> use C functions within Obj C classes where I would probably have just
> created another method. Is that just because they're dealing with C
> code exclusively or something else?

It's mostly accidental, but it is true that there is a tension between different styles here -- I find plain functions to be fine for doing the job of a function (being a functional programmer at heart helps, of course...) and Objective-C allows it seamlessly. Methods don't bring much when they don't somehow operate on the object state, and they require declaration in the .h file even if only used locally (I might be mistaken here).

Thus no religion here; I can do it either way.

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

[Unknown]

Mattias Engdegård <mattiase@acm.org> writes:

> The NS port shows key bindings in a rather cluttered way, with the key
> in brackets directly after the menu entry. The Mac port of Emacs is
> much neater with the bindings all aligned at a common tab position. We
> could do the same, but having done some experiments I actually prefer
> a right-alignment of the keys. Proof-of-concept patch attached.
>
> The alignment is made by padding with spaces, and then with hair
> spaces for extra precision; the result is not perfect but probably
> better than what we have now. If I get some time, I might do an
> experiment with more precise formatting.

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

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.