Re: Emacs 26 MacOS bugs

[Alan Third <alan@idiocy.org>]

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

On Wed, Feb 07, 2018 at 01:13:01PM +1100, Ben McGinnes wrote:
> On Tue, Feb 06, 2018 at 08:33:31PM -0500, Noam Postavsky wrote:
> > AFAIK, there's no firm date, but emacs-26 is in feature freeze and
> > only taking very safe/urgent bug fixes. So it's possible these bugs
> > won't be fixed until after 26.1
> 
> I think I could make a very convincing argument regarding the modifier
> keys here being urgent.  If it was only rendering the hyper key
> useless, then maybe you could argue it as not being urgent, but it's
> not merely that ...

The good news is that the patch that affects the modifier keys isn’t
in Emacs 26, it was pushed to master only.

> any attempt to assign the hyper function to any
> other key (e.g. right alt), just adds the hyper function instead of
> replacing the previous function with that.  So if left alt is meta and
> you set right alt as hyper, it just becomes a combined hyper-meta.
> It's no longer possible to set the hyper key on its own.
> 
> Which means the code commentary about changing the nil to none for all
> those ns modifier keys is, in fact, completely wrong.  You *do* want nil
> there because if you're re-assigning the key to perform another task
> you actually do want to clear the original setting out of it.
> 
> As for a stable fix, that's dead simple, roll back everything from the
> 2016 commit that was merged into master on the 5th and it's done.

I know you think there was nothing wrong with the old code, but it was
broken in various ways that weren’t obvious how to fix. The new code
should behave more consistently, once we clear up the bugs you’ve
found.

Can you please try the attached patch?

Philipp, it seems that EV_MODIFIERS2 was quite, quite broken. I’ve
split it into various parts and tried to fix it.

Additionally the fn key has some strange interactions with ‘function’
keys, like the Fx keys, arrows, tab and so on. The old code simply
didn’t set its modifier when those keys were used, so I’ve followed
that lead and am XORing it out of emacs_event->modifiers.
-- 
Alan Third

[-- Attachment #2: 0001-Fix-modifier-keys-on-NS-port.patch --]
[-- Type: text/plain, Size: 6472 bytes --]

From b089831704c91fbd0bf0baa45ba6f0c78cf06e41 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Wed, 7 Feb 2018 22:39:17 +0000
Subject: [PATCH] Fix modifier keys on NS port

* src/nsterm.m (EV_MODIFIERS_OPTION):
(EV_MODIFIERS_CONTROL):
(EV_MODIFIERS_COMMAND): New macros.
(EV_MODIFIERS2): Split out into new macros to make it easier to
understand.
(EmacsView::keyDown): Remove duplicate functionality and handle fn key
correctly.
---
 src/nsterm.m | 83 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 36 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index b7f5a32c09..58ebca4153 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -352,31 +352,45 @@ - (NSColor *)colorUsingDefaultColorSpace
 #define NSRightCommandKeyMask   (0x000010 | NSEventModifierFlagCommand)
 #define NSLeftAlternateKeyMask  (0x000020 | NSEventModifierFlagOption)
 #define NSRightAlternateKeyMask (0x000040 | NSEventModifierFlagOption)
-#define EV_MODIFIERS2(flags)                          \
-    (((flags & NSEventModifierFlagHelp) ?           \
-           hyper_modifier : 0)                        \
-     | (!EQ (ns_right_alternate_modifier, Qleft) && \
-        ((flags & NSRightAlternateKeyMask) \
-         == NSRightAlternateKeyMask) ? \
-           parse_solitary_modifier (ns_right_alternate_modifier) : 0) \
-     | ((flags & NSEventModifierFlagOption) ?                 \
-           parse_solitary_modifier (ns_alternate_modifier) : 0)   \
-     | ((flags & NSEventModifierFlagShift) ?     \
-           shift_modifier : 0)                        \
-     | (!EQ (ns_right_control_modifier, Qleft) && \
-        ((flags & NSRightControlKeyMask) \
-         == NSRightControlKeyMask) ? \
-           parse_solitary_modifier (ns_right_control_modifier) : 0) \
-     | ((flags & NSEventModifierFlagControl) ?      \
-           parse_solitary_modifier (ns_control_modifier) : 0)     \
-     | ((flags & NS_FUNCTION_KEY_MASK) ?  \
-           parse_solitary_modifier (ns_function_modifier) : 0)    \
-     | (!EQ (ns_right_command_modifier, Qleft) && \
-        ((flags & NSRightCommandKeyMask) \
-         == NSRightCommandKeyMask) ? \
-           parse_solitary_modifier (ns_right_command_modifier) : 0) \
-     | ((flags & NSEventModifierFlagCommand) ?      \
-           parse_solitary_modifier (ns_command_modifier):0))
+
+#define EV_MODIFIERS_OPTION(flags)                                      \
+  (((flags & NSRightAlternateKeyMask) == NSRightAlternateKeyMask &&     \
+    ! EQ (ns_right_alternate_modifier, Qleft)) ?                        \
+   parse_solitary_modifier (ns_right_alternate_modifier) : 0)           \
+  | (((flags & NSLeftAlternateKeyMask) == NSLeftAlternateKeyMask ||     \
+      (flags & NSRightAlternateKeyMask) == NSRightAlternateKeyMask &&   \
+      EQ (ns_right_alternate_modifier, Qleft)) ?                        \
+     parse_solitary_modifier (ns_alternate_modifier) : 0)
+
+#define EV_MODIFIERS_CONTROL(flags)                                   \
+  (((flags & NSRightControlKeyMask) == NSRightControlKeyMask &&       \
+    ! EQ (ns_right_control_modifier, Qleft)) ?                        \
+   parse_solitary_modifier (ns_right_control_modifier) : 0)           \
+  | (((flags & NSLeftControlKeyMask) == NSLeftControlKeyMask ||       \
+      (flags & NSRightControlKeyMask) == NSRightControlKeyMask &&     \
+      EQ (ns_right_control_modifier, Qleft)) ?                        \
+     parse_solitary_modifier (ns_control_modifier) : 0)
+
+#define EV_MODIFIERS_COMMAND(flags)                                     \
+  (((flags & NSRightCommandKeyMask) == NSRightCommandKeyMask &&         \
+    ! EQ (ns_right_command_modifier, Qleft)) ?                          \
+   parse_solitary_modifier (ns_right_command_modifier) : 0)             \
+  | (((flags & NSLeftCommandKeyMask) == NSLeftCommandKeyMask ||         \
+      (flags & NSRightCommandKeyMask) == NSRightCommandKeyMask &&       \
+      EQ (ns_right_command_modifier, Qleft)) ?                          \
+     parse_solitary_modifier (ns_command_modifier) : 0)
+
+#define EV_MODIFIERS2(flags)                                            \
+  (((flags & NSEventModifierFlagHelp) ?                                 \
+    hyper_modifier : 0)                                                 \
+   | ((flags & NSEventModifierFlagShift) ?                              \
+      shift_modifier : 0)                                               \
+   | ((flags & NS_FUNCTION_KEY_MASK) ?                                  \
+      parse_solitary_modifier (ns_function_modifier) : 0)               \
+   | EV_MODIFIERS_OPTION (flags)                                        \
+   | EV_MODIFIERS_CONTROL (flags)                                       \
+   | EV_MODIFIERS_COMMAND (flags))
+
 #define EV_MODIFIERS(e) EV_MODIFIERS2 ([e modifierFlags])
 
 #define EV_UDMODIFIERS(e)                                      \
@@ -6130,15 +6144,6 @@ flag set (this is probably a bug in the OS).
             code = fnKeysym;
         }
 
-      /* are there modifiers? */
-      emacs_event->modifiers = 0;
-
-      if (flags & NSEventModifierFlagHelp)
-          emacs_event->modifiers |= hyper_modifier;
-
-      if (flags & NSEventModifierFlagShift)
-        emacs_event->modifiers |= shift_modifier;
-
       /* The ⌘ and ⌥ modifiers can be either shift-like (for alternate
          character input) or control-like (as command prefix).  If we
          have only shift-like modifiers, then we should use the
@@ -6155,8 +6160,14 @@ untranslated characters (returned by the
          modifier keys, which returns 0 for shift-like modifiers.
          Therefore its return value is the set of control-like
          modifiers.  */
-      unsigned int control_modifiers = EV_MODIFIERS2 (flags);
-      emacs_event->modifiers |= control_modifiers;
+      emacs_event->modifiers = EV_MODIFIERS2 (flags);
+
+      /* Function keys (such as the F-keys, arrow keys, etc.) behave
+         as though the fn key has been pressed even when it hasn't.
+         We need to unset the fn modifier if one of these keys is
+         pressed.  FIXME: Avoid setting it in the first place.  */
+      if (fnKeysym && (flags & NS_FUNCTION_KEY_MASK))
+        emacs_event->modifiers ^= parse_solitary_modifier (ns_function_modifier);
 
       if (NS_KEYLOG)
         fprintf (stderr, "keyDown: code =%x\tfnKey =%x\tflags = %x\tmods = %x\n",
-- 
2.16.1