bug#19977: 24.4; Incorrect translation of Super modifier with Ctrl or Meta on OS X

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1385 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Mi., 30. März 2016 um 04:39 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 29 Mar 2016 20:07:55 +0000
> > Cc: Eli Zaretskii <eliz@gnu.org>, 19977@debbugs.gnu.org
> >
> > It seems that this behavior cannot be implemented without resorting to
> UCKeyTranslate. Therefore I'd
> > suggest to fall back to the next best option and ignore all shift-like
> modifiers if control-like modifiers are
> > present, similar to what we're doing with C-S on Unix terminals.
>
> I'm not sure what this means, but if it means something that worked
> before won't, please provide an option to get the old behavior back,
> just in case.
>
>
I've attached a patch that should keep the aforementioned input methods
working (by setting ns-command-modifier to none) and allow Command and
Option to be treated as either shift-like or control-like modifiers.
In my tests input now works as expected with the Dvorak - Querty and
similar input methods if ns-command-modifier is none. Also various key
combinations with Super work now if it's set to super.
One thing that might be unexpected is that e.g. Command-Control-A will be
interpreted as Control-A if ns-command-modifier is none, even if Command-A
would insert something other than A. It seems this is (undesirable)
behavior is actually already present at head.

[-- Attachment #1.2: Type: text/html, Size: 1899 bytes --]

[-- Attachment #2: 0001-Fix-handling-of-modifier-keys-on-OS-X.patch --]
[-- Type: application/octet-stream, Size: 13651 bytes --]

From 008e71c08cbc06ac2d3ee3b353fe363ba620f4fd Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Wed, 30 Mar 2016 19:22:56 +0200
Subject: [PATCH] Fix handling of modifier keys on OS X
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/nsterm.m (is_shift_modifier, has_shift_modifiers): New helper
functions.
(keyDown:): Distinguish between shift-like and control-like modifier
keys.  Allow treating ⌘ as shift-like modifier (e.g. for the
Gujarati – QUERTY input method, where ⌘ switches to QUERTY.)

* lisp/cus-start.el (standard): Change nil to none for
ns-command-modifier; update description.
---
 lisp/cus-start.el |   8 ++-
 src/nsterm.m      | 208 ++++++++++++++++++++++++------------------------------
 2 files changed, 98 insertions(+), 118 deletions(-)

diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 5be61ce..12269c1 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -389,6 +389,10 @@ minibuffer-prompt-properties--setter
 	     ;; msdos.c
 	     (dos-unsupported-char-glyph display integer)
 	     ;; nsterm.m
+             ;;
+             ;; FIXME: Why does ⌃ use nil instead of none?  Also the
+             ;; description is confusing; setting it to nil disables ⌃
+             ;; entirely.
 	     (ns-control-modifier
 	      ns
 	      (choice (const :tag "No modifier" nil)
@@ -405,13 +409,13 @@ minibuffer-prompt-properties--setter
 		      (const super)) "24.1")
 	     (ns-command-modifier
 	      ns
-	      (choice (const :tag "No modifier" nil)
+	      (choice (const :tag "No modifier (work as layout switch)" none)
 		      (const control) (const meta)
 		      (const alt) (const hyper)
 		      (const super)) "23.1")
 	     (ns-right-command-modifier
 	      ns
-	      (choice (const :tag "No modifier (work as command)" none)
+	      (choice (const :tag "No modifier (work as layout switch)" none)
 		      (const :tag "Use the value of ns-command-modifier"
 			     left)
 		      (const control) (const meta)
diff --git a/src/nsterm.m b/src/nsterm.m
index 4048ac4..ebccb27 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -37,6 +37,7 @@ GNUstep port and post-20 update by Adrian Robert (arobert@cogsci.ucsd.edu)
 #include <time.h>
 #include <signal.h>
 #include <unistd.h>
+#include <stdbool.h>
 
 #include <c-ctype.h>
 #include <c-strcase.h>
@@ -5670,6 +5671,43 @@ not_in_argv (NSString *arg)
 @end  /* EmacsApp */
 
 
+static bool
+is_shift_modifier (NSEventModifierFlags flags,
+                   NSEventModifierFlags generic_mask,
+                   NSEventModifierFlags left_mask,
+                   NSEventModifierFlags right_mask,
+                   Lisp_Object left_option,
+                   Lisp_Object right_option)
+{
+  bool is_right_key = (flags & right_mask) == right_mask;
+  bool is_left_key = (flags & left_mask) == left_mask
+    || (! is_right_key && (flags & generic_mask) == generic_mask);
+
+  unsigned int modifiers = 0;
+  if (is_right_key)
+    modifiers |= parse_solitary_modifier (EQ (right_option, Qleft)
+                                          ? left_option : right_option);
+  if (is_left_key)
+    modifiers |= parse_solitary_modifier (left_option);
+
+  /* parse_solitary_modifier returns 0 for a shift-like modifier.  */
+  return (is_left_key || is_right_key) && modifiers == 0;
+}
+
+static bool
+has_shift_modifiers (NSEventModifierFlags flags)
+{
+  /* Check only Command, Option, and Shift modifiers.  Control is
+     never a shift-like modifier key.  */
+  return is_shift_modifier (flags, NSCommandKeyMask,
+                            NSLeftCommandKeyMask, NSRightCommandKeyMask,
+                            ns_command_modifier, ns_right_command_modifier)
+    || is_shift_modifier (flags, NSAlternateKeyMask,
+                          NSLeftAlternateKeyMask, NSRightAlternateKeyMask,
+                          ns_alternate_modifier, ns_right_alternate_modifier)
+    || (flags & NSShiftKeyMask) == NSShiftKeyMask;
+}
+
 
 /* ==========================================================================
 
@@ -5812,10 +5850,8 @@ not_in_argv (NSString *arg)
 
   if (!processingCompose)
     {
-      /* When using screen sharing, no left or right information is sent,
-         so use Left key in those cases.  */
-      int is_left_key, is_right_key;
-
+      /* FIXME: What should happen for key sequences with more than
+         one character?  */
       code = ([[theEvent charactersIgnoringModifiers] length] == 0) ?
         0 : [[theEvent charactersIgnoringModifiers] characterAtIndex: 0];
 
@@ -5862,131 +5898,50 @@ not_in_argv (NSString *arg)
       if (flags & NSShiftKeyMask)
         emacs_event->modifiers |= shift_modifier;
 
-      is_right_key = (flags & NSRightCommandKeyMask) == NSRightCommandKeyMask;
-      is_left_key = (flags & NSLeftCommandKeyMask) == NSLeftCommandKeyMask
-        || (! is_right_key && (flags & NSCommandKeyMask) == NSCommandKeyMask);
+      /* 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
+         translated characters (returned by the characters method); if
+         we have only control-like modifiers, then we should use the
+         untranslated characters (returned by the
+         charactersIgnoringModifiers method).  An annoyance happens if
+         we have both shift-like and control-like modifiers because
+         the NSEvent API doesn’t let us ignore only some modifiers.
+         Therefore we ignore all shift-like modifiers in that
+         case.  */
+
+      /* EV_MODIFIERS2 uses parse_solitary_modifier on all known
+         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);
+      bool shift_modifiers =
+        control_modifiers ? false : has_shift_modifiers (flags);
+
+      emacs_event->modifiers |= control_modifiers;
 
-      if (is_right_key)
-        emacs_event->modifiers |= parse_solitary_modifier
-          (EQ (ns_right_command_modifier, Qleft)
-           ? ns_command_modifier
-           : ns_right_command_modifier);
-
-      if (is_left_key)
-        {
-          emacs_event->modifiers |= parse_solitary_modifier
-            (ns_command_modifier);
-
-          /* if super (default), take input manager's word so things like
-             dvorak / qwerty layout work */
-          if (EQ (ns_command_modifier, Qsuper)
-              && !fnKeysym
-              && [[theEvent characters] length] != 0)
-            {
-              /* XXX: the code we get will be unshifted, so if we have
-                 a shift modifier, must convert ourselves */
-              if (!(flags & NSShiftKeyMask))
-                code = [[theEvent characters] characterAtIndex: 0];
-#if 0
-              /* this is ugly and also requires linking w/Carbon framework
-                 (for LMGetKbdType) so for now leave this rare (?) case
-                 undealt with.. in future look into CGEvent methods */
-              else
-                {
-                  long smv = GetScriptManagerVariable (smKeyScript);
-                  Handle uchrHandle = GetResource
-                    ('uchr', GetScriptVariable (smv, smScriptKeys));
-                  UInt32 dummy = 0;
-                  UCKeyTranslate ((UCKeyboardLayout*)*uchrHandle,
-                                 [[theEvent characters] characterAtIndex: 0],
-                                 kUCKeyActionDisplay,
-                                 (flags & ~NSCommandKeyMask) >> 8,
-                                 LMGetKbdType (), kUCKeyTranslateNoDeadKeysMask,
-                                 &dummy, 1, &dummy, &code);
-                  code &= 0xFF;
-                }
-#endif
-            }
-        }
-
-      is_right_key = (flags & NSRightControlKeyMask) == NSRightControlKeyMask;
-      is_left_key = (flags & NSLeftControlKeyMask) == NSLeftControlKeyMask
-        || (! is_right_key && (flags & NSControlKeyMask) == NSControlKeyMask);
-
-      if (is_right_key)
-          emacs_event->modifiers |= parse_solitary_modifier
-              (EQ (ns_right_control_modifier, Qleft)
-               ? ns_control_modifier
-               : ns_right_control_modifier);
-
-      if (is_left_key)
-        emacs_event->modifiers |= parse_solitary_modifier
-          (ns_control_modifier);
-
-      if (flags & NS_FUNCTION_KEY_MASK && !fnKeysym)
-          emacs_event->modifiers |=
-            parse_solitary_modifier (ns_function_modifier);
-
-      left_is_none = NILP (ns_alternate_modifier)
-        || EQ (ns_alternate_modifier, Qnone);
-
-      is_right_key = (flags & NSRightAlternateKeyMask)
-        == NSRightAlternateKeyMask;
-      is_left_key = (flags & NSLeftAlternateKeyMask) == NSLeftAlternateKeyMask
-        || (! is_right_key
-            && (flags & NSAlternateKeyMask) == NSAlternateKeyMask);
-
-      if (is_right_key)
-        {
-          if ((NILP (ns_right_alternate_modifier)
-               || EQ (ns_right_alternate_modifier, Qnone)
-               || (EQ (ns_right_alternate_modifier, Qleft) && left_is_none))
-              && !fnKeysym)
-            {   /* accept pre-interp alt comb */
-              if ([[theEvent characters] length] > 0)
-                code = [[theEvent characters] characterAtIndex: 0];
-              /*HACK: clear lone shift modifier to stop next if from firing */
-              if (emacs_event->modifiers == shift_modifier)
-                emacs_event->modifiers = 0;
-            }
-          else
-            emacs_event->modifiers |= parse_solitary_modifier
-              (EQ (ns_right_alternate_modifier, Qleft)
-               ? ns_alternate_modifier
-               : ns_right_alternate_modifier);
-        }
-
-      if (is_left_key) /* default = meta */
-        {
-          if (left_is_none && !fnKeysym)
-            {   /* accept pre-interp alt comb */
-              if ([[theEvent characters] length] > 0)
-                code = [[theEvent characters] characterAtIndex: 0];
-              /*HACK: clear lone shift modifier to stop next if from firing */
-              if (emacs_event->modifiers == shift_modifier)
-                emacs_event->modifiers = 0;
-            }
-          else
-              emacs_event->modifiers |=
-                parse_solitary_modifier (ns_alternate_modifier);
-        }
-
-  if (NS_KEYLOG)
-    fprintf (stderr, "keyDown: code =%x\tfnKey =%x\tflags = %x\tmods = %x\n",
-             code, fnKeysym, flags, emacs_event->modifiers);
+      if (NS_KEYLOG)
+        fprintf (stderr, "keyDown: code =%x\tfnKey =%x\tflags = %x\tmods = %x\n",
+                 code, fnKeysym, flags, emacs_event->modifiers);
 
-      /* if it was a function key or had modifiers, pass it directly to emacs */
+      /* If it was a function key or had control-like modifiers, pass
+         it directly to Emacs.  */
       if (fnKeysym || (emacs_event->modifiers
                        && (emacs_event->modifiers != shift_modifier)
                        && [[theEvent charactersIgnoringModifiers] length] > 0))
 /*[[theEvent characters] length] */
         {
           emacs_event->kind = NON_ASCII_KEYSTROKE_EVENT;
+          /* FIXME: What are the next four lines supposed to do?  */
           if (code < 0x20)
             code |= (1<<28)|(3<<16);
           else if (code == 0x7f)
             code |= (1<<28)|(3<<16);
           else if (!fnKeysym)
+            /* FIXME: This seems wrong, characters in the range
+               [0x80, 0xFF] are not ASCII characters.  Can’t we just
+               use MULTIBYTE_CHAR_KEYSTROKE_EVENT here for all kinds
+               of characters?  */
             emacs_event->kind = code > 0xFF
               ? MULTIBYTE_CHAR_KEYSTROKE_EVENT : ASCII_KEYSTROKE_EVENT;
 
@@ -5997,11 +5952,32 @@ not_in_argv (NSString *arg)
         }
     }
 
+  /* If we get here, a non-function key without control-like modifiers
+     was hit.  Use interpretKeyEvents, which in turn will call
+     insertText; see
+     https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventOverview/HandlingKeyEvents/HandlingKeyEvents.html.  */
 
   if (NS_KEYLOG && !processingCompose)
     fprintf (stderr, "keyDown: Begin compose sequence.\n");
 
+  /* FIXME: interpretKeyEvents doesn’t seem to send insertText if ⌘ is
+     used as shift-like modifier, at least on El Capitan.  Mask it
+     out.  This shouldn’t be needed though; we should figure out what
+     the correct way of handling ⌘ is.  */
+  if ([theEvent modifierFlags] & NSCommandKeyMask)
+    theEvent = [NSEvent keyEventWithType:[theEvent type]
+                                location:[theEvent locationInWindow]
+                           modifierFlags:[theEvent modifierFlags] & ~NSCommandKeyMask
+                               timestamp:[theEvent timestamp]
+                            windowNumber:[theEvent windowNumber]
+                                 context:[theEvent context]
+                              characters:[theEvent characters]
+                        charactersIgnoringModifiers:[theEvent charactersIgnoringModifiers]
+                               isARepeat:[theEvent isARepeat]
+                                 keyCode:[theEvent keyCode]];
+
   processingCompose = YES;
+  /* FIXME: Use [NSArray arrayWithObject:theEvent]?  */
   [nsEvArray addObject: theEvent];
   [self interpretKeyEvents: nsEvArray];
   [nsEvArray removeObject: theEvent];
-- 
2.7.4