Merge pull request #8023 from keymanapp/fix/windows/cherry-pick/modif…

…ier-stuck

fix(windows): cherry pick modifier event is always serialized 🍒

windows/src/engine/keyman32/globals.h

@@ -278,9 +278,7 @@ extern UINT
   wm_keymanshift,
   wm_keymanim_close,
   wm_keymanim_contextchanged,
-  wm_test_keyman_functioning,
-  wm_keyman_keyevent;   // for serialized input
-
+  wm_test_keyman_functioning;
 extern BOOL
   flag_ShouldSerializeInput;
 

windows/src/engine/keyman32/k32_dbg.cpp

@@ -44,6 +44,7 @@
                     09 Aug 2015 - mcdurdin - I4843 - Log reported modifier state as well as Keyman current modifier state
 */
 #include "pch.h"
+#include "serialkeyeventcommon.h"
 #include <stdio.h>
 #include <stdarg.h>
 #include <evntprov.h>
@@ -206,8 +207,8 @@ void DebugMessage(LPMSG msg, WPARAM wParam)  // I2908
 	else if(msg->message == wm_keymankeyup)
     wsprintf(ds, "DebugMessage(%x, wm_keymankeyup: %s lParam: %X) [message flags: %x time: %d]", PtrToInt(msg->hwnd),
       Debug_VirtualKey((WORD) msg->wParam), (unsigned int) msg->lParam, wParam, (int) msg->time);
-  else if (msg->message == wm_keyman_keyevent)
-    wsprintf(ds, "DebugMessage(%x, wm_keyman_keyevent: %s lParam: %X) [message flags: %x time: %d]", PtrToInt(msg->hwnd),
+  else if (msg->message == WM_KEYMAN_KEY_EVENT)
+    wsprintf(ds, "DebugMessage(%x, WM_KEYMAN_KEY_EVENT: %s lParam: %X) [message flags: %x time: %d]", PtrToInt(msg->hwnd),
       Debug_VirtualKey((WORD)msg->wParam), (unsigned int) msg->lParam, wParam, (int) msg->time);
   else if(msg->message == WM_KEYDOWN || msg->message == WM_KEYUP || msg->message == WM_SYSKEYDOWN || msg->message == WM_SYSKEYUP)
     wsprintf(ds, "DebugMessage(%x, %s, wParam: %s, lParam: %X) [message flags: %x time: %d extra: %x]",

windows/src/engine/keyman32/k32_globals.cpp

@@ -72,7 +72,6 @@
 UINT
   //TODO: consolidate these messages -- they are probably not all required now
   wm_keyman = 0,						// user message - ignore msg   // I3594
-  wm_keyman_keyevent = 0,   // for serialized input
 	wm_kmdebug = 0,						//  " "  "  "   - debugging
 
 	wm_keymankeydown = 0,

windows/src/engine/keyman32/k32_lowlevelkeyboardhook.cpp

@@ -59,6 +59,22 @@ LRESULT CALLBACK kmnLowLevelKeyboardProc(
   return res;
 }
 
+BOOL isModifierKey(DWORD vkCode) {
+  switch (vkCode) {
+    case VK_LCONTROL:
+    case VK_RCONTROL:
+    case VK_CONTROL:
+    case VK_LMENU:
+    case VK_RMENU:
+    case VK_MENU:
+    case VK_LSHIFT:
+    case VK_RSHIFT:
+    case VK_SHIFT:
+      return TRUE;
+  }
+  return FALSE;
+}
+
 BOOL KeyLanguageSwitchPress(WPARAM wParam, BOOL extended, BOOL isUp, DWORD ShiftState);
 int ProcessLanguageSwitchShiftKey(WPARAM wParam, BOOL isUp);
 BOOL IsLanguageSwitchWindowVisible();
@@ -203,6 +219,15 @@ LRESULT _kmnLowLevelKeyboardProc(
     else FHotkeyShiftState |= Flag;
   }
 
+  // #7337 Post the modifier state ensuring the serialized queue is in sync
+  // Note that the modifier key may be posted again with WM_KEYMAN_KEY_EVENT,
+  // later in this function. This is intentional, as the WM_KEYMAN_MODIFIER_EVENT
+  // message only updates our internal modifier state, and does not do
+  // any additional processing or other serialization of the input queue.
+  if (isModifierKey(hs->vkCode) && flag_ShouldSerializeInput) {
+      PostMessage(ISerialKeyEventServer::GetServer()->GetWindow(), WM_KEYMAN_MODIFIER_EVENT, hs->vkCode, LLKHFFlagstoWMKeymanKeyEventFlags(hs));
+  }
+
   if(IsLanguageSwitchWindowVisible()) {
     SendDebugMessageFormat(0, sdmAIDefault, 0, "kmnLowLevelKeyboardProc: Sending to language switch window %x %x", wParam, lParam);
     SendToLanguageSwitchWindow(hs->vkCode, hs->flags);
@@ -222,10 +247,10 @@ LRESULT _kmnLowLevelKeyboardProc(
 
   */
 
-  if (hs->dwExtraInfo != 0 || 
+  if (hs->dwExtraInfo != 0 ||
       hs->scanCode == SCAN_FLAG_KEYMAN_KEY_EVENT ||
-      hs->vkCode == VK_PROCESSKEY || 
-      hs->vkCode == VK_PACKET || 
+      hs->vkCode == VK_PROCESSKEY ||
+      hs->vkCode == VK_PACKET ||
       !isKeymanKeyboardActive) {
     // This key event was generated by Keyman, so pass it through
     // dwExtraInfo is set to 0x4321DCBA by mstsc which does prefiltering. So we ignore for anything where dwExtraInfo!=0 because it
@@ -250,7 +275,7 @@ LRESULT _kmnLowLevelKeyboardProc(
 
       HWND hwnd = gui.hwndFocus ? gui.hwndFocus : gui.hwndActive;
       if (!IsConsoleWindow(hwnd)) {
-        PostMessage(ISerialKeyEventServer::GetServer()->GetWindow(), wm_keyman_keyevent, hs->vkCode, LLKHFFlagstoWMKeymanKeyEventFlags(hs));
+        PostMessage(ISerialKeyEventServer::GetServer()->GetWindow(), WM_KEYMAN_KEY_EVENT, hs->vkCode, LLKHFFlagstoWMKeymanKeyEventFlags(hs));
         return 1;
       }
       //else SendDebugMessageFormat(0, sdmGlobal, 0, "LowLevelHook: console window, not serializing"); // too noisy

windows/src/engine/keyman32/keyman32.cpp

@@ -208,17 +208,16 @@ void DoChangeWindowMessageFilter()
   }
 
 	DoCWMF(wm_keyman);   // I3594
-  DoCWMF(wm_keyman_keyevent);
-    DoCWMF(wm_keymankeydown);
-    DoCWMF(wm_keymankeyup);
-    DoCWMF(wm_keyman_grabwindowproc);
-    DoCWMF(wm_keyman_refresh);
-    DoCWMF(wm_kmgetactivekeymanid);
-    DoCWMF(wm_keymanim_close);
-    DoCWMF(wm_keymanim_contextchanged);
-    DoCWMF(wm_keymanshift);
-    DoCWMF(wm_keyman_control);   // I4714
-    DoCWMF(wm_keyman_control_internal);   // I4714
+  DoCWMF(wm_keymankeydown);
+  DoCWMF(wm_keymankeyup);
+  DoCWMF(wm_keyman_grabwindowproc);
+  DoCWMF(wm_keyman_refresh);
+  DoCWMF(wm_kmgetactivekeymanid);
+  DoCWMF(wm_keymanim_close);
+  DoCWMF(wm_keymanim_contextchanged);
+  DoCWMF(wm_keymanshift);
+  DoCWMF(wm_keyman_control);   // I4714
+  DoCWMF(wm_keyman_control_internal);   // I4714
 
     FreeLibrary(hUser32);
 }
@@ -300,7 +299,6 @@ BOOL InitialiseProcess(HWND hwnd)
   Initialise_Flag_ShouldSerializeInput();
 
 	wm_keyman = RegisterWindowMessage(RWM_KEYMAN);
-  wm_keyman_keyevent = RegisterWindowMessage("WM_KEYMAN_KEYEVENT");
 	wm_keymankeydown = RegisterWindowMessage("WM_KEYMANKEYDOWN");
 	wm_keymankeyup = RegisterWindowMessage("WM_KEYMANKEYUP");
 	wm_keyman_grabwindowproc = RegisterWindowMessage("WM_KEYMAN_GRABWINDOWPROC");

windows/src/engine/keyman32/kmhook_getmessage.cpp

@@ -63,6 +63,7 @@
 */
    // I3583   // I4287
 #include "pch.h"
+#include "serialkeyeventcommon.h"
 
 void ProcessWMKeymanControlInternal(HWND hwnd, WPARAM wParam, LPARAM lParam);
 void ProcessWMKeymanControl(WPARAM wParam, LPARAM lParam);
@@ -141,7 +142,7 @@ LRESULT _kmnGetMessageProc(int nCode, WPARAM wParam, LPARAM lParam)
   }
 
   if (((mp->message >= WM_KEYFIRST && mp->message <= WM_KEYLAST) || mp->message == wm_keymankeydown || mp->message == wm_keymankeyup ||
-    mp->message == wm_keyman_keyevent)
+    mp->message == WM_KEYMAN_KEY_EVENT)
     && ShouldDebug(sdmMessage)) {
     DebugMessage(mp, wParam);
   }

windows/src/engine/keyman32/serialkeyeventcommon.h

@@ -16,6 +16,12 @@
 #define GLOBAL_KEY_EVENT_NAME "KeymanEngine_KeyEvent"
 #define GLOBAL_KEY_MUTEX_NAME "KeymanEngine_KeyMutex"
 
+/**
+WM_USER private messages -- used only for communication 
+between low level keyboard hook and serial key event server
+*/
+#define WM_KEYMAN_KEY_EVENT (WM_USER + 1)
+#define WM_KEYMAN_MODIFIER_EVENT (WM_USER + 2)
 /**
   The INPUT structure and the KEYBDINPUT structure both vary in size between x86 and x64
   because of the presence of the ULONG_PTR member dwExtraInfo. Thus we need to maintain an
@@ -34,3 +40,5 @@ struct SerialKeyEventSharedData {
   DWORD nInputs;
   CSDINPUT inputs[MAX_KEYEVENT_INPUTS];
 };
+
+

windows/src/engine/keyman32/serialkeyeventserver.cpp

@@ -11,21 +11,21 @@
   due to restricted permissions, all SendInput must be done from this thread, which runs
   in the Keyman main process.
 
-  NOTE: Postponing writing architecture technical note because of change to architecture 
+  NOTE: Postponing writing architecture technical note because of change to architecture
   below...
 
-  TODO: For simplicity of proof-of-concept data sharing, we ran two copies of the key event 
-  sender thread: one in the 32 bit space, and one in the 64 bit space. This means that we 
+  TODO: For simplicity of proof-of-concept data sharing, we ran two copies of the key event
+  sender thread: one in the 32 bit space, and one in the 64 bit space. This means that we
   can still have a race condition because we lose serialization guarantees. Input is first
   processed in the Low Level Keyboard Hook which runs in the keyman.exe 32 bit space. This
   then gets forwarded to the target application with the necessary flags on the message to
-  tell Keyman to not reprocess it. However, after keystroke processing, the target 
-  application fills the shared data structure and signals the key event sender thread in its 
-  own bitness space (32 or 64 bit). The key event sender thread then takes the final shared 
-  data and sends it to the target app. And that breaks the serialization guarantee because 
+  tell Keyman to not reprocess it. However, after keystroke processing, the target
+  application fills the shared data structure and signals the key event sender thread in its
+  own bitness space (32 or 64 bit). The key event sender thread then takes the final shared
+  data and sends it to the target app. And that breaks the serialization guarantee because
   the 64 bit apps are not serialized with the original 32 bit captured input.
 
-  The fix is to redesign the shared data to use a memory mapped file, which can be shared 
+  The fix is to redesign the shared data to use a memory mapped file, which can be shared
   across the 32-64 boundary. Must tweak the permissions on this file, of course.
 
   TODO: Console apps still not working
@@ -73,7 +73,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
     m_pInputs = NULL;
     m_pSharedData = NULL;
 
-    // We create the file mapping and global data on the main thread but release it on the 
+    // We create the file mapping and global data on the main thread but release it on the
     // local thread. This ensures that these objects are available for other processes to
     // open even if we haven't completed startup of the local thread.
     if (!InitSharedData()) {
@@ -117,7 +117,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
 
     // Normally, this is cleaned up by thread termination, but this
     // handles error conditions better
-    CloseSharedData(); 
+    CloseSharedData();
   }
 
   virtual HWND GetWindow() const {
@@ -295,7 +295,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
 
   /**
     Main message loop for thread. Terminates on error or when
-    m_hThreadExitEvent is signaled. Sleeps until either a 
+    m_hThreadExitEvent is signaled. Sleeps until either a
     window message is received or a key event is signaled from
     a client app.
   */
@@ -332,7 +332,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
     HANDLE handles[2] = { m_hThreadExitEvent, m_hKeyMutex };
 
     //
-    // Wait for access to the shared data (must also watch out for 
+    // Wait for access to the shared data (must also watch out for
     // shutdown event so we don't stall forever here)
     //
     switch (WaitForMultipleObjects(2, handles, FALSE, INFINITE)) {
@@ -377,7 +377,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
   }
 
   /**
-    Add modifier state adjustment events and then copy the new input 
+    Add modifier state adjustment events and then copy the new input
     events from the shared buffer
   */
   void PrepareInjectedInput() {
@@ -406,7 +406,7 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
     if (server == NULL) {
       return DefWindowProc(hwnd, msg, wParam, lParam);
     }
-      
+
     return server->WndProc(hwnd, msg, wParam, lParam);
   }
 
@@ -436,15 +436,15 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
       You can disable this flag with flag_ShouldSerializeInput.
     */
 
-    if (msg == wm_keyman_keyevent && flag_ShouldSerializeInput  /*&& _td->lpActiveKeyboard*/) {
+    if ((msg == WM_KEYMAN_KEY_EVENT || msg == WM_KEYMAN_MODIFIER_EVENT) && flag_ShouldSerializeInput  /*&& _td->lpActiveKeyboard*/) {
 
       if (wParam == VK_RMENU && (lParam & (KEYEVENTF_EXTENDEDKEY | KEYEVENTF_KEYUP)) == (KEYEVENTF_EXTENDEDKEY | KEYEVENTF_KEYUP) && GetKeyState(VK_LCONTROL) < 0) {
-        /* 
-          When Windows has a European layout that uses AltGr installed, it can emit an additional LCtrl down via software 
+        /*
+          When Windows has a European layout that uses AltGr installed, it can emit an additional LCtrl down via software
           when RAlt is pressed. However, the corresponding LCtrl up is never received, seemingly because when Keyman
           re-emits the LCtrl+RAlt, there are subtle differences in the event flags which we cannot duplicate -- specifically
-          the flag that emits WM_SYSKEYDOWN for the VK_LCONTROL, even though it is received before the VK_RALT event. It 
-          appears that Windows figures this out by giving this VK_LCONTROL the scan code 0x21D instead of 0x1D. But we 
+          the flag that emits WM_SYSKEYDOWN for the VK_LCONTROL, even though it is received before the VK_RALT event. It
+          appears that Windows figures this out by giving this VK_LCONTROL the scan code 0x21D instead of 0x1D. But we
           are unable to emit that scan code: Windows truncates the scan code sent through SendInput so that we can only
           send 0x1D.
 
@@ -485,8 +485,13 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
         input[1].ki.dwExtraInfo = EXTRAINFO_FLAG_SERIALIZED_USER_KEY_EVENT;
         input[1].ki.dwFlags = lParam & 0xFFFF;
 
-        if (!SendInput(2, input, sizeof(INPUT))) {
-          DebugLastError("SendInput");
+        if (msg == WM_KEYMAN_KEY_EVENT) {
+          // We track changes to modifiers with WM_KEYMAN_MODIFIER_EVENT, but only ever
+          // pass them on to the app when we receive them with the WM_KEYMAN_KEY_EVENT
+          // message.
+          if (!SendInput(2, input, sizeof(INPUT))) {
+            DebugLastError("SendInput");
+          }
         }
 
         UpdateLocalModifierState(
@@ -510,8 +515,10 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
         input.ki.dwExtraInfo = EXTRAINFO_FLAG_SERIALIZED_USER_KEY_EVENT;
         input.ki.dwFlags = lParam & 0xFFFF;
 
-        if (!SendInput(1, &input, sizeof(INPUT))) {
-          DebugLastError("SendInput");
+        if (msg == WM_KEYMAN_KEY_EVENT){
+          if (!SendInput(1, &input, sizeof(INPUT))) {
+            DebugLastError("SendInput");
+          }
         }
 
         UpdateLocalModifierState(
@@ -527,10 +534,10 @@ class SerialKeyEventServer: public ISerialKeyEventServer {
   }
 
   /**
-   When a physical key event is received by the serializer, we know that this will 
-   reflect the key state that the app sees at the time that the input is sent. 
+   When a physical key event is received by the serializer, we know that this will
+   reflect the key state that the app sees at the time that the input is sent.
    We maintain a local modifier state here rather than using GetKeyState because that
-   ensures that we are keeping the keyboard state consistent with our version of 
+   ensures that we are keeping the keyboard state consistent with our version of
    reality.
   */
   void UpdateLocalModifierState(BYTE bVk, BOOL fIsExtendedKey, BYTE bScan, BOOL fIsUp) {