From 7db5d6a307e7caf027fa9d652f28e34498b22b6b Mon Sep 17 00:00:00 2001 From: Arjun Balgovind <32061677+arjunbalgovind@users.noreply.github.com> Date: Fri, 10 Jul 2020 17:53:41 -0700 Subject: [PATCH] [Keyboard Manager] Fixed app-specific shortcut causing app to lose focus scenario (#4902) * Fixed focus issue and added tests * Changed key names * Use constant instead of hardcoded empty string --- .../common/KeyboardManagerConstants.h | 3 + .../common/KeyboardManagerState.cpp | 14 ++- .../common/KeyboardManagerState.h | 9 ++ .../dll/KeyboardEventHandlers.cpp | 52 +++++++++-- .../dll/KeyboardEventHandlers.h | 2 +- .../AppSpecificShortcutRemappingTests.cpp | 91 ++++++++++++++++++- .../keyboardmanager/test/TestHelpers.cpp | 7 ++ 7 files changed, 164 insertions(+), 14 deletions(-) diff --git a/src/modules/keyboardmanager/common/KeyboardManagerConstants.h b/src/modules/keyboardmanager/common/KeyboardManagerConstants.h index 6f9a7f7c8e..39e802f2d4 100644 --- a/src/modules/keyboardmanager/common/KeyboardManagerConstants.h +++ b/src/modules/keyboardmanager/common/KeyboardManagerConstants.h @@ -96,4 +96,7 @@ namespace KeyboardManagerConstants // String constant for the default app name in Remap shortcuts inline const std::wstring DefaultAppName = L"All Apps"; + + // String constant to represent no activated application in app-specific shortcuts + inline const std::wstring NoActivatedApp = L""; } \ No newline at end of file diff --git a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp b/src/modules/keyboardmanager/common/KeyboardManagerState.cpp index 55ce692445..fbf9d0d952 100644 --- a/src/modules/keyboardmanager/common/KeyboardManagerState.cpp +++ b/src/modules/keyboardmanager/common/KeyboardManagerState.cpp @@ -528,4 +528,16 @@ std::wstring KeyboardManagerState::GetCurrentConfigName() { std::lock_guard lock(currentConfig_mutex); return currentConfig; -} \ No newline at end of file +} + +// Sets the activated target application in app-specfic shortcut +void KeyboardManagerState::SetActivatedApp(const std::wstring& appName) +{ + activatedAppSpecificShortcutTarget = appName; +} + +// Gets the activated target application in app-specfic shortcut +std::wstring KeyboardManagerState::GetActivatedApp() +{ + return activatedAppSpecificShortcutTarget; +} diff --git a/src/modules/keyboardmanager/common/KeyboardManagerState.h b/src/modules/keyboardmanager/common/KeyboardManagerState.h index 3344050963..d04a9be102 100644 --- a/src/modules/keyboardmanager/common/KeyboardManagerState.h +++ b/src/modules/keyboardmanager/common/KeyboardManagerState.h @@ -71,6 +71,9 @@ private: std::map> keyDelays; std::mutex keyDelays_mutex; + // Stores the activated target application in app-specfic shortcut + std::wstring activatedAppSpecificShortcutTarget; + // Display a key by appending a border Control as a child of the panel. void AddKeyToLayout(const StackPanel& panel, const winrt::hstring& key); @@ -190,4 +193,10 @@ public: // Gets the Current Active Configuration Name. std::wstring GetCurrentConfigName(); + + // Sets the activated target application in app-specfic shortcut + void SetActivatedApp(const std::wstring& appName); + + // Gets the activated target application in app-specfic shortcut + std::wstring GetActivatedApp(); }; diff --git a/src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp b/src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp index 547ef87451..2dca529ada 100644 --- a/src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp +++ b/src/modules/keyboardmanager/dll/KeyboardEventHandlers.cpp @@ -114,7 +114,7 @@ namespace KeyboardEventHandlers } // Function to a handle a shortcut remap - __declspec(dllexport) intptr_t HandleShortcutRemapEvent(InputInterface& ii, LowlevelKeyboardEvent* data, std::map& reMap, std::mutex& map_mutex) noexcept + __declspec(dllexport) intptr_t HandleShortcutRemapEvent(InputInterface& ii, LowlevelKeyboardEvent* data, std::map& reMap, std::mutex& map_mutex, KeyboardManagerState& keyboardManagerState, const std::wstring& activatedApp) noexcept { // The mutex should be unlocked before SendInput is called to avoid re-entry into the same mutex. More details can be found at https://github.com/microsoft/PowerToys/pull/1789#issuecomment-607555837 std::unique_lock lock(map_mutex); @@ -260,6 +260,11 @@ namespace KeyboardEventHandlers } it.second.isShortcutInvoked = true; + // If app specific shortcut is invoked, store the target application + if (activatedApp != KeyboardManagerConstants::NoActivatedApp) + { + keyboardManagerState.SetActivatedApp(activatedApp); + } lock.unlock(); UINT res = ii.SendVirtualInput((UINT)key_count, keyEventList, sizeof(INPUT)); delete[] keyEventList; @@ -359,6 +364,11 @@ namespace KeyboardEventHandlers it.second.isShortcutInvoked = false; it.second.winKeyInvoked = ModifierKey::Disabled; + // If app specific shortcut has finished invoking, reset the target application + if (activatedApp != KeyboardManagerConstants::NoActivatedApp) + { + keyboardManagerState.SetActivatedApp(KeyboardManagerConstants::NoActivatedApp); + } lock.unlock(); // key count can be 0 if both shortcuts have same modifiers and the action key is not held down. delete will throw an error if keyEventList is empty @@ -559,6 +569,11 @@ namespace KeyboardEventHandlers it.second.isShortcutInvoked = false; it.second.winKeyInvoked = ModifierKey::Disabled; + // If app specific shortcut has finished invoking, reset the target application + if (activatedApp != KeyboardManagerConstants::NoActivatedApp) + { + keyboardManagerState.SetActivatedApp(KeyboardManagerConstants::NoActivatedApp); + } lock.unlock(); UINT res = ii.SendVirtualInput((UINT)key_count, keyEventList, sizeof(INPUT)); delete[] keyEventList; @@ -571,6 +586,11 @@ namespace KeyboardEventHandlers // If it was in isShortcutInvoked state and none of the above cases occur, then reset the flags it.second.isShortcutInvoked = false; it.second.winKeyInvoked = ModifierKey::Disabled; + // If app specific shortcut has finished invoking, reset the target application + if (activatedApp != KeyboardManagerConstants::NoActivatedApp) + { + keyboardManagerState.SetActivatedApp(KeyboardManagerConstants::NoActivatedApp); + } } } @@ -583,7 +603,7 @@ namespace KeyboardEventHandlers // Check if the key event was generated by KeyboardManager to avoid remapping events generated by us. if (data->lParam->dwExtraInfo != KeyboardManagerConstants::KEYBOARDMANAGER_SHORTCUT_FLAG) { - bool result = HandleShortcutRemapEvent(ii, data, keyboardManagerState.osLevelShortcutReMap, keyboardManagerState.osLevelShortcutReMap_mutex); + bool result = HandleShortcutRemapEvent(ii, data, keyboardManagerState.osLevelShortcutReMap, keyboardManagerState.osLevelShortcutReMap_mutex, keyboardManagerState); return result; } @@ -614,22 +634,34 @@ namespace KeyboardEventHandlers std::transform(process_name.begin(), process_name.end(), process_name.begin(), towlower); std::unique_lock lock(keyboardManagerState.appSpecificShortcutReMap_mutex); - std::wstring query_string = process_name; - auto it = keyboardManagerState.appSpecificShortcutReMap.find(query_string); + std::wstring query_string; - // If no entry is found, search for the process name without it's file extension - if (it == keyboardManagerState.appSpecificShortcutReMap.end()) + std::map>::iterator it; + // Check if an app-specific shortcut is already activated + if (keyboardManagerState.GetActivatedApp() == KeyboardManagerConstants::NoActivatedApp) { - // Find index of the file extension - size_t extensionIndex = process_name.find_last_of(L"."); - query_string = process_name.substr(0, extensionIndex); + query_string = process_name; + it = keyboardManagerState.appSpecificShortcutReMap.find(query_string); + + // If no entry is found, search for the process name without it's file extension + if (it == keyboardManagerState.appSpecificShortcutReMap.end()) + { + // Find index of the file extension + size_t extensionIndex = process_name.find_last_of(L"."); + query_string = process_name.substr(0, extensionIndex); + it = keyboardManagerState.appSpecificShortcutReMap.find(query_string); + } + } + else + { + query_string = keyboardManagerState.GetActivatedApp(); it = keyboardManagerState.appSpecificShortcutReMap.find(query_string); } if (it != keyboardManagerState.appSpecificShortcutReMap.end()) { lock.unlock(); - bool result = HandleShortcutRemapEvent(ii, data, keyboardManagerState.appSpecificShortcutReMap[query_string], keyboardManagerState.appSpecificShortcutReMap_mutex); + bool result = HandleShortcutRemapEvent(ii, data, keyboardManagerState.appSpecificShortcutReMap[query_string], keyboardManagerState.appSpecificShortcutReMap_mutex, keyboardManagerState, query_string); return result; } } diff --git a/src/modules/keyboardmanager/dll/KeyboardEventHandlers.h b/src/modules/keyboardmanager/dll/KeyboardEventHandlers.h index 2084c66d63..bd6450ccc3 100644 --- a/src/modules/keyboardmanager/dll/KeyboardEventHandlers.h +++ b/src/modules/keyboardmanager/dll/KeyboardEventHandlers.h @@ -12,7 +12,7 @@ namespace KeyboardEventHandlers __declspec(dllexport) intptr_t HandleSingleKeyToggleToModEvent(InputInterface& ii, LowlevelKeyboardEvent* data, KeyboardManagerState& keyboardManagerState) noexcept; // Function to a handle a shortcut remap - __declspec(dllexport) intptr_t HandleShortcutRemapEvent(InputInterface& ii, LowlevelKeyboardEvent* data, std::map& reMap, std::mutex& map_mutex) noexcept; + __declspec(dllexport) intptr_t HandleShortcutRemapEvent(InputInterface& ii, LowlevelKeyboardEvent* data, std::map& reMap, std::mutex& map_mutex, KeyboardManagerState& keyboardManagerState, const std::wstring& activatedApp = KeyboardManagerConstants::NoActivatedApp) noexcept; // Function to a handle an os-level shortcut remap __declspec(dllexport) intptr_t HandleOSLevelShortcutRemapEvent(InputInterface& ii, LowlevelKeyboardEvent* data, KeyboardManagerState& keyboardManagerState) noexcept; diff --git a/src/modules/keyboardmanager/test/AppSpecificShortcutRemappingTests.cpp b/src/modules/keyboardmanager/test/AppSpecificShortcutRemappingTests.cpp index def6f6048d..4ff6b98768 100644 --- a/src/modules/keyboardmanager/test/AppSpecificShortcutRemappingTests.cpp +++ b/src/modules/keyboardmanager/test/AppSpecificShortcutRemappingTests.cpp @@ -15,8 +15,8 @@ namespace RemappingLogicTests private: MockedInput mockedInputHandler; KeyboardManagerState testState; - std::wstring testApp1 = L"TestProcess1.exe"; - std::wstring testApp2 = L"TestProcess2.exe"; + std::wstring testApp1 = L"testtrocess1.exe"; + std::wstring testApp2 = L"testprocess2.exe"; public: TEST_METHOD_INITIALIZE(InitializeTestEnv) @@ -92,5 +92,92 @@ namespace RemappingLogicTests Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(VK_MENU), false); Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(0x56), false); } + + // Test if the the keyboard manager state's activated app is correctly set after an app specific remap takes place + TEST_METHOD (AppSpecificShortcut_ShouldSetCorrectActivatedApp_WhenRemapOccurs) + { + // Remap Ctrl+A to Alt+V + Shortcut src; + src.SetKey(VK_CONTROL); + src.SetKey(0x41); + Shortcut dest; + dest.SetKey(VK_MENU); + dest.SetKey(0x56); + testState.AddAppSpecificShortcut(testApp1, src, dest); + + // Set the testApp as the foreground process + mockedInputHandler.SetForegroundProcess(testApp1); + + const int nInputs = 2; + INPUT input[nInputs] = {}; + input[0].type = INPUT_KEYBOARD; + input[0].ki.wVk = VK_CONTROL; + input[1].type = INPUT_KEYBOARD; + input[1].ki.wVk = 0x41; + + // Send Ctrl+A keydown + mockedInputHandler.SendVirtualInput(nInputs, input, sizeof(INPUT)); + + // Activated app should be testApp1 + Assert::AreEqual(testApp1, testState.GetActivatedApp()); + + input[0].type = INPUT_KEYBOARD; + input[0].ki.wVk = 0x41; + input[0].ki.dwFlags = KEYEVENTF_KEYUP; + input[1].type = INPUT_KEYBOARD; + input[1].ki.wVk = VK_CONTROL; + input[1].ki.dwFlags = KEYEVENTF_KEYUP; + + // Release A then Ctrl + mockedInputHandler.SendVirtualInput(nInputs, input, sizeof(INPUT)); + + // Activated app should be empty string + Assert::AreEqual(std::wstring(KeyboardManagerConstants::NoActivatedApp), testState.GetActivatedApp()); + } + // Test if the key states get cleared if foreground app changes after app-specific shortcut is invoked and then released + TEST_METHOD (AppSpecificShortcut_ShouldClearKeyStates_WhenForegroundAppChangesAfterShortcutIsPressedOnRelease) + { + // Remap Ctrl+A to Alt+Tab + Shortcut src; + src.SetKey(VK_CONTROL); + src.SetKey(0x41); + Shortcut dest; + dest.SetKey(VK_MENU); + dest.SetKey(VK_TAB); + testState.AddAppSpecificShortcut(testApp1, src, dest); + + // Set the testApp as the foreground process + mockedInputHandler.SetForegroundProcess(testApp1); + + const int nInputs = 2; + INPUT input[nInputs] = {}; + input[0].type = INPUT_KEYBOARD; + input[0].ki.wVk = VK_CONTROL; + input[1].type = INPUT_KEYBOARD; + input[1].ki.wVk = 0x41; + + // Send Ctrl+A keydown + mockedInputHandler.SendVirtualInput(nInputs, input, sizeof(INPUT)); + + // Set the testApp as the foreground process + mockedInputHandler.SetForegroundProcess(testApp2); + + input[0].type = INPUT_KEYBOARD; + input[0].ki.wVk = 0x41; + input[0].ki.dwFlags = KEYEVENTF_KEYUP; + input[1].type = INPUT_KEYBOARD; + input[1].ki.wVk = VK_CONTROL; + input[1].ki.dwFlags = KEYEVENTF_KEYUP; + + // Release A then Ctrl + mockedInputHandler.SendVirtualInput(nInputs, input, sizeof(INPUT)); + + + // Ctrl, A, Alt and Tab should all be false + Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(VK_CONTROL), false); + Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(0x41), false); + Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(VK_MENU), false); + Assert::AreEqual(mockedInputHandler.GetVirtualKeyState(VK_TAB), false); + } }; } diff --git a/src/modules/keyboardmanager/test/TestHelpers.cpp b/src/modules/keyboardmanager/test/TestHelpers.cpp index 028090e8d4..05216b5e31 100644 --- a/src/modules/keyboardmanager/test/TestHelpers.cpp +++ b/src/modules/keyboardmanager/test/TestHelpers.cpp @@ -12,5 +12,12 @@ namespace TestHelpers input.SetForegroundProcess(L""); state.ClearSingleKeyRemaps(); state.ClearOSLevelShortcuts(); + state.ClearAppSpecificShortcuts(); + + // Allocate memory for the keyboardManagerState activatedApp member to avoid CRT assert errors + std::wstring maxLengthString; + maxLengthString.resize(MAX_PATH); + state.SetActivatedApp(maxLengthString); + state.SetActivatedApp(KeyboardManagerConstants::NoActivatedApp); } }