From 5fc29b914e2775f069f22f0b020aef4aded18f68 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 4 Aug 2021 13:43:58 -0500 Subject: [PATCH 1/5] Add more logging that helped identify the source of the issue here --- src/cascadia/Remoting/Monarch.cpp | 27 ++++++++++++++++ src/cascadia/WindowsTerminal/AppHost.cpp | 12 ++++++- src/cascadia/WindowsTerminal/IslandWindow.cpp | 32 ++++++++++++------- src/cascadia/WindowsTerminal/IslandWindow.h | 2 +- 4 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index f65422d3f9d..7a08dfcca8b 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -201,6 +201,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation _clearOldMruEntries(id); } + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_lookupPeasantIdForName", + TraceLoggingWideString(winrt::hstring{ name }.c_str(), "name", "the name we're looking for"), + TraceLoggingUInt64(result, "peasantID", "the ID of the peasant with that name"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); return result; } @@ -750,6 +756,27 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { targetPeasant.Summon(args.SummonBehavior()); args.FoundMatch(true); + + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_SummonWindow_Success", + TraceLoggingWideString(searchedForName.c_str(), "searchedForName", "The name of the window we tried to summon"), + TraceLoggingUInt64(windowId, "peasantID", "The id of the window we tried to summon"), + TraceLoggingBoolean(args.OnCurrentDesktop(), "OnCurrentDesktop", "true iff the window needs to be on the current virtual desktop"), + TraceLoggingBoolean(args.SummonBehavior().MoveToCurrentDesktop(), "MoveToCurrentDesktop", "if true, move the window to the current virtual desktop"), + TraceLoggingBoolean(args.SummonBehavior().ToggleVisibility(), "ToggleVisibility", "true if we should toggle the visibity of the window"), + TraceLoggingUInt32(args.SummonBehavior().DropdownDuration(), "DropdownDuration", "the duration to dropdown the window"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + } + else + { + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_SummonWindow_NoPeasant", + TraceLoggingWideString(searchedForName.c_str(), "searchedForName", "The name of the window we tried to summon"), + TraceLoggingUInt64(windowId, "peasantID", "The id of the window we tried to summon"), + TraceLoggingBoolean(args.OnCurrentDesktop(), "OnCurrentDesktop", "true iff the window needs to be on the current virtual desktop"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } } catch (...) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 092fe8eaeab..4a9dd137797 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -686,7 +686,17 @@ winrt::fire_and_forget AppHost::_setupGlobalHotkeys() { if (auto summonArgs = cmd.ActionAndArgs().Args().try_as()) { - _window->RegisterHotKey(gsl::narrow_cast(_hotkeys.size()), keyChord); + int index = gsl::narrow_cast(_hotkeys.size()); + const bool succeeded = _window->RegisterHotKey(index, keyChord); + + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_setupGlobalHotkey", + TraceLoggingDescription("Emitted when setting a single hotkey"), + TraceLoggingInt64(index, "index", "the index of the hotkey to add"), + TraceLoggingWideString(cmd.Name().c_str(), "name", "the name of the command"), + TraceLoggingBoolean(succeeded, "succeeded", "true if we succeeded"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); _hotkeys.emplace_back(summonArgs); } } diff --git a/src/cascadia/WindowsTerminal/IslandWindow.cpp b/src/cascadia/WindowsTerminal/IslandWindow.cpp index 50fd2d156d6..2a97662a532 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp @@ -1046,8 +1046,9 @@ void IslandWindow::UnregisterHotKey(const int index) noexcept { TraceLoggingWrite( g_hWindowsTerminalProvider, - "UnregisterAllHotKeys", + "UnregisterHotKey", TraceLoggingDescription("Emitted when clearing previously set hotkeys"), + TraceLoggingInt64(index, "index", "the index of the hotkey to remove"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); @@ -1064,15 +1065,8 @@ void IslandWindow::UnregisterHotKey(const int index) noexcept // - hotkey: The key-combination to register. // Return Value: // - -void IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept +bool IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept { - TraceLoggingWrite( - g_hWindowsTerminalProvider, - "RegisterHotKey", - TraceLoggingDescription("Emitted when setting hotkeys"), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), - TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - auto vkey = hotkey.Vkey(); if (!vkey) { @@ -1080,7 +1074,7 @@ void IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Termi } if (!vkey) { - return; + return false; } auto hotkeyFlags = MOD_NOREPEAT; @@ -1094,7 +1088,23 @@ void IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Termi // TODO GH#8888: We should display a warning of some kind if this fails. // This can fail if something else already bound this hotkey. - LOG_IF_WIN32_BOOL_FALSE(::RegisterHotKey(_window.get(), index, hotkeyFlags, vkey)); + const auto result = ::RegisterHotKey(_window.get(), index, hotkeyFlags, vkey); + LOG_IF_WIN32_BOOL_FALSE(result); + + TraceLoggingWrite(g_hWindowsTerminalProvider, + "RegisterHotKey", + TraceLoggingDescription("Emitted when setting hotkeys"), + TraceLoggingInt64(index, "index", "the index of the hotkey to add"), + TraceLoggingUInt64(vkey, "vkey", "the key"), + TraceLoggingUInt64(WI_IsFlagSet(hotkeyFlags, MOD_WIN), "win", "is WIN in the modifiers"), + TraceLoggingUInt64(WI_IsFlagSet(hotkeyFlags, MOD_ALT), "alt", "is ALT in the modifiers"), + TraceLoggingUInt64(WI_IsFlagSet(hotkeyFlags, MOD_CONTROL), "control", "is CONTROL in the modifiers"), + TraceLoggingUInt64(WI_IsFlagSet(hotkeyFlags, MOD_SHIFT), "shift", "is SHIFT in the modifiers"), + TraceLoggingBool(result, "succeeded", "true if we succeeded"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + + return result; } // Method Description: diff --git a/src/cascadia/WindowsTerminal/IslandWindow.h b/src/cascadia/WindowsTerminal/IslandWindow.h index 8e4a0ec4df2..0aebe79a4fc 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.h +++ b/src/cascadia/WindowsTerminal/IslandWindow.h @@ -39,7 +39,7 @@ class IslandWindow : void SetTaskbarProgress(const size_t state, const size_t progress); void UnregisterHotKey(const int index) noexcept; - void RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept; + bool RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept; winrt::fire_and_forget SummonWindow(winrt::Microsoft::Terminal::Remoting::SummonWindowBehavior args); From 57719b90f091f6c1cb064193d6830632629f45e9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 4 Aug 2021 14:42:18 -0500 Subject: [PATCH 2/5] Well that would explain it. --- .../KeyBindingsTests.cpp | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index a5479f6a7f7..dc189449dca 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -41,6 +41,8 @@ namespace SettingsModelLocalTests TEST_METHOD(LayerKeybindings); TEST_METHOD(UnbindKeybindings); + TEST_METHOD(LayerScancodeKeybindings); + TEST_METHOD(TestArbitraryArgs); TEST_METHOD(TestSplitPaneArgs); @@ -105,13 +107,24 @@ namespace SettingsModelLocalTests }, }; + // Use the KeyChordHash and KeyChordEquality to compare if two + // KeyChord's are the same. If you try to just directly compare them + // with VERIFY_ARE_EQUAL, that will always fail. It'll revert to the + // default winrt equality operator, which will compare if they point to + // literally the same object (they won't). + implementation::KeyChordHash hash; + implementation::KeyChordEquality equals; for (const auto& tc : testCases) { + Log::Comment(NoThrowString().Format(L"Testing case:\"%s\"", tc.expected.data())); + KeyChord expectedKeyChord{ tc.modifiers, tc.vkey, tc.scanCode }; const auto actualString = KeyChordSerialization::ToString(expectedKeyChord); VERIFY_ARE_EQUAL(tc.expected, actualString); + const auto actualKeyChord = KeyChordSerialization::FromString(actualString); - VERIFY_ARE_EQUAL(expectedKeyChord, actualKeyChord); + VERIFY_ARE_EQUAL(hash(expectedKeyChord), hash(actualKeyChord)); + VERIFY_IS_TRUE(equals(expectedKeyChord, actualKeyChord)); } } @@ -722,4 +735,31 @@ namespace SettingsModelLocalTests VerifyKeyChordEquality({ VirtualKeyModifiers::Control | VirtualKeyModifiers::Shift, static_cast('P'), 0 }, kbd); } } + + void KeyBindingsTests::LayerScancodeKeybindings() + { + Log::Comment(L"Layering a keybinding with a character literal on top of" + L" an equivalent sc() key should replace it."); + + // Wrap the first one in `R"!(...)!"` because it has `()` internally. + const std::string bindings0String{ R"!([ { "command": "quakeMode", "keys":"win+sc(41)" } ])!" }; + const std::string bindings1String{ R"([ { "keys": "win+`", "command": { "action": "globalSummon", "monitor": "any" } } ])" }; + const std::string bindings2String{ R"([ { "keys": "ctrl+shift+`", "command": { "action": "quakeMode" } } ])" }; + + const auto bindings0Json = VerifyParseSucceeded(bindings0String); + const auto bindings1Json = VerifyParseSucceeded(bindings1String); + const auto bindings2Json = VerifyParseSucceeded(bindings2String); + + auto actionMap = winrt::make_self(); + VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); + + actionMap->LayerJson(bindings0Json); + VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); + + actionMap->LayerJson(bindings1Json); + VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size(), L"Layering the second action should replace the first one."); + + actionMap->LayerJson(bindings2Json); + VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); + } } From 23e3306bfbc266b679289fdaf54339f676dc4962 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 4 Aug 2021 15:41:53 -0500 Subject: [PATCH 3/5] LOL clrealy these tests were never run --- src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index dc189449dca..5e6ed399fa6 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -97,13 +97,13 @@ namespace SettingsModelLocalTests VirtualKeyModifiers::Control | VirtualKeyModifiers::Menu | VirtualKeyModifiers::Shift | VirtualKeyModifiers::Windows, 255, 0, - L"ctrl+shift+alt+win+vk(255)", + L"win+ctrl+alt+shift+vk(255)", }, testCase{ - VirtualKeyModifiers::Windows, + VirtualKeyModifiers::Control | VirtualKeyModifiers::Menu | VirtualKeyModifiers::Shift | VirtualKeyModifiers::Windows, 0, 123, - L"ctrl+shift+alt+win+sc(123)", + L"win+ctrl+alt+shift+sc(123)", }, }; From 823d64390eb9b75068ebc99489853fc72b20322a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 11 Aug 2021 05:52:03 -0500 Subject: [PATCH 4/5] spel --- src/cascadia/Remoting/Monarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 7a08dfcca8b..4c9e70f7226 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -763,7 +763,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingUInt64(windowId, "peasantID", "The id of the window we tried to summon"), TraceLoggingBoolean(args.OnCurrentDesktop(), "OnCurrentDesktop", "true iff the window needs to be on the current virtual desktop"), TraceLoggingBoolean(args.SummonBehavior().MoveToCurrentDesktop(), "MoveToCurrentDesktop", "if true, move the window to the current virtual desktop"), - TraceLoggingBoolean(args.SummonBehavior().ToggleVisibility(), "ToggleVisibility", "true if we should toggle the visibity of the window"), + TraceLoggingBoolean(args.SummonBehavior().ToggleVisibility(), "ToggleVisibility", "true if we should toggle the visibility of the window"), TraceLoggingUInt32(args.SummonBehavior().DropdownDuration(), "DropdownDuration", "the duration to dropdown the window"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); From 5c17526cc59b42d9533649efbb2fe40a34bcb1b1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 11 Aug 2021 09:55:52 -0500 Subject: [PATCH 5/5] Update src/cascadia/Remoting/Monarch.cpp --- src/cascadia/Remoting/Monarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 4c9e70f7226..a5444e3d498 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -203,7 +203,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingWrite(g_hRemotingProvider, "Monarch_lookupPeasantIdForName", - TraceLoggingWideString(winrt::hstring{ name }.c_str(), "name", "the name we're looking for"), + TraceLoggingWideString(std::wstring{ name }.c_str(), "name", "the name we're looking for"), TraceLoggingUInt64(result, "peasantID", "the ID of the peasant with that name"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE));