From b4f65030e3a95f74bae6ed7385595f05f7ceeadf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 11 Apr 2023 17:10:11 -0500 Subject: [PATCH 01/13] Fix re-persisting the new legacy themes (#15160) Yep, I forgot to not write them back to the settings file here. Regressed in #15108 Closes #15152 --- .../CascadiaSettingsSerialization.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 9b73d35565..a251622dc8 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -56,6 +56,15 @@ constexpr std::wstring_view legacySystemThemeName{ L"legacySystem" }; constexpr std::wstring_view legacyDarkThemeName{ L"legacyDark" }; constexpr std::wstring_view legacyLightThemeName{ L"legacyLight" }; +static constexpr std::array builtinThemes{ + systemThemeName, + lightThemeName, + darkThemeName, + legacySystemThemeName, + legacyLightThemeName, + legacyDarkThemeName, +}; + static constexpr std::wstring_view jsonExtension{ L".json" }; static constexpr std::wstring_view FragmentsSubDirectory{ L"\\Fragments" }; static constexpr std::wstring_view FragmentsPath{ L"\\Microsoft\\Windows Terminal\\Fragments" }; @@ -566,8 +575,9 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source if (const auto theme = Theme::FromJson(themeJson)) { const auto& name{ theme->Name() }; + if (origin != OriginTag::InBox && - (name == systemThemeName || name == lightThemeName || name == darkThemeName || name == legacySystemThemeName || name == legacyDarkThemeName || name == legacyLightThemeName)) + (std::ranges::find(builtinThemes, name) != builtinThemes.end())) { // If the theme didn't come from the in-box themes, and its // name was one of the reserved names, then just ignore it. @@ -1274,7 +1284,8 @@ Json::Value CascadiaSettings::ToJson() const // Ignore the built in themes, when serializing the themes back out. We // don't want to re-include them in the user settings file. const auto theme{ winrt::get_self(entry.Value()) }; - if (theme->Name() == systemThemeName || theme->Name() == lightThemeName || theme->Name() == darkThemeName) + const auto& name{ theme->Name() }; + if (std::ranges::find(builtinThemes, name) != builtinThemes.end()) { continue; } From 508adbb1ec04e47d369e3213f178c33ed47bb53b Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 11 Apr 2023 23:59:25 +0100 Subject: [PATCH 02/13] Send a KeyUp sequence only once a key has been released (#15130) When win32-input-mode is enabled, we generate an input sequence for both key down and key up events. However, in the initial implementation the key up sequence for most keypresses would be faked - they were generated at the same time as the key down sequence, regardless of when the key was actually released. After this PR, we'll only generate the key up sequence once a key has actually been released. ## References and Relevant Issues The initial implementation of win32-input-mode was in PR #6309. The spec for win32-input-mode was in PR #5887. ## Validation Steps Performed I've manually tested this with an app that polls `ReadConsoleInput` in a loop and logs the results. With this PR applied, I can now see the key up events as a key is released, rather than when it was first pressed. When compared with conhost, though, there are some differences. When typing a shifted key, e.g. `Shift`+`A`, WT generates key down and key up events for both the `Shift` and the `A`, while conhost only generates both events for the `Shift` - the `A` won't generate a key up event unless you release the `Shift` before the `A`. That seems more like a conhost flaw though. Another case I tested was the Japanese Microsoft IME, which in conhost will generate a key down event for the Japanese character being inserted followed by a key up event for for `Return`. WT generates key up events for the ASCII characters being typed in the IME, then both a key down and key up event for the inserted Japanese character, and finally a key up event for `Return`. Both of those seem weird, but they still appear to work OK. The current version of WT actually produces the most sensible behavior for the IME - it just generates key up and key down events for the inserted character. But that's only because it's dropping most of the system generated key up events. Closes #8440 --- src/cascadia/TerminalCore/Terminal.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 40a506c3b3..99fa845347 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -666,7 +666,7 @@ std::optional Terminal::GetHyperlinkIntervalFromViewportPos // - Character events (e.g. WM_CHAR) are generally the best way to properly receive // keyboard input on Windows though, as the OS is suited best at handling the // translation of the current keyboard layout, dead keys, etc. -// As a result of this false is returned for all key events that contain characters. +// As a result of this false is returned for all key down events that contain characters. // SendCharEvent may then be called with the data obtained from a character event. // - As a special case we'll always handle VK_TAB key events. // This must be done due to TermControl::_KeyDownHandler (one of the callers) @@ -728,15 +728,15 @@ bool Terminal::SendKeyEvent(const WORD vkey, const auto isSuppressedAltGrAlias = !_altGrAliasing && states.IsAltPressed() && states.IsCtrlPressed() && !states.IsAltGrPressed(); const auto ch = isSuppressedAltGrAlias ? UNICODE_NULL : _CharacterFromKeyEvent(vkey, sc, states); - // Delegate it to the character event handler if this key event can be - // mapped to one (see method description above). For Alt+key combinations + // Delegate it to the character event handler if this is a key down event that + // can be mapped to one (see method description above). For Alt+key combinations // we'll not receive another character event for some reason though. // -> Don't delegate the event if this is a Alt+key combination. // // As a special case we'll furthermore always handle VK_TAB // key events here instead of in Terminal::SendCharEvent. // See the method description for more information. - if (!isAltOnlyPressed && vkey != VK_TAB && ch != UNICODE_NULL) + if (keyDown && !isAltOnlyPressed && vkey != VK_TAB && ch != UNICODE_NULL) { return false; } @@ -818,15 +818,8 @@ bool Terminal::SendCharEvent(const wchar_t ch, const WORD scanCode, const Contro MarkOutputStart(); } - // Unfortunately, the UI doesn't give us both a character down and a - // character up event, only a character received event. So fake sending both - // to the terminal input translator. Unless it's in win32-input-mode, it'll - // ignore the keyup. const KeyEvent keyDown{ true, 1, vkey, scanCode, ch, states.Value() }; - const KeyEvent keyUp{ false, 1, vkey, scanCode, ch, states.Value() }; - const auto handledDown = _terminalInput->HandleKey(&keyDown); - const auto handledUp = _terminalInput->HandleKey(&keyUp); - return handledDown || handledUp; + return _terminalInput->HandleKey(&keyDown); } // Method Description: From 56d451ded7cf9ca64abc511740221b963c98ded3 Mon Sep 17 00:00:00 2001 From: Ian O'Neill Date: Wed, 12 Apr 2023 00:01:11 +0100 Subject: [PATCH 03/13] Support environment variables in the settings (#15082) Existing environment variables can be referenced by enclosing the name in percent characters (e.g. `%PATH%`). Resurrects #9287 by @christapley. Tests added and manually tested. Closes #2785 Closes #9233 Co-authored-by: Chris Tapley --- doc/cascadia/profiles.schema.json | 7 + .../LocalTests_SettingsModel/ProfileTests.cpp | 47 +++++ .../SerializationTests.cpp | 8 +- .../Resources/en-US/Resources.resw | 3 + src/cascadia/TerminalApp/TerminalPage.cpp | 17 +- src/cascadia/TerminalApp/TerminalWindow.cpp | 1 + .../TerminalConnection/ConptyConnection.cpp | 71 ++++---- .../TerminalConnection/ConptyConnection.h | 4 +- .../TerminalConnection/ConptyConnection.idl | 3 +- .../TerminalControl/IControlSettings.idl | 1 - .../CascadiaSettings.cpp | 26 +++ .../TerminalSettingsModel/CascadiaSettings.h | 1 + .../TerminalSettingsModel/MTSMSettings.h | 1 + src/cascadia/TerminalSettingsModel/Profile.h | 2 + .../TerminalSettingsModel/Profile.idl | 5 + .../TerminalSettings.cpp | 14 ++ .../TerminalSettingsModel/TerminalSettings.h | 3 +- .../TerminalSettings.idl | 5 +- .../TerminalWarnings.idl | 1 + src/cascadia/inc/ControlProperties.h | 1 - src/inc/til/env.h | 167 +++++++++++++----- src/inc/til/string.h | 31 ++++ src/types/Environment.cpp | 132 -------------- src/types/inc/Environment.hpp | 31 ---- src/types/lib/types.vcxproj | 2 - 25 files changed, 322 insertions(+), 262 deletions(-) delete mode 100644 src/types/Environment.cpp delete mode 100644 src/types/inc/Environment.hpp diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 2c62802115..55a866a05b 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -2574,6 +2574,13 @@ "default": false, "description": "When true, this profile should always open in an elevated context. If the window isn't running as an Administrator, then a new elevated window will be created." }, + "environment": { + "description": "Key-value pairs representing environment variables to set. Environment variable names are not case sensitive. You can reference existing environment variable names by enclosing them in literal percent characters (e.g. %PATH%).", + "type": "object", + "additionalProperties": { + "type": "string" + } + }, "experimental.autoMarkPrompts": { "default": false, "description": "When set to true, prompts will automatically be marked.", diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp index fdb160fe27..a73b2bec3e 100644 --- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp @@ -38,6 +38,9 @@ namespace SettingsModelLocalTests TEST_METHOD(LayerProfileProperties); TEST_METHOD(LayerProfileIcon); TEST_METHOD(LayerProfilesOnArray); + TEST_METHOD(ProfileWithEnvVars); + TEST_METHOD(ProfileWithEnvVarsSameNameDifferentCases); + TEST_METHOD(DuplicateProfileTest); TEST_METHOD(TestGenGuidsForProfiles); TEST_METHOD(TestCorrectOldDefaultShellPaths); @@ -349,6 +352,50 @@ namespace SettingsModelLocalTests VERIFY_ARE_NOT_EQUAL(settings->AllProfiles().GetAt(0).Guid(), settings->AllProfiles().GetAt(1).Guid()); } + void ProfileTests::ProfileWithEnvVars() + { + const std::string profileString{ R"({ + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "environment": { + "VAR_1": "value1", + "VAR_2": "value2", + "VAR_3": "%VAR_3%;value3" + } + })" }; + const auto profile = implementation::Profile::FromJson(VerifyParseSucceeded(profileString)); + std::vector envVarMaps{}; + envVarMaps.emplace_back(profile->EnvironmentVariables()); + for (auto& envMap : envVarMaps) + { + VERIFY_ARE_EQUAL(static_cast(3), envMap.Size()); + VERIFY_ARE_EQUAL(L"value1", envMap.Lookup(L"VAR_1")); + VERIFY_ARE_EQUAL(L"value2", envMap.Lookup(L"VAR_2")); + VERIFY_ARE_EQUAL(L"%VAR_3%;value3", envMap.Lookup(L"VAR_3")); + } + } + + void ProfileTests::ProfileWithEnvVarsSameNameDifferentCases() + { + const std::string userSettings{ R"({ + "profiles": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "environment": { + "FOO": "VALUE", + "Foo": "Value" + } + } + ] + })" }; + const auto settings = winrt::make_self(userSettings); + const auto warnings = settings->Warnings(); + VERIFY_ARE_EQUAL(static_cast(2), warnings.Size()); + uint32_t index; + VERIFY_IS_TRUE(warnings.IndexOf(SettingsLoadWarnings::InvalidProfileEnvironmentVariables, index)); + } + void ProfileTests::TestCorrectOldDefaultShellPaths() { static constexpr std::string_view inboxProfiles{ R"({ diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp index d4cbd49aaf..ffc7dea07b 100644 --- a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp @@ -165,7 +165,13 @@ namespace SettingsModelLocalTests "historySize": 9001, "closeOnExit": "graceful", - "experimental.retroTerminalEffect": false + "experimental.retroTerminalEffect": false, + "environment": + { + "KEY_1": "VALUE_1", + "KEY_2": "%KEY_1%", + "KEY_3": "%PATH%" + } })" }; static constexpr std::string_view smallProfileString{ R"( diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index 02b8d8d76e..e3c992f1d9 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -282,6 +282,9 @@ Failed to parse "startupActions". {Locked="\"startupActions\""} + + Found multiple environment variables with the same name in different cases (lower/upper) - only one value will be used. + An optional command, with arguments, to be spawned in the new tab or pane diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 92fc375f8e..240ee8eca8 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1216,7 +1216,8 @@ namespace winrt::TerminalApp::implementation nullptr, settings.InitialRows(), settings.InitialCols(), - winrt::guid()); + winrt::guid(), + profile.Guid()); if constexpr (Feature_VtPassthroughMode::IsEnabled()) { @@ -1228,12 +1229,9 @@ namespace winrt::TerminalApp::implementation else { - // profile is guaranteed to exist here - auto guidWString = Utils::GuidToString(profile.Guid()); - - StringMap envMap{}; - envMap.Insert(L"WT_PROFILE_ID", guidWString); - envMap.Insert(L"WSLENV", L"WT_PROFILE_ID"); + const auto environment = settings.EnvironmentVariables() != nullptr ? + settings.EnvironmentVariables().GetView() : + nullptr; // Update the path to be relative to whatever our CWD is. // @@ -1264,10 +1262,11 @@ namespace winrt::TerminalApp::implementation auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(), newWorkingDirectory, settings.StartingTitle(), - envMap.GetView(), + environment, settings.InitialRows(), settings.InitialCols(), - winrt::guid()); + winrt::guid(), + profile.Guid()); valueSet.Insert(L"passthroughMode", Windows::Foundation::PropertyValue::CreateBoolean(settings.VtPassthrough())); valueSet.Insert(L"reloadEnvironmentVariables", diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 07cc41274e..66d8a53f59 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -50,6 +50,7 @@ static const std::array settingsLoadWarningsLabels{ USES_RESOURCE(L"InvalidColorSchemeInCmd"), USES_RESOURCE(L"InvalidSplitSize"), USES_RESOURCE(L"FailedToParseStartupActions"), + USES_RESOURCE(L"InvalidProfileEnvironmentVariables"), USES_RESOURCE(L"FailedToParseSubCommands"), USES_RESOURCE(L"UnknownTheme"), USES_RESOURCE(L"DuplicateRemainingProfilesEntry"), diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index fef88c3362..9e59098985 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -5,12 +5,12 @@ #include "ConptyConnection.h" #include +#include #include #include #include "CTerminalHandoff.h" #include "LibraryResources.h" -#include "../../types/inc/Environment.hpp" #include "../../types/inc/utils.hpp" #include "ConptyConnection.g.cpp" @@ -84,31 +84,19 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation auto cmdline{ wil::ExpandEnvironmentStringsW(_commandline.c_str()) }; // mutable copy -- required for CreateProcessW - Utils::EnvironmentVariableMapW environment; + til::env environment; auto zeroEnvMap = wil::scope_exit([&]() noexcept { - // Can't zero the keys, but at least we can zero the values. - for (auto& [name, value] : environment) - { - ::SecureZeroMemory(value.data(), value.size() * sizeof(decltype(value.begin())::value_type)); - } - environment.clear(); }); // Populate the environment map with the current environment. if (_reloadEnvironmentVariables) { - til::env refreshedEnvironment; - refreshedEnvironment.regenerate(); - - for (auto& [key, value] : refreshedEnvironment.as_map()) - { - environment.try_emplace(key, std::move(value)); - } + environment.regenerate(); } else { - RETURN_IF_FAILED(Utils::UpdateEnvironmentMapW(environment)); + environment = til::env::from_current_environment(); } { @@ -119,39 +107,45 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation const auto guidSubStr = std::wstring_view{ wsGuid }.substr(1); // Ensure every connection has the unique identifier in the environment. - environment.insert_or_assign(L"WT_SESSION", guidSubStr.data()); + environment.as_map().insert_or_assign(L"WT_SESSION", guidSubStr.data()); + // The profile Guid does include the enclosing '{}' + const auto profileGuid{ Utils::GuidToString(_profileGuid) }; + environment.as_map().insert_or_assign(L"WT_PROFILE_ID", profileGuid.data()); + + // WSLENV is a colon-delimited list of environment variables (+flags) that should appear inside WSL + // https://devblogs.microsoft.com/commandline/share-environment-vars-between-wsl-and-windows/ + std::wstring wslEnv{ L"WT_SESSION:WT_PROFILE_ID:" }; if (_environment) { - // add additional WT env vars like WT_SETTINGS, WT_DEFAULTS and WT_PROFILE_ID - for (auto item : _environment) + // Order the environment variable names so that resolution order is consistent + std::set keys{}; + for (const auto item : _environment) + { + keys.insert(item.Key().c_str()); + } + // add additional env vars + for (const auto& key : keys) { try { - auto key = item.Key(); // This will throw if the value isn't a string. If that // happens, then just skip this entry. - auto value = winrt::unbox_value(item.Value()); + const auto value = winrt::unbox_value(_environment.Lookup(key)); - // avoid clobbering WSLENV - if (std::wstring_view{ key } == L"WSLENV") - { - auto current = environment[L"WSLENV"]; - value = current + L":" + value; - } - - environment.insert_or_assign(key.c_str(), value.c_str()); + environment.set_user_environment_var(key.c_str(), value.c_str()); + // For each environment variable added to the environment, also add it to WSLENV + wslEnv += key + L":"; } CATCH_LOG(); } } - // WSLENV is a colon-delimited list of environment variables (+flags) that should appear inside WSL - // https://devblogs.microsoft.com/commandline/share-environment-vars-between-wsl-and-windows/ - - auto wslEnv = environment[L"WSLENV"]; - wslEnv = L"WT_SESSION:" + wslEnv; // prepend WT_SESSION to make sure it's visible inside WSL. - environment.insert_or_assign(L"WSLENV", wslEnv); + // We want to prepend new environment variables to WSLENV - that way if a variable already + // exists in WSLENV but with a flag, the flag will be respected. + // (This behaviour was empirically observed) + wslEnv += environment.as_map()[L"WSLENV"]; + environment.as_map().insert_or_assign(L"WSLENV", wslEnv); } std::vector newEnvVars; @@ -160,7 +154,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation newEnvVars.size() * sizeof(decltype(newEnvVars.begin())::value_type)); }); - RETURN_IF_FAILED(Utils::EnvironmentMapToEnvironmentStringsW(environment, newEnvVars)); + RETURN_IF_FAILED(environment.to_environment_strings_w(newEnvVars)); auto lpEnvironment = newEnvVars.empty() ? nullptr : newEnvVars.data(); @@ -244,7 +238,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation const Windows::Foundation::Collections::IMapView& environment, uint32_t rows, uint32_t columns, - const winrt::guid& guid) + const winrt::guid& guid, + const winrt::guid& profileGuid) { Windows::Foundation::Collections::ValueSet vs{}; @@ -254,6 +249,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation vs.Insert(L"initialRows", Windows::Foundation::PropertyValue::CreateUInt32(rows)); vs.Insert(L"initialCols", Windows::Foundation::PropertyValue::CreateUInt32(columns)); vs.Insert(L"guid", Windows::Foundation::PropertyValue::CreateGuid(guid)); + vs.Insert(L"profileGuid", Windows::Foundation::PropertyValue::CreateGuid(profileGuid)); if (environment) { @@ -288,6 +284,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation } _reloadEnvironmentVariables = winrt::unbox_value_or(settings.TryLookup(L"reloadEnvironmentVariables").try_as(), _reloadEnvironmentVariables); + _profileGuid = winrt::unbox_value_or(settings.TryLookup(L"profileGuid").try_as(), _profileGuid); } if (_guid == guid{}) diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h index 60528dc046..8ac589d646 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.h +++ b/src/cascadia/TerminalConnection/ConptyConnection.h @@ -52,7 +52,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation const Windows::Foundation::Collections::IMapView& environment, uint32_t rows, uint32_t columns, - const winrt::guid& guid); + const winrt::guid& guid, + const winrt::guid& profileGuid); WINRT_CALLBACK(TerminalOutput, TerminalOutputHandler); @@ -90,6 +91,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation std::array _buffer{}; bool _passthroughMode{}; bool _reloadEnvironmentVariables{}; + guid _profileGuid{}; struct StartupInfoFromDefTerm { diff --git a/src/cascadia/TerminalConnection/ConptyConnection.idl b/src/cascadia/TerminalConnection/ConptyConnection.idl index 135a2fd2fb..c9928d5c42 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.idl +++ b/src/cascadia/TerminalConnection/ConptyConnection.idl @@ -31,6 +31,7 @@ namespace Microsoft.Terminal.TerminalConnection IMapView environment, UInt32 rows, UInt32 columns, - Guid guid); + Guid guid, + Guid profileGuid); }; } diff --git a/src/cascadia/TerminalControl/IControlSettings.idl b/src/cascadia/TerminalControl/IControlSettings.idl index 1e2f7d71f2..131b9432bd 100644 --- a/src/cascadia/TerminalControl/IControlSettings.idl +++ b/src/cascadia/TerminalControl/IControlSettings.idl @@ -52,7 +52,6 @@ namespace Microsoft.Terminal.Control String Commandline { get; }; String StartingDirectory { get; }; - String EnvironmentVariables { get; }; TextAntialiasingMode AntialiasingMode { get; }; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 8a73dea58a..851e667ac9 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -15,6 +15,7 @@ #include #include #include +#include using namespace winrt::Microsoft::Terminal; using namespace winrt::Microsoft::Terminal::Settings; @@ -417,6 +418,7 @@ void CascadiaSettings::_validateSettings() _validateKeybindings(); _validateColorSchemesInCommands(); _validateThemeExists(); + _validateProfileEnvironmentVariables(); } // Method Description: @@ -541,6 +543,30 @@ void CascadiaSettings::_validateMediaResources() } } +// Method Description: +// - Checks if the profiles contain multiple environment variables with the same name, but different +// cases +void CascadiaSettings::_validateProfileEnvironmentVariables() +{ + for (const auto& profile : _allProfiles) + { + std::set envVarNames{}; + if (profile.EnvironmentVariables() == nullptr) + { + continue; + } + for (const auto [key, value] : profile.EnvironmentVariables()) + { + const auto iterator = envVarNames.insert(key.c_str()); + if (!iterator.second) + { + _warnings.Append(SettingsLoadWarnings::InvalidProfileEnvironmentVariables); + return; + } + } + } +} + // Method Description: // - Helper to get the GUID of a profile, given an optional index and a possible // "profile" value to override that. diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index b41a17c57e..d4ad396e26 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -162,6 +162,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _validateSettings(); void _validateAllSchemesExist(); void _validateMediaResources(); + void _validateProfileEnvironmentVariables(); void _validateKeybindings() const; void _validateColorSchemesInCommands() const; bool _hasInvalidColorScheme(const Model::Command& command) const; diff --git a/src/cascadia/TerminalSettingsModel/MTSMSettings.h b/src/cascadia/TerminalSettingsModel/MTSMSettings.h index 84a6002d0e..544366fcea 100644 --- a/src/cascadia/TerminalSettingsModel/MTSMSettings.h +++ b/src/cascadia/TerminalSettingsModel/MTSMSettings.h @@ -82,6 +82,7 @@ Author(s): X(CloseOnExitMode, CloseOnExit, "closeOnExit", CloseOnExitMode::Automatic) \ X(hstring, TabTitle, "tabTitle") \ X(Model::BellStyle, BellStyle, "bellStyle", BellStyle::Audible) \ + X(IEnvironmentVariableMap, EnvironmentVariables, "environment", nullptr) \ X(bool, UseAtlasEngine, "useAtlasEngine", Feature_AtlasEngine::IsEnabled()) \ X(bool, RightClickContextMenu, "experimental.rightClickContextMenu", false) \ X(Windows::Foundation::Collections::IVector, BellSound, "bellSound", nullptr) \ diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index dbc33fc9ef..05bd77cc72 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -67,6 +67,8 @@ namespace TerminalAppUnitTests class JsonTests; }; +using IEnvironmentVariableMap = winrt::Windows::Foundation::Collections::IMap; + // GUID used for generating GUIDs at runtime, for profiles that did not have a // GUID specified manually. constexpr GUID RUNTIME_GENERATED_PROFILE_NAMESPACE_GUID = { 0xf65ddb7e, 0x706b, 0x4499, { 0x8a, 0x50, 0x40, 0x31, 0x3c, 0xaf, 0x51, 0x0a } }; diff --git a/src/cascadia/TerminalSettingsModel/Profile.idl b/src/cascadia/TerminalSettingsModel/Profile.idl index 0f6bf14921..f6ab00a26c 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.idl +++ b/src/cascadia/TerminalSettingsModel/Profile.idl @@ -9,6 +9,8 @@ import "FontConfig.idl"; _BASE_INHERITABLE_SETTING(Type, Name); \ Microsoft.Terminal.Settings.Model.Profile Name##OverrideSource { get; } +#define COMMA , + namespace Microsoft.Terminal.Settings.Model { // This tag is used to identify the context in which the Profile was created @@ -81,6 +83,9 @@ namespace Microsoft.Terminal.Settings.Model INHERITABLE_PROFILE_SETTING(Boolean, SnapOnInput); INHERITABLE_PROFILE_SETTING(Boolean, AltGrAliasing); INHERITABLE_PROFILE_SETTING(BellStyle, BellStyle); + + INHERITABLE_PROFILE_SETTING(Windows.Foundation.Collections.IMap, EnvironmentVariables); + INHERITABLE_PROFILE_SETTING(Boolean, UseAtlasEngine); INHERITABLE_PROFILE_SETTING(Windows.Foundation.Collections.IVector, BellSound); diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp b/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp index 091b33e444..a69ba7086b 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp @@ -306,6 +306,20 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _TabColor = static_cast(colorRef); } + const auto profileEnvVars = profile.EnvironmentVariables(); + if (profileEnvVars == nullptr) + { + _EnvironmentVariables = std::nullopt; + } + else + { + _EnvironmentVariables = winrt::single_threaded_map(); + for (const auto& [key, value] : profileEnvVars) + { + _EnvironmentVariables.value().Insert(key, value); + } + } + _Elevate = profile.Elevate(); _AutoMarkPrompts = Feature_ScrollbarMarks::IsEnabled() && profile.AutoMarkPrompts(); _ShowMarks = Feature_ScrollbarMarks::IsEnabled() && profile.ShowMarks(); diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.h b/src/cascadia/TerminalSettingsModel/TerminalSettings.h index cb43e24eb0..7df75e1ab5 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalSettings.h +++ b/src/cascadia/TerminalSettingsModel/TerminalSettings.h @@ -22,6 +22,7 @@ Author(s): using IFontAxesMap = winrt::Windows::Foundation::Collections::IMap; using IFontFeatureMap = winrt::Windows::Foundation::Collections::IMap; +using IEnvironmentVariableMap = winrt::Windows::Foundation::Collections::IMap; // fwdecl unittest classes namespace SettingsModelLocalTests @@ -143,7 +144,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation INHERITABLE_SETTING(Model::TerminalSettings, hstring, StartingDirectory); INHERITABLE_SETTING(Model::TerminalSettings, hstring, StartingTitle); INHERITABLE_SETTING(Model::TerminalSettings, bool, SuppressApplicationTitle); - INHERITABLE_SETTING(Model::TerminalSettings, hstring, EnvironmentVariables); + INHERITABLE_SETTING(Model::TerminalSettings, IEnvironmentVariableMap, EnvironmentVariables); INHERITABLE_SETTING(Model::TerminalSettings, Microsoft::Terminal::Control::ScrollbarState, ScrollState, Microsoft::Terminal::Control::ScrollbarState::Visible); INHERITABLE_SETTING(Model::TerminalSettings, bool, UseAtlasEngine, false); diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.idl b/src/cascadia/TerminalSettingsModel/TerminalSettings.idl index 514fcf7e36..3bc2c72ed0 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalSettings.idl +++ b/src/cascadia/TerminalSettingsModel/TerminalSettings.idl @@ -3,6 +3,8 @@ import "CascadiaSettings.idl"; +#define COMMA , + namespace Microsoft.Terminal.Settings.Model { runtimeclass TerminalSettingsCreateResult @@ -26,6 +28,8 @@ namespace Microsoft.Terminal.Settings.Model { TerminalSettings(); + Windows.Foundation.Collections.IMap EnvironmentVariables; + static TerminalSettings CreateForPreview(CascadiaSettings appSettings, Profile profile); static TerminalSettingsCreateResult CreateWithProfile(CascadiaSettings appSettings, Profile profile, Microsoft.Terminal.Control.IKeyBindings keybindings); static TerminalSettingsCreateResult CreateWithNewTerminalArgs(CascadiaSettings appSettings, NewTerminalArgs newTerminalArgs, Microsoft.Terminal.Control.IKeyBindings keybindings); @@ -39,7 +43,6 @@ namespace Microsoft.Terminal.Settings.Model // able to change these at runtime (e.g. when duplicating a pane). String Commandline { set; }; String StartingDirectory { set; }; - String EnvironmentVariables { set; }; Boolean Elevate; }; diff --git a/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl b/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl index a82acf0d5d..aa02d18ff4 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl +++ b/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl @@ -20,6 +20,7 @@ namespace Microsoft.Terminal.Settings.Model InvalidColorSchemeInCmd, InvalidSplitSize, FailedToParseStartupActions, + InvalidProfileEnvironmentVariables, FailedToParseSubCommands, UnknownTheme, DuplicateRemainingProfilesEntry, diff --git a/src/cascadia/inc/ControlProperties.h b/src/cascadia/inc/ControlProperties.h index 4d2bec9e20..cccde919a9 100644 --- a/src/cascadia/inc/ControlProperties.h +++ b/src/cascadia/inc/ControlProperties.h @@ -66,7 +66,6 @@ X(winrt::Microsoft::Terminal::Control::IKeyBindings, KeyBindings, nullptr) \ X(winrt::hstring, Commandline) \ X(winrt::hstring, StartingDirectory) \ - X(winrt::hstring, EnvironmentVariables) \ X(winrt::Microsoft::Terminal::Control::ScrollbarState, ScrollState, winrt::Microsoft::Terminal::Control::ScrollbarState::Visible) \ X(winrt::Microsoft::Terminal::Control::TextAntialiasingMode, AntialiasingMode, winrt::Microsoft::Terminal::Control::TextAntialiasingMode::Grayscale) \ X(bool, ForceFullRepaintRendering, false) \ diff --git a/src/inc/til/env.h b/src/inc/til/env.h index 72e626a8da..1ed0ada927 100644 --- a/src/inc/til/env.h +++ b/src/inc/til/env.h @@ -5,6 +5,7 @@ #include #include +#include #pragma warning(push) #pragma warning(disable : 26429) // Symbol '...' is never tested for nullness, it can be marked as not_null (f.23). @@ -21,37 +22,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" namespace details { - // - // A case-insensitive wide-character map is used to store environment variables - // due to documented requirements: - // - // "All strings in the environment block must be sorted alphabetically by name. - // The sort is case-insensitive, Unicode order, without regard to locale. - // Because the equal sign is a separator, it must not be used in the name of - // an environment variable." - // https://docs.microsoft.com/en-us/windows/desktop/ProcThread/changing-environment-variables - // - // - Returns CSTR_LESS_THAN, CSTR_EQUAL or CSTR_GREATER_THAN - [[nodiscard]] inline int compare_string_ordinal(const std::wstring_view& lhs, const std::wstring_view& rhs) noexcept - { - const auto result = CompareStringOrdinal( - lhs.data(), - ::base::saturated_cast(lhs.size()), - rhs.data(), - ::base::saturated_cast(rhs.size()), - TRUE); - FAIL_FAST_LAST_ERROR_IF(!result); - return result; - } - - struct wstring_case_insensitive_compare - { - [[nodiscard]] bool operator()(const std::wstring& lhs, const std::wstring& rhs) const noexcept - { - return compare_string_ordinal(lhs, rhs) == CSTR_LESS_THAN; - } - }; - namespace vars { inline constexpr wil::zwstring_view system_root{ L"SystemRoot" }; @@ -250,7 +220,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" friend class ::EnvTests; #endif - std::map _envMap{}; + std::map _envMap{}; // We make copies of the environment variable names to ensure they are null terminated. void get(wil::zwstring_view variable) @@ -438,13 +408,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return expanded; } - void set_user_environment_var(std::wstring_view var, std::wstring_view value) - { - auto valueString = expand_environment_strings(value); - valueString = check_for_temp(var, valueString); - save_to_map(std::wstring{ var }, std::move(valueString)); - } - void concat_var(std::wstring var, std::wstring value) { if (const auto existing = _envMap.find(var); existing != _envMap.end()) @@ -475,8 +438,8 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" { static constexpr std::wstring_view temp{ L"temp" }; static constexpr std::wstring_view tmp{ L"tmp" }; - if (til::details::compare_string_ordinal(var, temp) == CSTR_EQUAL || - til::details::compare_string_ordinal(var, tmp) == CSTR_EQUAL) + if (til::compare_string_ordinal(var, temp) == CSTR_EQUAL || + til::compare_string_ordinal(var, tmp) == CSTR_EQUAL) { return til::details::wil_env::GetShortPathNameW(value.data()); } @@ -491,9 +454,9 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" static constexpr std::wstring_view path{ L"Path" }; static constexpr std::wstring_view libPath{ L"LibPath" }; static constexpr std::wstring_view os2LibPath{ L"Os2LibPath" }; - return til::details::compare_string_ordinal(input, path) == CSTR_EQUAL || - til::details::compare_string_ordinal(input, libPath) == CSTR_EQUAL || - til::details::compare_string_ordinal(input, os2LibPath) == CSTR_EQUAL; + return til::compare_string_ordinal(input, path) == CSTR_EQUAL || + til::compare_string_ordinal(input, libPath) == CSTR_EQUAL || + til::compare_string_ordinal(input, os2LibPath) == CSTR_EQUAL; } static void strip_trailing_null(std::wstring& str) noexcept @@ -533,6 +496,35 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" parse(block); } + // Function Description: + // - Creates a new environment with the current process's unicode environment + // variables. + // Return Value: + // - A new environment + static til::env from_current_environment() + { + LPWCH currentEnvVars{}; + auto freeCurrentEnv = wil::scope_exit([&] { + if (currentEnvVars) + { + FreeEnvironmentStringsW(currentEnvVars); + currentEnvVars = nullptr; + } + }); + + currentEnvVars = ::GetEnvironmentStringsW(); + THROW_HR_IF_NULL(E_OUTOFMEMORY, currentEnvVars); + + return til::env{ currentEnvVars }; + } + + void set_user_environment_var(std::wstring_view var, std::wstring_view value) + { + auto valueString = expand_environment_strings(value); + valueString = check_for_temp(var, valueString); + save_to_map(std::wstring{ var }, std::move(valueString)); + } + void regenerate() { // Generally replicates the behavior of shell32!RegenerateUserEnvironment @@ -572,6 +564,93 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" return result; } + void clear() noexcept + { + // Can't zero the keys, but at least we can zero the values. + for (auto& [name, value] : _envMap) + { + ::SecureZeroMemory(value.data(), value.size() * sizeof(decltype(value.begin())::value_type)); + } + + _envMap.clear(); + } + + // Function Description: + // - Creates a new environment block using the provided vector as appropriate + // (resizing if needed) based on the current environment variable map + // matching the format of GetEnvironmentStringsW. + // Arguments: + // - newEnvVars: The vector that will be used to create the new environment block. + // Return Value: + // - S_OK if we succeeded, or an appropriate HRESULT for failing + HRESULT to_environment_strings_w(std::vector& newEnvVars) + try + { + // Clear environment block before use. + constexpr auto cbChar{ sizeof(decltype(newEnvVars.begin())::value_type) }; + + if (!newEnvVars.empty()) + { + ::SecureZeroMemory(newEnvVars.data(), newEnvVars.size() * cbChar); + } + + // Resize environment block to fit map. + size_t cchEnv{ 2 }; // For the block's double NULL-terminator. + for (const auto& [name, value] : _envMap) + { + // Final form of "name=value\0". + cchEnv += name.size() + 1 + value.size() + 1; + } + newEnvVars.resize(cchEnv); + + // Ensure new block is wiped if we exit due to failure. + auto zeroNewEnv = wil::scope_exit([&]() noexcept { + ::SecureZeroMemory(newEnvVars.data(), newEnvVars.size() * cbChar); + }); + + // Transform each map entry and copy it into the new environment block. + auto pEnvVars{ newEnvVars.data() }; + auto cbRemaining{ cchEnv * cbChar }; + for (const auto& [name, value] : _envMap) + { + // Final form of "name=value\0" for every entry. + { + const auto cchSrc{ name.size() }; + const auto cbSrc{ cchSrc * cbChar }; + RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, name.c_str(), cbSrc) != 0); + pEnvVars += cchSrc; + cbRemaining -= cbSrc; + } + + RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"=", cbChar) != 0); + ++pEnvVars; + cbRemaining -= cbChar; + + { + const auto cchSrc{ value.size() }; + const auto cbSrc{ cchSrc * cbChar }; + RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, value.c_str(), cbSrc) != 0); + pEnvVars += cchSrc; + cbRemaining -= cbSrc; + } + + RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"\0", cbChar) != 0); + ++pEnvVars; + cbRemaining -= cbChar; + } + + // Environment block only has to be NULL-terminated, but double NULL-terminate anyway. + RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"\0\0", cbChar * 2) != 0); + cbRemaining -= cbChar * 2; + + RETURN_HR_IF(E_UNEXPECTED, cbRemaining != 0); + + zeroNewEnv.release(); // success; don't wipe new environment block on exit + + return S_OK; + } + CATCH_RETURN(); + auto& as_map() noexcept { return _envMap; diff --git a/src/inc/til/string.h b/src/inc/til/string.h index 00b8c1e4eb..fb9bd3e566 100644 --- a/src/inc/til/string.h +++ b/src/inc/til/string.h @@ -342,4 +342,35 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" { return prefix_split<>(str, needle); } + + // + // A case-insensitive wide-character map is used to store environment variables + // due to documented requirements: + // + // "All strings in the environment block must be sorted alphabetically by name. + // The sort is case-insensitive, Unicode order, without regard to locale. + // Because the equal sign is a separator, it must not be used in the name of + // an environment variable." + // https://docs.microsoft.com/en-us/windows/desktop/ProcThread/changing-environment-variables + // + // - Returns CSTR_LESS_THAN, CSTR_EQUAL or CSTR_GREATER_THAN + [[nodiscard]] inline int compare_string_ordinal(const std::wstring_view& lhs, const std::wstring_view& rhs) noexcept + { + const auto result = CompareStringOrdinal( + lhs.data(), + ::base::saturated_cast(lhs.size()), + rhs.data(), + ::base::saturated_cast(rhs.size()), + TRUE); + FAIL_FAST_LAST_ERROR_IF(!result); + return result; + } + + struct wstring_case_insensitive_compare + { + [[nodiscard]] bool operator()(const std::wstring& lhs, const std::wstring& rhs) const noexcept + { + return compare_string_ordinal(lhs, rhs) == CSTR_LESS_THAN; + } + }; } diff --git a/src/types/Environment.cpp b/src/types/Environment.cpp deleted file mode 100644 index 9e6a111858..0000000000 --- a/src/types/Environment.cpp +++ /dev/null @@ -1,132 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -#include "precomp.h" -#include "inc/Environment.hpp" - -using namespace ::Microsoft::Console::Utils; - -// We cannot use spand or not_null because we're dealing with \0\0-terminated buffers of unknown length -#pragma warning(disable : 26481 26429) - -// Function Description: -// - Updates an EnvironmentVariableMapW with the current process's unicode -// environment variables ignoring ones already set in the provided map. -// Arguments: -// - map: The map to populate with the current processes's environment variables. -// Return Value: -// - S_OK if we succeeded, or an appropriate HRESULT for failing -HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map) noexcept -try -{ - LPWCH currentEnvVars{}; - auto freeCurrentEnv = wil::scope_exit([&] { - if (currentEnvVars) - { - FreeEnvironmentStringsW(currentEnvVars); - currentEnvVars = nullptr; - } - }); - - currentEnvVars = ::GetEnvironmentStringsW(); - RETURN_HR_IF_NULL(E_OUTOFMEMORY, currentEnvVars); - - // Each entry is NULL-terminated; block is guaranteed to be double-NULL terminated at a minimum. - for (const wchar_t* lastCh{ currentEnvVars }; *lastCh != '\0'; ++lastCh) - { - // Copy current entry into temporary map. - const auto cchEntry{ ::wcslen(lastCh) }; - const std::wstring_view entry{ lastCh, cchEntry }; - - // Every entry is of the form "name=value\0". - const auto pos = entry.find_first_of(L"=", 0, 1); - RETURN_HR_IF(E_UNEXPECTED, pos == std::wstring::npos); - - std::wstring name{ entry.substr(0, pos) }; // portion before '=' - std::wstring value{ entry.substr(pos + 1) }; // portion after '=' - - // Don't replace entries that already exist. - map.try_emplace(std::move(name), std::move(value)); - lastCh += cchEntry; - } - - return S_OK; -} -CATCH_RETURN(); - -// Function Description: -// - Creates a new environment block using the provided vector as appropriate -// (resizing if needed) based on the provided environment variable map -// matching the format of GetEnvironmentStringsW. -// Arguments: -// - map: The map to populate the new environment block vector with. -// - newEnvVars: The vector that will be used to create the new environment block. -// Return Value: -// - S_OK if we succeeded, or an appropriate HRESULT for failing -HRESULT Microsoft::Console::Utils::EnvironmentMapToEnvironmentStringsW(EnvironmentVariableMapW& map, std::vector& newEnvVars) noexcept -try -{ - // Clear environment block before use. - constexpr auto cbChar{ sizeof(decltype(newEnvVars.begin())::value_type) }; - - if (!newEnvVars.empty()) - { - ::SecureZeroMemory(newEnvVars.data(), newEnvVars.size() * cbChar); - } - - // Resize environment block to fit map. - size_t cchEnv{ 2 }; // For the block's double NULL-terminator. - for (const auto& [name, value] : map) - { - // Final form of "name=value\0". - cchEnv += name.size() + 1 + value.size() + 1; - } - newEnvVars.resize(cchEnv); - - // Ensure new block is wiped if we exit due to failure. - auto zeroNewEnv = wil::scope_exit([&]() noexcept { - ::SecureZeroMemory(newEnvVars.data(), newEnvVars.size() * cbChar); - }); - - // Transform each map entry and copy it into the new environment block. - auto pEnvVars{ newEnvVars.data() }; - auto cbRemaining{ cchEnv * cbChar }; - for (const auto& [name, value] : map) - { - // Final form of "name=value\0" for every entry. - { - const auto cchSrc{ name.size() }; - const auto cbSrc{ cchSrc * cbChar }; - RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, name.c_str(), cbSrc) != 0); - pEnvVars += cchSrc; - cbRemaining -= cbSrc; - } - - RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"=", cbChar) != 0); - ++pEnvVars; - cbRemaining -= cbChar; - - { - const auto cchSrc{ value.size() }; - const auto cbSrc{ cchSrc * cbChar }; - RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, value.c_str(), cbSrc) != 0); - pEnvVars += cchSrc; - cbRemaining -= cbSrc; - } - - RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"\0", cbChar) != 0); - ++pEnvVars; - cbRemaining -= cbChar; - } - - // Environment block only has to be NULL-terminated, but double NULL-terminate anyway. - RETURN_HR_IF(E_OUTOFMEMORY, memcpy_s(pEnvVars, cbRemaining, L"\0\0", cbChar * 2) != 0); - cbRemaining -= cbChar * 2; - - RETURN_HR_IF(E_UNEXPECTED, cbRemaining != 0); - - zeroNewEnv.release(); // success; don't wipe new environment block on exit - - return S_OK; -} -CATCH_RETURN(); diff --git a/src/types/inc/Environment.hpp b/src/types/inc/Environment.hpp deleted file mode 100644 index 0994a17776..0000000000 --- a/src/types/inc/Environment.hpp +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -namespace Microsoft::Console::Utils -{ - // - // A case-insensitive wide-character map is used to store environment variables - // due to documented requirements: - // - // "All strings in the environment block must be sorted alphabetically by name. - // The sort is case-insensitive, Unicode order, without regard to locale. - // Because the equal sign is a separator, it must not be used in the name of - // an environment variable." - // https://docs.microsoft.com/en-us/windows/desktop/ProcThread/changing-environment-variables - // - struct WStringCaseInsensitiveCompare - { - [[nodiscard]] bool operator()(const std::wstring& lhs, const std::wstring& rhs) const noexcept - { - return (::_wcsicmp(lhs.c_str(), rhs.c_str()) < 0); - } - }; - - using EnvironmentVariableMapW = std::map; - - [[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map) noexcept; - - [[nodiscard]] HRESULT EnvironmentMapToEnvironmentStringsW(EnvironmentVariableMapW& map, - std::vector& newEnvVars) noexcept; - -}; diff --git a/src/types/lib/types.vcxproj b/src/types/lib/types.vcxproj index 9d015dff9b..bceb8348e8 100644 --- a/src/types/lib/types.vcxproj +++ b/src/types/lib/types.vcxproj @@ -15,7 +15,6 @@ - @@ -43,7 +42,6 @@ - From 10bdadffbdd150b66afe5a258b40efe39b396b79 Mon Sep 17 00:00:00 2001 From: James Pack Date: Wed, 12 Apr 2023 12:56:55 -0400 Subject: [PATCH 04/13] Skip generating a profile for rancher-desktop (#15166) Don't generate a profile for rancher-desktop utility WSL distro. Adds a check for rancher-desktop as well as docker. As mentioned in the discussion of this issue. This becomes much more difficult to maintain once other folks inevitably start to follow this pattern. But the easy win was up for grabs so I took it :) Closes #12757 --- src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index 373282866a..2b656154ad 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -11,6 +11,7 @@ static constexpr std::wstring_view WslHomeDirectory{ L"~" }; static constexpr std::wstring_view DockerDistributionPrefix{ L"docker-desktop" }; +static constexpr std::wstring_view RancherDistributionPrefix{ L"rancher-desktop" }; // The WSL entries are structured as such: // HKCU\Software\Microsoft\Windows\CurrentVersion\Lxss @@ -78,9 +79,9 @@ static void namesToProfiles(const std::vector& names, std::vector< { for (const auto& distName : names) { - if (til::starts_with(distName, DockerDistributionPrefix)) + if (til::starts_with(distName, DockerDistributionPrefix) || til::starts_with(distName, RancherDistributionPrefix)) { - // Docker for Windows creates some utility distributions to handle Docker commands. + // Docker for Windows and Rancher for Windows creates some utility distributions to handle Docker commands. // Pursuant to GH#3556, because they are _not_ user-facing we want to hide them. continue; } From f671f065bf313f1450c7e27e271856e4cf76989a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 12 Apr 2023 11:57:25 -0500 Subject: [PATCH 05/13] Register the GetWindowLayoutRequested handler only when ready (#15161) Moves our `GetWindowLayoutRequested` handler AFTER the xaml island is started. The `AppHost::_GetWindowLayoutAsync` handler requires us to be able to work on our UI thread, which requires that we have a `Dispatcher` ready for us to move to. If we set up this callback in the ctor, then it is possible for there to be a time slice where * the monarch creates the peasant for us, * we get ctor'ed (registering the callback) * then the monarch attempts to query all _peasants_ for their layout, coming back to ask us even before XAML has been created. I believe this was the source of the crash that was reported in a mail thread. It actually happened to me once while debugging another branch. Alas, this was realy hard to hit in the first place, so I'm not _totally_ certain this fixes it. Related to #14957 --- src/cascadia/WindowsTerminal/AppHost.cpp | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 633b3519e5..9866cb74e2 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -87,13 +87,6 @@ AppHost::AppHost(const winrt::TerminalApp::AppLogic& logic, _window->SetAutoHideWindow(_windowLogic.AutoHideWindow()); _window->MakeWindow(); - - _GetWindowLayoutRequestedToken = _peasant.GetWindowLayoutRequested([this](auto&&, - const Remoting::GetWindowLayoutArgs& args) { - // The peasants are running on separate threads, so they'll need to - // swap what context they are in to the ui thread to get the actual layout. - args.WindowLayoutJsonAsync(_GetWindowLayoutAsync()); - }); } AppHost::~AppHost() @@ -388,6 +381,23 @@ void AppHost::Initialize() _revokers.RequestMoveContent = _windowLogic.RequestMoveContent(winrt::auto_revoke, { this, &AppHost::_handleMoveContent }); _revokers.RequestReceiveContent = _windowLogic.RequestReceiveContent(winrt::auto_revoke, { this, &AppHost::_handleReceiveContent }); _revokers.SendContentRequested = _peasant.SendContentRequested(winrt::auto_revoke, { this, &AppHost::_handleSendContent }); + + // Add our GetWindowLayoutRequested handler AFTER the xaml island is + // started. Our _GetWindowLayoutAsync handler requires us to be able to work + // on our UI thread, which requires that we have a Dispatcher ready for us + // to move to. If we set up this callback in the ctor, then it is possible + // for there to be a time slice where + // * the monarch creates the peasant for us, + // * we get constructed (registering the callback) + // * then the monarch attempts to query all _peasants_ for their layout, + // coming back to ask us even before XAML has been created. + _GetWindowLayoutRequestedToken = _peasant.GetWindowLayoutRequested([this](auto&&, + const Remoting::GetWindowLayoutArgs& args) { + // The peasants are running on separate threads, so they'll need to + // swap what context they are in to the ui thread to get the actual layout. + args.WindowLayoutJsonAsync(_GetWindowLayoutAsync()); + }); + // BODGY // On certain builds of Windows, when Terminal is set as the default // it will accumulate an unbounded amount of queued animations while From 72d0566fa6d681291799e59a69168fc0483d8277 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 13 Apr 2023 13:38:38 -0500 Subject: [PATCH 06/13] Back off between attempts to start the tests (#15106) Looking through this test, I seriously don't understand how this doesn't work. I mean, I don't really get how it _does_ work, but at this point in the tests, we've actually established that both `Nihilist.exe` _and_ openconsole are running. From my read, there's no reason these should be failing at this point. We previously added a "retry 5 times" bit to this test, in #8534. That did work back then. So uh, just do that... again? --- src/host/ft_host/InitTests.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/host/ft_host/InitTests.cpp b/src/host/ft_host/InitTests.cpp index 7b7b334849..50e5c0cf47 100644 --- a/src/host/ft_host/InitTests.cpp +++ b/src/host/ft_host/InitTests.cpp @@ -237,11 +237,12 @@ MODULE_SETUP(ModuleSetup) // to the one that belongs to the CMD.exe in the new OpenConsole.exe window. VERIFY_WIN32_BOOL_SUCCEEDED_RETURN(FreeConsole()); - // Wait a moment for the driver to be ready after freeing to attach. VERIFY_WIN32_BOOL_SUCCEEDED_RETURN(AttachConsole(dwFindPid)); - auto tries = 0; - while (tries < 5) + int tries = 0; + DWORD delay; + // This will wait for up to 32s in total (from 10ms to 163840ms) + for (delay = 10; delay < 30000u; delay *= 2) { tries++; Log::Comment(NoThrowString().Format(L"Attempt #%d to confirm we've attached", tries)); @@ -267,17 +268,20 @@ MODULE_SETUP(ModuleSetup) auto succeeded = GetConsoleScreenBufferInfoEx(hOut, &csbiexBefore); if (!succeeded) { - auto gle = GetLastError(); + const auto gle = GetLastError(); VERIFY_ARE_EQUAL(6u, gle, L"If we fail to set up the console, GetLastError should return 6 here."); - Sleep(1000); + + // Sleep with a backoff, to give us longer to try next time. + WaitForSingleObject(GetCurrentThread(), delay); } else { + Log::Comment(NoThrowString().Format(L"Succeeded on try #%d", tries)); break; } }; - VERIFY_IS_LESS_THAN(tries, 5, L"Make sure we set up the new console in time"); + VERIFY_IS_LESS_THAN(delay, 30000u, L"Make sure we set up the new console in time"); return true; } From 789b0b065f9c2f0694a59adbea64525c6d8a33af Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 14 Apr 2023 12:13:07 -0500 Subject: [PATCH 07/13] Actually use the persisted position with `centerOnLaunch:true` (#15179) Fixes an issue when using both: ```json "centerOnLaunch": true, "firstWindowPreference": "persistedWindowLayout", ``` In this case, the Terminal would ignore the persisted location and still just center on launch. This has been really annoying while testing tear-out, as we keep re-opening all my debug windows as a stack on top of each other. --- src/cascadia/TerminalApp/TerminalWindow.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 66d8a53f59..f8a906b55a 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -740,9 +740,19 @@ namespace winrt::TerminalApp::implementation { // If // * the position has been specified on the commandline, + // * we're re-opening from a persisted layout, // * We're opening the window as a part of tear out (and _contentBounds were set) // then don't center on launch - return !_contentBounds && _settings.GlobalSettings().CenterOnLaunch() && !_appArgs.GetPosition().has_value(); + bool hadPersistedPosition = false; + if (const auto layout = LoadPersistedLayout()) + { + hadPersistedPosition = (bool)layout.InitialPosition(); + } + + return !_contentBounds && + !hadPersistedPosition && + _settings.GlobalSettings().CenterOnLaunch() && + !_appArgs.GetPosition().has_value(); } // Method Description: From 21464fe41c9c09eac4b9e2d85225f18f1f3c2c7b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 14 Apr 2023 13:07:05 -0500 Subject: [PATCH 08/13] Manually hide our DesktopWindowXamlSource (#15165) As discussed in #6507 Newer builds of Windows do this automatically. However, this was spotted in the wild on 1.18. It's possible the threading changes created a situation where the OS-side fix no longer applied to us. So let's just do it manually. It doesn't have any side effects. I saw this once on Win11, but couldn't repro it this morning when I tried to add this fix. I'm just gonna assume this worked, despite the fact that I can't repro it on win11 anymore. closes #6507 See also #14957 ## detailed description > `WindowsXamlManager::XamlCore::Initialize` calls `ConfigureCoreWindow`, which creates a `CoreWindow` on the thread > Problem is, we're calling that on the main thread (which doesn't have _any_ windows), and then eventually creating a `DesktopWindowXamlSource` on a second thread for the actual window > It's not that it "manages a window", it's that it "manages xaml on Windows OS". just use ICoreWindowInterop -- QI for ICoreWindowInterop and call get_WindowHandle. Also see: * [ICoreWindowInterop](https://learn.microsoft.com/en-us/windows/win32/api/corewindow/nn-corewindow-icorewindowinterop) * [WindowsXamlManager.InitializeForCurrentThread](https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.hosting.windowsxamlmanager.initializeforcurrentthread?view=winrt-22621#windows-ui-xaml-hosting-windowsxamlmanager-initializeforcurrentthread) * The source code in `onecoreuap\windows\dxaml\xcp\dxaml\lib\WindowsXamlManager_Partial.*` * os.2020!6102020 which fixed MSFT:33498969, MSFT:27807465, MSFT:21854264 --- src/cascadia/TerminalApp/App.cpp | 22 +++++++++++++++++++ src/cascadia/WindowsTerminal/IslandWindow.cpp | 5 +++++ src/cascadia/WindowsTerminal/IslandWindow.h | 4 ++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalApp/App.cpp b/src/cascadia/TerminalApp/App.cpp index b4019d9522..0a73c8bb41 100644 --- a/src/cascadia/TerminalApp/App.cpp +++ b/src/cascadia/TerminalApp/App.cpp @@ -4,6 +4,7 @@ #include "pch.h" #include "App.h" #include "App.g.cpp" +#include using namespace winrt; using namespace winrt::Windows::ApplicationModel::Activation; @@ -32,6 +33,27 @@ namespace winrt::TerminalApp::implementation if (!dispatcherQueue) { _windowsXamlManager = xaml::Hosting::WindowsXamlManager::InitializeForCurrentThread(); + + // As of Process Model v3, terminal windows are all created on their + // own threads, but we still initiate XAML for the App on the main + // thread. Thing is, just initializing XAML creates a CoreWindow for + // us. On Windows 10, that CoreWindow will show up as a visible + // window on the taskbar, unless we hide it manually. So, go get it + // and do the SW_HIDE thing on it. + if (const auto& coreWindow{ winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread() }) + { + if (const auto& interop{ coreWindow.try_as() }) + { + HWND coreHandle{ 0 }; + interop->get_WindowHandle(&coreHandle); + if (coreHandle) + { + // This prevents an empty "DesktopWindowXamlSource" from + // appearing on the taskbar + ShowWindow(coreHandle, SW_HIDE); + } + } + } } else { diff --git a/src/cascadia/WindowsTerminal/IslandWindow.cpp b/src/cascadia/WindowsTerminal/IslandWindow.cpp index 7e4c5ff8ae..17a240f8fa 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp @@ -307,6 +307,11 @@ void IslandWindow::Initialize() // stash the child interop handle so we can resize it when the main hwnd is resized interop->get_WindowHandle(&_interopWindowHandle); + // Immediately hide our XAML island hwnd. On earlier versions of Windows, + // this HWND could sometimes appear as an actual window in the taskbar + // without this! + ShowWindow(_interopWindowHandle, SW_HIDE); + _rootGrid = winrt::Windows::UI::Xaml::Controls::Grid(); _source.Content(_rootGrid); diff --git a/src/cascadia/WindowsTerminal/IslandWindow.h b/src/cascadia/WindowsTerminal/IslandWindow.h index 3673ce8f5b..7a00528ae5 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.h +++ b/src/cascadia/WindowsTerminal/IslandWindow.h @@ -94,8 +94,8 @@ protected: HWND _interopWindowHandle; - winrt::Windows::UI::Xaml::Hosting::DesktopWindowXamlSource _source; - winrt::Windows::UI::Xaml::Controls::Grid _rootGrid; + winrt::Windows::UI::Xaml::Hosting::DesktopWindowXamlSource _source; // nulled in ctor + winrt::Windows::UI::Xaml::Controls::Grid _rootGrid; // nulled in ctor wil::com_ptr _taskbar; std::function _pfnCreateCallback; From 19069e03be029d0a97c504aa235aa59355f3e6a1 Mon Sep 17 00:00:00 2001 From: James Pack Date: Fri, 14 Apr 2023 14:52:39 -0400 Subject: [PATCH 09/13] A more efficient copy assignment operator for Pane.LayoutSizeNode (#15169) ## Summary of the Pull Request This pull request updates the implementation of the copy assignment operator for Pane::LayoutSizeNode to a more efficient version and eliminates the need for the _AssignChildNode code block. ## References and Relevant Issues #11965 #11963 ## Detailed Description of the Pull Request / Additional comments My understanding of the discussion and intent of the two linked issues is that this is a more efficient way to implement the copy assignment operator for Pane.LayoutSizeNode and eliminates the need for the code block _AssignChildNode. Since both were relatively small changes, I combined the two in one PR. If that is not desirable, I can separate them. All existing tests continue to pass. image ## Validation Steps Performed All existing tests pass. No visible changes in behavior of the terminal. ## PR Checklist - [x] Closes #11963 - [x] Closes #11965 - [x] Tests added/passed - [ ] Documentation updated - If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx - [ ] Schema updated (if necessary) --- .../TerminalApp/Pane.LayoutSizeNode.cpp | 35 +++---------------- src/cascadia/TerminalApp/Pane.h | 3 -- 2 files changed, 4 insertions(+), 34 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.LayoutSizeNode.cpp b/src/cascadia/TerminalApp/Pane.LayoutSizeNode.cpp index f3cc8647ff..75215b1fe6 100644 --- a/src/cascadia/TerminalApp/Pane.LayoutSizeNode.cpp +++ b/src/cascadia/TerminalApp/Pane.LayoutSizeNode.cpp @@ -37,37 +37,10 @@ Pane::LayoutSizeNode& Pane::LayoutSizeNode::operator=(const LayoutSizeNode& othe size = other.size; isMinimumSize = other.isMinimumSize; - _AssignChildNode(firstChild, other.firstChild.get()); - _AssignChildNode(secondChild, other.secondChild.get()); - _AssignChildNode(nextFirstChild, other.nextFirstChild.get()); - _AssignChildNode(nextSecondChild, other.nextSecondChild.get()); + firstChild = other.firstChild ? std::make_unique(*other.firstChild) : nullptr; + secondChild = other.secondChild ? std::make_unique(*other.secondChild) : nullptr; + nextFirstChild = other.nextFirstChild ? std::make_unique(*other.nextFirstChild) : nullptr; + nextSecondChild = other.nextSecondChild ? std::make_unique(*other.nextSecondChild) : nullptr; return *this; } - -// Method Description: -// - Performs assignment operation on a single child node reusing -// - current one if present. -// Arguments: -// - nodeField: Reference to our field holding concerned node. -// - other: Node to take the values from. -// Return Value: -// - -void Pane::LayoutSizeNode::_AssignChildNode(std::unique_ptr& nodeField, const LayoutSizeNode* const newNode) -{ - if (newNode) - { - if (nodeField) - { - *nodeField = *newNode; - } - else - { - nodeField = std::make_unique(*newNode); - } - } - else - { - nodeField.release(); - } -} diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index a163ba2b4b..cc84474386 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -394,9 +394,6 @@ private: LayoutSizeNode(const LayoutSizeNode& other); LayoutSizeNode& operator=(const LayoutSizeNode& other); - - private: - void _AssignChildNode(std::unique_ptr& nodeField, const LayoutSizeNode* const newNode); }; friend struct winrt::TerminalApp::implementation::TerminalTab; From eb725e99936dcdf7b6eabd48fc0efd606c6a4925 Mon Sep 17 00:00:00 2001 From: "Mitch Capper (they, them)" Date: Fri, 14 Apr 2023 13:25:07 -0700 Subject: [PATCH 10/13] fix: WpfTerminalControl allow Connection set to null (#15062) Hides the cursor when null, shows it when not. Clear the screen any time the connection is changed. This prevents the WPF Control from crashing when set back to null, clears the console and hides the mouse as well. It sends 3 VT sequences as well now: 1) When the Connection is set to null the cursor is hidden (reflects what the default state is) 2) When the Connection is set to a value and it was null before we show the cursor (not a breaking change as requires it to have been null which previously would cause a crash, except for for set) 3) When the Connection is changed the terminal is reset. A breaking change officially although not sure if there are use cases where this behavior is not desired. For added safety we could make sure we are not being set to the same value we currently are. None of the ansi commands are needed, users could do it all themselves as well, the behavior largely seemed natural though. I didn't see any ansi constants anywhere so they are just hard coded with comments, but not sure if there is an established better practice. Closes #15061 --- .../WpfTerminalControl/TerminalContainer.cs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/cascadia/WpfTerminalControl/TerminalContainer.cs b/src/cascadia/WpfTerminalControl/TerminalContainer.cs index 5480f53fce..c9bed99c5c 100644 --- a/src/cascadia/WpfTerminalControl/TerminalContainer.cs +++ b/src/cascadia/WpfTerminalControl/TerminalContainer.cs @@ -113,10 +113,23 @@ namespace Microsoft.Terminal.Wpf { this.connection.TerminalOutput -= this.Connection_TerminalOutput; } - + this.Connection_TerminalOutput(this, new TerminalOutputEventArgs("\x001bc\x1b]104\x1b\\")); //reset console/clear screen - https://github.com/microsoft/terminal/pull/15062#issuecomment-1505654110 + var wasNull = this.connection == null; this.connection = value; - this.connection.TerminalOutput += this.Connection_TerminalOutput; - this.connection.Start(); + if (this.connection != null) + { + if (wasNull) + { + this.Connection_TerminalOutput(this, new TerminalOutputEventArgs("\x1b[?25h")); //show cursor + } + this.connection.TerminalOutput += this.Connection_TerminalOutput; + this.connection.Start(); + } + else + { + this.Connection_TerminalOutput(this, new TerminalOutputEventArgs("\x1b[?25l")); //hide cursor + } + } } From 1d354d0f5cfdee7588431db66ea0b1e5521d4e9c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 14 Apr 2023 23:15:08 +0200 Subject: [PATCH 11/13] Fix a hang when writing wide glyphs into a 1 column terminal (#15171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code changes are mostly self-explanatory: Just skip glyphs that can never be inserted. I implemented it slightly incorrectly (a newline will be inserted every time you write such a wide glyph), but it's a niche issue and I think the simplicity of the fix is more important than its exact correctness. It also contains a fix for some severe log spam due to `_PrepareForDoubleByteSequence` complaining in this situation. The spam is so bad that it freezes the app for a few seconds during text buffer reflow. Closes #7416 ## Validation Steps Performed * Open an extra pane and run `TerminalStress.exe` in there * Resize to 1 column * Doesn't hang ✅ --- src/buffer/out/textBuffer.cpp | 4 ---- src/terminal/adapter/adaptDispatch.cpp | 24 ++++++++++++++++-------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 9e969c6ed2..b3c2e590e3 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -272,10 +272,6 @@ bool TextBuffer::_AssertValidDoubleByteSequence(const DbcsAttribute dbcsAttribut // - false otherwise (out of memory) bool TextBuffer::_PrepareForDoubleByteSequence(const DbcsAttribute dbcsAttribute) { - // This function corrects most errors. If this is false, we had an uncorrectable one which - // older versions of conhost simply let pass by unflinching. - LOG_HR_IF(E_NOT_VALID_STATE, !(_AssertValidDoubleByteSequence(dbcsAttribute))); // Shouldn't be uncorrectable sequences unless something is very wrong. - auto fSuccess = true; // Now compensate if we don't have enough space for the upcoming double byte sequence // We only need to compensate for leading bytes diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 32c06d67bf..37e7125627 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -139,18 +139,26 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string) if (isWrapping) { + // We want to wrap, but we failed to write even a single character into the row. + // ROW::Write() returns the lineWidth and leaves stringIterator untouched. To prevent a + // deadlock, because stringIterator never advances, we need to throw that glyph away. + // + // This can happen under two circumstances: + // * The glyph is wider than the buffer and can never be inserted in + // the first place. There's no good way to detect this, so we check + // whether the begin column is the left margin, which is the column + // at which any legit insertion should work at a minimum. + // * The DECAWM Autowrap mode is disabled ("\x1b[?7l", !wrapAtEOL) and + // we tried writing a wide glyph into the last column which can't work. + if (textPositionBefore == textPositionAfter && (state.columnBegin == 0 || !wrapAtEOL)) + { + textBuffer.ConsumeGrapheme(state.text); + } + if (wrapAtEOL) { cursor.DelayEOLWrap(); } - else if (textPositionBefore == textPositionAfter) - { - // We want to wrap, but we're not allowed to and we failed to write even a single character into the row. - // This can only mean one thing! The DECAWM Autowrap mode is disabled ("\x1b[?7l") and we tried writing a - // wide glyph into the last column. ROW::Write() returns the lineWidth and leaves stringIterator untouched. - // To prevent a deadlock, because stringIterator never advances, we need to throw that glyph away. - textBuffer.ConsumeGrapheme(state.text); - } } } From 9b960bc88c75b3bd910c4929382713802e7e83c3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 17 Apr 2023 08:27:52 -0500 Subject: [PATCH 12/13] Fix reordering tabs mysteriously shuffling the actual backing tabs (#15178) TL;DR: we stopped getting `TabView.TabItemsChanged`. This meant that the tab view would change its apparent order, but we wouldn't change the backing tab order. I'm fixing this by grabbing the index of the tab that starts the drag, and the index of the tab view item at the end of the drag, and using that to reorder our backing list. Closes #15121 Upstream https://github.com/microsoft/microsoft-ui-xaml/issues/8388 Regressed in #15078 - I'm pretty confident about this, since I've got a 1.18.931 build of the Terminal with tear-out, but not MUX 2.8. --- src/cascadia/TerminalApp/TabManagement.cpp | 53 ++++++++++------------ src/cascadia/TerminalApp/TerminalPage.cpp | 7 +-- src/cascadia/TerminalApp/TerminalPage.h | 5 +- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index 457cf34c1c..5191808242 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -890,34 +890,6 @@ namespace winrt::TerminalApp::implementation co_await _HandleCloseTabRequested(tab); } } - // Method Description: - // - Responds to changes in the TabView's item list by changing the - // tabview's visibility. - // - This method is also invoked when tabs are dragged / dropped as part of - // tab reordering and this method hands that case as well in concert with - // TabDragStarting and TabDragCompleted handlers that are set up in - // TerminalPage::Create() - // Arguments: - // - sender: the control that originated this event - // - eventArgs: the event's constituent arguments - void TerminalPage::_OnTabItemsChanged(const IInspectable& /*sender*/, const Windows::Foundation::Collections::IVectorChangedEventArgs& eventArgs) - { - if (_rearranging) - { - if (eventArgs.CollectionChange() == Windows::Foundation::Collections::CollectionChange::ItemRemoved) - { - _rearrangeFrom = eventArgs.Index(); - } - - if (eventArgs.CollectionChange() == Windows::Foundation::Collections::CollectionChange::ItemInserted) - { - _rearrangeTo = eventArgs.Index(); - } - } - - CommandPalette().Visibility(Visibility::Collapsed); - _UpdateTabView(); - } // Method Description: // - Additional responses to clicking on a TabView's item. Currently, just remove tab with middle click @@ -1079,16 +1051,37 @@ namespace winrt::TerminalApp::implementation } void TerminalPage::_TabDragStarted(const IInspectable& /*sender*/, - const IInspectable& /*eventArgs*/) + const winrt::MUX::Controls::TabViewTabDragStartingEventArgs& eventArgs) { _rearranging = true; _rearrangeFrom = std::nullopt; _rearrangeTo = std::nullopt; + + // Start tracking the index of the tab that is being dragged. In + // `_TabDragCompleted`, we'll use this to determine how to reorder our + // internal tabs list. + const auto& draggedTabViewItem{ eventArgs.Tab() }; + uint32_t tabIndexFromControl{}; + const auto tabItems{ _tabView.TabItems() }; + if (tabItems.IndexOf(draggedTabViewItem, tabIndexFromControl)) + { + // If IndexOf returns true, we've actually got an index + _rearrangeFrom = tabIndexFromControl; + } } void TerminalPage::_TabDragCompleted(const IInspectable& /*sender*/, - const IInspectable& /*eventArgs*/) + const winrt::MUX::Controls::TabViewTabDragCompletedEventArgs& eventArgs) { + const auto& draggedTabViewItem{ eventArgs.Tab() }; + + uint32_t tabIndexFromControl{}; + const auto tabItems{ _tabView.TabItems() }; + if (tabItems.IndexOf(draggedTabViewItem, tabIndexFromControl)) + { + _rearrangeTo = tabIndexFromControl; + } + auto& from{ _rearrangeFrom }; auto& to{ _rearrangeTo }; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 240ee8eca8..fa2612c6c2 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -245,7 +245,6 @@ namespace winrt::TerminalApp::implementation }); _tabView.SelectionChanged({ this, &TerminalPage::_OnTabSelectionChanged }); _tabView.TabCloseRequested({ this, &TerminalPage::_OnTabCloseRequested }); - _tabView.TabItemsChanged({ this, &TerminalPage::_OnTabItemsChanged }); _tabView.TabDragStarting({ this, &TerminalPage::_onTabDragStarting }); _tabView.TabStripDragOver({ this, &TerminalPage::_onTabStripDragOver }); @@ -4720,6 +4719,8 @@ namespace winrt::TerminalApp::implementation co_await wil::resume_foreground(Dispatcher()); if (const auto& page{ weakThis.get() }) { + // `this` is safe to use + // // First we need to get the position in the List to drop to auto index = -1; @@ -4739,8 +4740,8 @@ namespace winrt::TerminalApp::implementation } } - // `this` is safe to use - const auto request = winrt::make_self(src, _WindowProperties.WindowId(), index); + const auto myId{ _WindowProperties.WindowId() }; + const auto request = winrt::make_self(src, myId, index); // This will go up to the monarch, who will then dispatch the request // back down to the source TerminalPage, who will then perform a diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 83b80bb633..a775902396 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -412,12 +412,11 @@ namespace winrt::TerminalApp::implementation fire_and_forget _LaunchSettings(const Microsoft::Terminal::Settings::Model::SettingsTarget target); - void _TabDragStarted(const IInspectable& sender, const IInspectable& eventArgs); - void _TabDragCompleted(const IInspectable& sender, const IInspectable& eventArgs); + void _TabDragStarted(const IInspectable& sender, const winrt::Microsoft::UI::Xaml::Controls::TabViewTabDragStartingEventArgs& eventArgs); + void _TabDragCompleted(const IInspectable& sender, const winrt::Microsoft::UI::Xaml::Controls::TabViewTabDragCompletedEventArgs& eventArgs); void _OnTabClick(const IInspectable& sender, const Windows::UI::Xaml::Input::PointerRoutedEventArgs& eventArgs); void _OnTabSelectionChanged(const IInspectable& sender, const Windows::UI::Xaml::Controls::SelectionChangedEventArgs& eventArgs); - void _OnTabItemsChanged(const IInspectable& sender, const Windows::Foundation::Collections::IVectorChangedEventArgs& eventArgs); void _OnTabCloseRequested(const IInspectable& sender, const Microsoft::UI::Xaml::Controls::TabViewTabCloseRequestedEventArgs& eventArgs); void _OnFirstLayout(const IInspectable& sender, const IInspectable& eventArgs); void _UpdatedSelectedTab(const winrt::TerminalApp::TabBase& tab); From 52171d2daba85a618440353aee09f938ac079c86 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 17 Apr 2023 08:28:29 -0500 Subject: [PATCH 13/13] Update to MUX 2.8.3 (#15183) This fixes the BreadcrumbBar issue that would crash into the debugger anytime you open the SUI on a second thread. See #14957. Maybe also tracked in #15144 - let's have @j4james test when this merges. --- dep/nuget/packages.config | 2 +- src/common.nugetversions.props | 2 +- src/common.nugetversions.targets | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dep/nuget/packages.config b/dep/nuget/packages.config index c39e95a3c8..2e4323f691 100644 --- a/dep/nuget/packages.config +++ b/dep/nuget/packages.config @@ -7,7 +7,7 @@ - + diff --git a/src/common.nugetversions.props b/src/common.nugetversions.props index a3ac0f8ec9..0955a1bdc4 100644 --- a/src/common.nugetversions.props +++ b/src/common.nugetversions.props @@ -37,6 +37,6 @@ - + diff --git a/src/common.nugetversions.targets b/src/common.nugetversions.targets index 7cffff5ec8..6c501971ab 100644 --- a/src/common.nugetversions.targets +++ b/src/common.nugetversions.targets @@ -52,7 +52,7 @@ - + - +