From ee17d6c55e621c1d26f8e3ed79466974121fff6d Mon Sep 17 00:00:00 2001 From: Muhammad Danish Date: Fri, 13 Oct 2023 01:14:56 +0500 Subject: [PATCH 01/15] Update note regarding WinGet installation (#16159) ## Summary of the Pull Request Dependency support is now GA in WinGet. Updating the instructions in README ## References and Relevant Issues ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed ## PR Checklist - [ ] Closes #xxx - [ ] 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) --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2ace5f109b5..88968fa7d22 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ winget install --id Microsoft.WindowsTerminal -e ``` > **Note**\ -> Due to [a dependency issue](https://github.com/microsoft/terminal/issues/15663), Terminal's current versions cannot be installed via the Windows Package Manager CLI. To install the stable release 1.17 or later, or the Preview release 1.18 or later, please use an alternative installation method. +> Dependency support is available in WinGet version [1.6.2631 or later](https://github.com/microsoft/winget-cli/releases). To install the Terminal stable release 1.18 or later, please make sure you have the updated version of the WinGet client. #### Via Chocolatey (unofficial) @@ -123,11 +123,11 @@ Windows Terminal Canary is a nightly build of Windows Terminal. This build has t Windows Terminal Canary is our least stable offering, so you may discover bugs before we have had a chance to find them. -Windows Terminal Canary is available as an App Installer distribution and a Portable ZIP distribution. +Windows Terminal Canary is available as an App Installer distribution and a Portable ZIP distribution. -The App Installer distribution supports automatic updates. Due to platform limitations, this installer only works on Windows 11. +The App Installer distribution supports automatic updates. Due to platform limitations, this installer only works on Windows 11. -The Portable ZIP distribution is a portable application. It will not automatically update and will not automatically check for updates. This portable ZIP distribution works on Windows 10 (19041+) and Windows 11. +The Portable ZIP distribution is a portable application. It will not automatically update and will not automatically check for updates. This portable ZIP distribution works on Windows 10 (19041+) and Windows 11. | Distribution | Architecture | Link | |---------------|:---------------:|------------------------------------------------------| From 81889a685ca115d2c2763a02edec1ab131c230c6 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 13 Oct 2023 14:08:49 -0500 Subject: [PATCH 02/15] derp --- src/cascadia/TerminalApp/Pane.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index d55611b33fd..237c71dafb2 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -2927,7 +2927,6 @@ void Pane::FinalizeConfigurationGivenDefault() { terminalPane.MarkAsDefterm(); } - // _isDefTermSession = true; } // Method Description: From fd0640997d5d746942563eb2f74a8259969f9c98 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 13 Oct 2023 15:17:38 -0500 Subject: [PATCH 03/15] annoying build break --- .../LocalTests_TerminalApp/TabTests.cpp | 20 +++++++++---------- src/cascadia/LocalTests_TerminalApp/pch.h | 2 ++ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 4a09828c680..d21497a51d9 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -1326,7 +1326,7 @@ namespace TerminalAppLocalTests const auto& controlSettings = activeControl.Settings(); VERIFY_IS_NOT_NULL(controlSettings); - VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1344,7 +1344,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed to the preview"); - VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, til::color{ controlSettings.DefaultBackground() }); // And we should have stored a function to revert the change. VERIFY_ARE_EQUAL(1u, page->_restorePreviewFuncs.size()); @@ -1366,7 +1366,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed"); - VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, til::color{ controlSettings.DefaultBackground() }); // After preview there should be no more restore functions to execute. VERIFY_ARE_EQUAL(0u, page->_restorePreviewFuncs.size()); @@ -1394,7 +1394,7 @@ namespace TerminalAppLocalTests const auto& controlSettings = activeControl.Settings(); VERIFY_IS_NOT_NULL(controlSettings); - VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1412,7 +1412,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed to the preview"); - VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1428,7 +1428,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be the same as it originally was"); - VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, til::color{ controlSettings.DefaultBackground() }); }); Log::Comment(L"Sleep to let events propagate"); Sleep(250); @@ -1450,7 +1450,7 @@ namespace TerminalAppLocalTests const auto& controlSettings = activeControl.Settings(); VERIFY_IS_NOT_NULL(controlSettings); - VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1467,7 +1467,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed to the preview"); - VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1484,7 +1484,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed to the preview"); - VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, til::color{ controlSettings.DefaultBackground() }); }); TestOnUIThread([&page]() { @@ -1503,7 +1503,7 @@ namespace TerminalAppLocalTests VERIFY_IS_NOT_NULL(controlSettings); Log::Comment(L"Color should be changed"); - VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, controlSettings.DefaultBackground()); + VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, til::color{ controlSettings.DefaultBackground() }); }); Log::Comment(L"Sleep to let events propagate"); Sleep(250); diff --git a/src/cascadia/LocalTests_TerminalApp/pch.h b/src/cascadia/LocalTests_TerminalApp/pch.h index f82561888b1..154fe5d0177 100644 --- a/src/cascadia/LocalTests_TerminalApp/pch.h +++ b/src/cascadia/LocalTests_TerminalApp/pch.h @@ -56,6 +56,8 @@ Author(s): #include #include +#include +#include #include #include From 27e1081c8cc417fade9e053b7b6738a6c382f7b9 Mon Sep 17 00:00:00 2001 From: Jaswir Date: Fri, 13 Oct 2023 22:43:38 +0200 Subject: [PATCH 04/15] Allow Opacity to be set differently in both focused and unfocused terminals (#15974) ## Summary of the Pull Request Closes #11092 Allowing `opacity `to be set differently in both focused and unfocused terminals ## References and Relevant Issues #11092 , references: #7158 ## Detailed Description of the Pull Request / Additional comments ### Allowing Opacity to be set differently in both focused and unfocused terminals: ![unfocused_opacity](https://github.com/microsoft/terminal/assets/15957528/1c38e40b-4678-43ec-b328-ad79d222579f) ![image](https://github.com/microsoft/terminal/assets/15957528/3e3342a8-7908-41db-9c37-26c89f7f2456) ![jolsen](https://github.com/microsoft/terminal/assets/15957528/68553507-d29e-4513-89ce-b1cd305d28b7) ![image](https://github.com/microsoft/terminal/assets/15957528/18864f60-91d0-4159-87da-2b2ee1637a4c) ## `_runtimeFocusedOpacity` Mike also had to say something about this: https://github.com/microsoft/terminal/issues/2531#issuecomment-1668442774 Initially I had something like ` _setOpacity(newAppearance->Opacity());` But with the introduction of unfocused opacity we encounter new challenges: When Adjusting the Opacity with **CTRL+SHIFT+Mouse Scroll Wheel** or **Set background opacity** in command pallette, the Runtime opacity changes, but when we go to unfocused and back to focused the opacity changes back to focused opacity in Settings. Also when adjusting opacity through the command palette the window becomes unfocused and then focused again after setting background opacity hence the ` _setOpacity(newAppearance->Opacity());` would override the changes made through command palette ![runtimeFocusedOpacity](https://github.com/microsoft/terminal/assets/15957528/4de63057-d658-4b5e-99ad-7db050834ade) ![command_pallette_focusswitches](https://github.com/microsoft/terminal/assets/15957528/372526eb-cf0c-40f8-a4e5-a0739f1f0e05) With the introduction of unfocused opacity we encounter new challenges. The runtime opacity stores both the unfocused opacity and focused opacity from settings at different moments. This all works well until we combine this with Adjusting the Opacity with **CTRL+SHIFT+Mouse Scroll Wheel** or **Set background opacity** in command pallette. This brings the need for a separate Focused Opacity. When we change the runtime opacity with scroll wheel or through command pallette this value needs to be stored separately from the one in settings. So we can change back to it when going to unfocused mode and back to focused instead of the focused opacity defined in settings. ## `skipUnfocusedOpacity` solves Opacity going from solid to unfocused to focused bug: ![skipUnfocusedOpacity_bug](https://github.com/microsoft/terminal/assets/15957528/ecc06dcf-fbef-4fef-a40f-68278fdbfb12) ## Validation Steps Performed - Checked if unfocused Opacity works well when adjusting opacity through Mouse Scroll Wheel or Command Palette and in combination with Acrylic as mentioned in "Detailed Description of the Pull Request / Additional comments" ## PR Checklist - [x] Closes #11092 - [ ] Tests added/passed - [x] Documentation updated - If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here:(https://github.com/MicrosoftDocs/terminal/pull/714) - [ ] Schema updated (if necessary) --- doc/cascadia/profiles.schema.json | 7 ++++ src/cascadia/TerminalControl/ControlCore.cpp | 38 +++++++++++++++----- src/cascadia/TerminalControl/ControlCore.h | 3 +- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 9dc3a8afd58..f4b0d5e37c8 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -273,6 +273,13 @@ "useAcrylic":{ "description": "When set to true, the window will have an acrylic material background when unfocused. When set to false, the window will have a plain, untextured background when unfocused.", "type": "boolean" + }, + "opacity": { + "default": 100, + "description": "Sets the opacity of the window for the profile when unfocused. Accepts values from 0-100.", + "maximum": 100, + "minimum": 0, + "type": "number" } }, "type": "object" diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index a7609d29747..7925fc16eb8 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -680,7 +680,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation _setOpacity(Opacity() + adjustment); } - void ControlCore::_setOpacity(const double opacity) + // Method Description: + // - Updates the opacity of the terminal + // Arguments: + // - opacity: The new opacity to set. + // - focused (default == true): Whether the window is focused or unfocused. + // Return Value: + // - + void ControlCore::_setOpacity(const double opacity, bool focused) { const auto newOpacity = std::clamp(opacity, 0.0, @@ -694,6 +701,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Update our runtime opacity value _runtimeOpacity = newOpacity; + //Stores the focused runtime opacity separately from unfocused opacity + //to transition smoothly between the two. + _runtimeFocusedOpacity = focused ? newOpacity : _runtimeFocusedOpacity; + // Manually turn off acrylic if they turn off transparency. _runtimeUseAcrylic = newOpacity < 1.0 && _settings->UseAcrylic(); @@ -824,6 +835,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _cellWidth = CSSLengthPercentage::FromString(_settings->CellWidth().c_str()); _cellHeight = CSSLengthPercentage::FromString(_settings->CellHeight().c_str()); _runtimeOpacity = std::nullopt; + _runtimeFocusedOpacity = std::nullopt; // Manually turn off acrylic if they turn off transparency. _runtimeUseAcrylic = _settings->Opacity() < 1.0 && _settings->UseAcrylic(); @@ -874,21 +886,29 @@ namespace winrt::Microsoft::Terminal::Control::implementation _renderEngine->SetRetroTerminalEffect(newAppearance->RetroTerminalEffect()); _renderEngine->SetPixelShaderPath(newAppearance->PixelShaderPath()); + // Incase EnableUnfocusedAcrylic is disabled and Focused Acrylic is set to true, + // the terminal should ignore the unfocused opacity from settings. + // The Focused Opacity from settings should be ignored if overridden at runtime. + bool useFocusedRuntimeOpacity = focused || (!_settings->EnableUnfocusedAcrylic() && UseAcrylic()); + double newOpacity = useFocusedRuntimeOpacity ? + FocusedOpacity() : + newAppearance->Opacity(); + _setOpacity(newOpacity, focused); + // No need to update Acrylic if UnfocusedAcrylic is disabled if (_settings->EnableUnfocusedAcrylic()) { // Manually turn off acrylic if they turn off transparency. _runtimeUseAcrylic = Opacity() < 1.0 && newAppearance->UseAcrylic(); + } - // Update the renderer as well. It might need to fall back from - // cleartype -> grayscale if the BG is transparent / acrylic. - _renderEngine->EnableTransparentBackground(_isBackgroundTransparent()); - _renderer->NotifyPaintFrame(); - - auto eventArgs = winrt::make_self(Opacity()); + // Update the renderer as well. It might need to fall back from + // cleartype -> grayscale if the BG is transparent / acrylic. + _renderEngine->EnableTransparentBackground(_isBackgroundTransparent()); + _renderer->NotifyPaintFrame(); - _TransparencyChangedHandlers(*this, *eventArgs); - } + auto eventArgs = winrt::make_self(Opacity()); + _TransparencyChangedHandlers(*this, *eventArgs); _renderer->TriggerRedrawAll(true, true); } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index c21730f4789..90cc5fca4d4 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -246,6 +246,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool ShouldShowSelectOutput(); RUNTIME_SETTING(double, Opacity, _settings->Opacity()); + RUNTIME_SETTING(double, FocusedOpacity, FocusedAppearance().Opacity()); RUNTIME_SETTING(bool, UseAcrylic, _settings->UseAcrylic()); // -------------------------------- WinRT Events --------------------------------- @@ -386,7 +387,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _updateAntiAliasingMode(); void _connectionOutputHandler(const hstring& hstr); void _updateHoveredCell(const std::optional terminalPosition); - void _setOpacity(const double opacity); + void _setOpacity(const double opacity, const bool focused = true); bool _isBackgroundTransparent(); void _focusChanged(bool focused); From 0b9f0417060567b4b2ae9613b92dc18b0545fe7c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 13 Oct 2023 13:44:14 -0700 Subject: [PATCH 05/15] Fix issues and warnings caused by profiles.schema.json (#16103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This addresses the following issues: * The JSON Schema spec doesn't actually define whether objects with a "properties" key still require `"type": "object"` or not. VS Code for instance largely pretends as if it's implied, but when it encounters them inside a `oneOf` tree, then it behaves as if it isn't. In other words, we need to always set `"type": "object"`. * Declaring an `oneOf` containing a `"type": "string"` and an `enum` doesn't work, because if one of the `enum` cases is given, it results in both variants to match, since any `enum` is also a `string`. We have to use `anyOf` instead. * `SuggestionSource` used `"BuiltinSuggestionSource"` inside a `type` key which doesn't work. We have to use `$ref` for that. Closes #13387 ## Validation Steps Performed * VS Code stops complaining ✅ * https://www.jsonschemavalidator.net/ ✅ --- doc/cascadia/profiles.schema.json | 145 +++++++++++++++++++++--------- 1 file changed, 105 insertions(+), 40 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index f4b0d5e37c8..0bc403498cf 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -86,12 +86,19 @@ ] }, "BuiltinSuggestionSource": { - "enum": [ - "commandHistory", - "tasks", - "all" - ], - "type": "string" + "type": "string", + "anyOf": [ + { + "type": "string" + }, + { + "enum": [ + "commandHistory", + "tasks", + "all" + ] + } + ] }, "SuggestionSource": { "default": "all", @@ -99,15 +106,17 @@ "$comment": "`tasks` and `local` are sources that would be added by the Tasks feature, as a follow-up", "oneOf": [ { - "type": [ "string", "null", "BuiltinSuggestionSource" ] + "type": "null" }, { - "type": "array", - "items": { "type": "BuiltinSuggestionSource" } + "$ref": "#/$defs/BuiltinSuggestionSource" }, { "type": "array", - "items": { "type": "string" } + "items": { + "$ref": "#/$defs/BuiltinSuggestionSource" + }, + "uniqueItems": true } ] }, @@ -201,10 +210,6 @@ "desktopWallpaper" ] } - ], - "type": [ - "string", - "null" ] }, "backgroundImageOpacity": { @@ -270,7 +275,7 @@ "description": "Use to set a path to a pixel shader to use with the Terminal when unfocused. Overrides `experimental.retroTerminalEffect`. This is an experimental feature, and its continued existence is not guaranteed.", "type": "string" }, - "useAcrylic":{ + "useAcrylic": { "description": "When set to true, the window will have an acrylic material background when unfocused. When set to false, the window will have a plain, untextured background when unfocused.", "type": "boolean" }, @@ -652,6 +657,7 @@ "$ref": "#/$defs/NewTabMenuEntry" }, { + "type": "object", "properties": { "type": { "type": "string", @@ -694,6 +700,7 @@ "$ref": "#/$defs/NewTabMenuEntry" }, { + "type": "object", "properties": { "type": { "type": "string", @@ -710,6 +717,7 @@ "$ref": "#/$defs/NewTabMenuEntry" }, { + "type": "object", "properties": { "type": { "type": "string", @@ -731,6 +739,7 @@ "$ref": "#/$defs/NewTabMenuEntry" }, { + "type": "object", "properties": { "type": { "type": "string", @@ -747,6 +756,7 @@ "$ref": "#/$defs/NewTabMenuEntry" }, { + "type": "object", "properties": { "type": { "type": "string", @@ -787,6 +797,7 @@ ] }, "ShortcutAction": { + "type": "object", "properties": { "action": { "description": "The action to execute", @@ -795,8 +806,7 @@ }, "required": [ "action" - ], - "type": "object" + ] }, "AdjustFontSizeAction": { "description": "Arguments corresponding to an Adjust Font Size Action", @@ -805,6 +815,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -829,6 +840,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -870,6 +882,7 @@ "$ref": "#/$defs/NewTerminalArgs" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -886,6 +899,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -910,6 +924,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -934,6 +949,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -958,6 +974,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -982,6 +999,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1006,6 +1024,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1030,6 +1049,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1057,6 +1077,7 @@ "$ref": "#/$defs/NewTerminalArgs" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1089,6 +1110,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1126,6 +1148,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1153,6 +1176,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1174,6 +1198,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1195,6 +1220,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1219,6 +1245,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1239,6 +1266,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1259,6 +1287,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1279,6 +1308,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1303,6 +1333,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1331,6 +1362,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1359,6 +1391,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1387,6 +1420,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1411,6 +1445,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1435,6 +1470,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1458,6 +1494,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1483,6 +1520,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1504,6 +1542,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1531,6 +1570,7 @@ "$ref": "#/$defs/NewTerminalArgs" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1547,6 +1587,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1568,6 +1609,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1589,6 +1631,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1610,6 +1653,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1631,6 +1675,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1653,6 +1698,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1674,6 +1720,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1695,6 +1742,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1746,6 +1794,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1762,6 +1811,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1785,6 +1835,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1813,6 +1864,7 @@ "$ref": "#/$defs/ShortcutAction" }, { + "type": "object", "properties": { "action": { "type": "string", @@ -1899,7 +1951,11 @@ "properties": { "applicationTheme": { "description": "Which UI theme the Terminal should use for controls", - "enum": [ "light", "dark", "system" ], + "enum": [ + "light", + "dark", + "system" + ], "type": "string" }, "useMica": { @@ -1930,7 +1986,11 @@ "type": "string", "description": "The name of the theme. This will be displayed in the settings UI.", "not": { - "enum": [ "light", "dark", "system" ] + "enum": [ + "light", + "dark", + "system" + ] } }, "tab": { @@ -1947,6 +2007,7 @@ "ThemePair": { "additionalProperties": false, "description": "A pair of Theme names, to allow the Terminal to switch theme based on the OS theme", + "type": "object", "properties": { "light": { "type": "string", @@ -2128,16 +2189,16 @@ }, "name": { "description": "The name that will appear in the command palette. If one isn't provided, the terminal will attempt to automatically generate a name.\nIf name is a string, it will be the name of the command.\nIf name is a object, the key property of the object will be used to lookup a localized string resource for the command", - "properties": { - "key": { - "type": "string" - } - }, "type": [ "string", "object", "null" - ] + ], + "properties": { + "key": { + "type": "string" + } + } }, "iterateOn": { "type": "string", @@ -2174,6 +2235,7 @@ "Globals": { "additionalProperties": true, "description": "Properties that affect the entire window, regardless of the profile settings.", + "type": "object", "properties": { "alwaysOnTop": { "default": false, @@ -2185,7 +2247,7 @@ "description": "When set to true, tabs are always displayed. When set to false and \"showTabsInTitlebar\" is set to false, tabs only appear after opening a new tab.", "type": "boolean" }, - "compatibility.enableUnfocusedAcrylic":{ + "compatibility.enableUnfocusedAcrylic": { "default": true, "description": "When set to true, unfocused windows can have acrylic instead of opaque.", "type": "boolean" @@ -2388,10 +2450,17 @@ "theme": { "default": "dark", "description": "Sets the theme of the application. This value should be the name of one of the themes defined in `themes`. The Terminal also includes the themes `dark`, `light`, and `system`.", - "oneOf": [ + "anyOf": [ { "type": "string" }, + { + "enum": [ + "dark", + "light", + "system" + ] + }, { "$ref": "#/$defs/ThemePair" } @@ -2501,12 +2570,12 @@ }, "required": [ "defaultProfile" - ], - "type": "object" + ] }, "Profile": { "description": "Properties specific to a unique profile.", "additionalProperties": false, + "type": "object", "properties": { "acrylicOpacity": { "default": 0.5, @@ -2558,7 +2627,7 @@ }, "backgroundImage": { "description": "Sets the file location of the image to draw over the window background.", - "oneOf": [ + "anyOf": [ { "type": [ "string", @@ -2570,10 +2639,6 @@ "desktopWallpaper" ] } - ], - "type": [ - "string", - "null" ] }, "backgroundImageAlignment": { @@ -2910,8 +2975,7 @@ "description": "When set to true, the window will have an acrylic material background. When set to false, the window will have a plain, untextured background.", "type": "boolean" } - }, - "type": "object" + } }, "ProfileList": { "description": "A list of profiles and the properties specific to each.", @@ -2926,6 +2990,7 @@ }, "ProfilesObject": { "description": "A list of profiles and default settings that apply to all of them", + "type": "object", "properties": { "list": { "$ref": "#/$defs/ProfileList" @@ -2934,12 +2999,12 @@ "description": "The default settings that apply to every profile.", "$ref": "#/$defs/Profile" } - }, - "type": "object" + } }, "SchemeList": { "description": "Properties are specific to each color scheme. ColorTool is a great tool you can use to create and explore new color schemes. All colors use hex color format.", "items": { + "type": "object", "additionalProperties": false, "properties": { "name": { @@ -3028,8 +3093,7 @@ "$ref": "#/$defs/Color", "description": "Sets the color used as ANSI yellow." } - }, - "type": "object" + } }, "type": "array" } @@ -3039,6 +3103,7 @@ "$ref": "#/$defs/Globals" }, { + "type": "object", "additionalItems": true, "properties": { "profiles": { From f2c3ddd10523c146260a259a1c2ee2a85b264be8 Mon Sep 17 00:00:00 2001 From: AtariDreams <83477269+AtariDreams@users.noreply.github.com> Date: Fri, 13 Oct 2023 16:47:56 -0400 Subject: [PATCH 06/15] Flip bits instead of checking for them (#16160) No need to check for if a bit is set before manually clearing or setting them when xor will do the trick. --- src/tools/vtapp/Program.cs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/tools/vtapp/Program.cs b/src/tools/vtapp/Program.cs index 85f38b0085c..3751b6e4212 100644 --- a/src/tools/vtapp/Program.cs +++ b/src/tools/vtapp/Program.cs @@ -287,15 +287,7 @@ static void Main(string[] args) if (Pinvoke.GetConsoleMode(hCon, out mode)) { - if ((mode & Pinvoke.ENABLE_VIRTUAL_TERMINAL_PROCESSING) != 0) - { - mode &= ~Pinvoke.ENABLE_VIRTUAL_TERMINAL_PROCESSING; - } - else - { - mode |= Pinvoke.ENABLE_VIRTUAL_TERMINAL_PROCESSING; - } - + mode ^= Pinvoke.ENABLE_VIRTUAL_TERMINAL_PROCESSING; Pinvoke.SetConsoleMode(hCon, mode); } break; @@ -307,14 +299,7 @@ static void Main(string[] args) int mode; if (Pinvoke.GetConsoleMode(hCon, out mode)) { - if ((mode & Pinvoke.ENABLE_VIRTUAL_TERMINAL_INPUT) != 0) - { - mode &= ~Pinvoke.ENABLE_VIRTUAL_TERMINAL_INPUT; - } - else - { - mode |= Pinvoke.ENABLE_VIRTUAL_TERMINAL_INPUT; - } + mode ^= Pinvoke.ENABLE_VIRTUAL_TERMINAL_INPUT; mode &= ~Pinvoke.ENABLE_PROCESSED_INPUT; Pinvoke.SetConsoleMode(hCon, mode); } From 174585740759f3e473c5a4efe3207a20d2c78511 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 17 Oct 2023 14:11:54 -0500 Subject: [PATCH 07/15] Fix the color of marks (#16106) Guess what _doesn't_ have the same layout as a bitmap? A `til::color`. Noticed in 1.19. Regressed in #16006 --- src/cascadia/TerminalControl/TermControl.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 8a1c9018809..4406d364c92 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -359,9 +359,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto end = beg + pipHeight * stride; for (; beg < end; beg += stride) { - // Coincidentally a til::color has the same RGBA format as the bitmap. + // a til::color does NOT have the same RGBA format as the bitmap. #pragma warning(suppress : 26490) // Don't use reinterpret_cast (type.1). - std::fill_n(reinterpret_cast(beg), pipWidth, color); + const DWORD c = 0xff << 24 | color.r << 16 | color.g << 8 | color.b; + std::fill_n(reinterpret_cast(beg), pipWidth, c); } }; From 64b5b2884a3ef4dbed5026c73079f4ffcdad6de5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 18 Oct 2023 17:46:16 -0700 Subject: [PATCH 08/15] env: properly handle nulls in REG_SZ strings (#16190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit eb871bf fails to properly handle REG_SZ strings, which are documented as being null-terminated _and_ length restricted. `wcsnlen` is the perfect fit for handling this situation as it returns the position of the first \0, or the given length parameter. As a drive by improvement, this also drops some redundant code: * `to_environment_strings_w` which is the same as `to_string` * Retrieving `USERNAME`/`USERDOMAIN` via `LookupAccountSidW` and `COMPUTERNAME` via `GetComputerNameW` is not necessary as the variables are "volatile" and I believe there's generally no expectation that they change unless you log in again. Closes #16051 ## Validation Steps Performed * Run this in PowerShell to insert a env value with \0: ```pwsh $hklm = [Microsoft.Win32.RegistryKey]::OpenBaseKey( [Microsoft.Win32.RegistryHive]::LocalMachine, 0 ) $key = $hklm.OpenSubKey( 'SYSTEM\CurrentControlSet\Control\Session Manager\Environment', $true ) $key.SetValue('test', "foo`0bar") ``` * All `EnvTests` still pass ✅ * (Don't forget to remove the above value again!) --- .../TerminalConnection/ConptyConnection.cpp | 20 +- src/inc/til/env.h | 229 ++---------------- src/til/ut_til/EnvTests.cpp | 33 ++- 3 files changed, 40 insertions(+), 242 deletions(-) diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index e086c9b2cfc..5b20f0d6ef1 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -82,14 +82,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation nullptr)); auto cmdline{ wil::ExpandEnvironmentStringsW(_commandline.c_str()) }; // mutable copy -- required for CreateProcessW - - til::env environment; - auto zeroEnvMap = wil::scope_exit([&]() noexcept { - environment.clear(); - }); - - // Populate the environment map with the current environment. - environment = _initialEnv; + auto environment = _initialEnv; { // Convert connection Guid to string and ignore the enclosing '{}'. @@ -140,15 +133,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation environment.as_map().insert_or_assign(L"WSLENV", wslEnv); } - std::vector newEnvVars; - auto zeroNewEnv = wil::scope_exit([&]() noexcept { - ::SecureZeroMemory(newEnvVars.data(), - newEnvVars.size() * sizeof(decltype(newEnvVars.begin())::value_type)); - }); - - RETURN_IF_FAILED(environment.to_environment_strings_w(newEnvVars)); - - auto lpEnvironment = newEnvVars.empty() ? nullptr : newEnvVars.data(); + auto newEnvVars = environment.to_string(); + const auto lpEnvironment = newEnvVars.empty() ? nullptr : newEnvVars.data(); // If we have a startingTitle, create a mutable character buffer to add // it to the STARTUPINFO. diff --git a/src/inc/til/env.h b/src/inc/til/env.h index 4d95f9f2f98..1546a83afb6 100644 --- a/src/inc/til/env.h +++ b/src/inc/til/env.h @@ -76,65 +76,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" namespace wil_env { - /** Looks up the computer name and fails if it is not found. */ - template - HRESULT GetComputerNameW(string_type& result) WI_NOEXCEPT - { - return wil::AdaptFixedSizeToAllocatedResult(result, [&](_Out_writes_(valueLength) PWSTR value, size_t valueLength, _Out_ size_t* valueLengthNeededWithNul) -> HRESULT { - // If the function succeeds, the return value is the number of characters stored in the buffer - // pointed to by lpBuffer, not including the terminating null character. - // - // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in - // characters, required to hold the string and its terminating null character and the contents of - // lpBuffer are undefined. - // - // If the function fails, the return value is zero. If the specified environment variable was not - // found in the environment block, GetLastError returns ERROR_ENVVAR_NOT_FOUND. - - ::SetLastError(ERROR_SUCCESS); - - DWORD length = static_cast(valueLength); - - const auto result = ::GetComputerNameW(value, &length); - *valueLengthNeededWithNul = length; - RETURN_IF_WIN32_BOOL_FALSE_EXPECTED(result); - if (*valueLengthNeededWithNul < valueLength) - { - (*valueLengthNeededWithNul)++; // It fit, account for the null. - } - return S_OK; - }); - } - - /** Looks up the computer name and returns null if it is not found. */ - template - HRESULT TryGetComputerNameW(string_type& result) WI_NOEXCEPT - { - const auto hr = wil_env::GetComputerNameW(result); - RETURN_HR_IF(hr, FAILED(hr) && (hr != HRESULT_FROM_WIN32(ERROR_ENVVAR_NOT_FOUND))); - return S_OK; - } - -#ifdef WIL_ENABLE_EXCEPTIONS - /** Looks up the computer name and fails if it is not found. */ - template - string_type GetComputerNameW() - { - string_type result; - THROW_IF_FAILED((wil_env::GetComputerNameW(result))); - return result; - } - - /** Looks up the computer name and returns null if it is not found. */ - template - string_type TryGetComputerNameW() - { - string_type result; - THROW_IF_FAILED((wil_env::TryGetComputerNameW(result))); - return result; - } -#endif - /** Looks up a registry value from 'key' and fails if it is not found. */ template HRESULT RegQueryValueExW(HKEY key, PCWSTR valueName, string_type& result) WI_NOEXCEPT @@ -231,41 +172,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" } } - void get_computer_name() - { - if (auto value = til::details::wil_env::TryGetComputerNameW(); !value.empty()) - { - save_to_map(std::wstring{ til::details::vars::computer_name }, std::move(value)); - } - } - - void get_user_name_and_domain() - try - { - const auto token = wil::open_current_access_token(); - const auto user = wil::get_token_information(token.get()); - - DWORD accountNameSize = 0, userDomainSize = 0; - SID_NAME_USE sidNameUse; - SetLastError(ERROR_SUCCESS); - if (LookupAccountSidW(nullptr, user.get()->User.Sid, nullptr, &accountNameSize, nullptr, &userDomainSize, &sidNameUse) || GetLastError() == ERROR_INSUFFICIENT_BUFFER) - { - std::wstring accountName, userDomain; - accountName.resize(accountNameSize); - userDomain.resize(userDomainSize); - - SetLastError(ERROR_SUCCESS); - if (LookupAccountSidW(nullptr, user.get()->User.Sid, accountName.data(), &accountNameSize, userDomain.data(), &userDomainSize, &sidNameUse)) - { - strip_trailing_null(accountName); - strip_trailing_null(userDomain); - save_to_map(std::wstring{ til::details::vars::user_name }, std::move(accountName)); - save_to_map(std::wstring{ til::details::vars::user_domain }, std::move(userDomain)); - } - } - } - CATCH_LOG() - void get_program_files() { wil::unique_hkey key; @@ -325,23 +231,18 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" std::wstring data; if (pass == 0 && (type == REG_SZ) && valueDataSize >= sizeof(wchar_t)) { - data = { - reinterpret_cast(valueData.data()), valueData.size() / sizeof(wchar_t) - }; + const auto p = reinterpret_cast(valueData.data()); + const auto l = wcsnlen(p, valueData.size() / sizeof(wchar_t)); + data.assign(p, l); } else if (pass == 1 && (type == REG_EXPAND_SZ) && valueDataSize >= sizeof(wchar_t)) { - data = { - reinterpret_cast(valueData.data()), valueData.size() / sizeof(wchar_t) - }; + const auto p = reinterpret_cast(valueData.data()); + const auto l = wcsnlen(p, valueData.size() / sizeof(wchar_t)); + data.assign(p, l); data = expand_environment_strings(data.data()); } - // Because Registry data may or may not be null terminated... check if we've managed - // to store an extra null in the wstring by telling it to create itself from pointer and size. - // If we did, pull it off. - strip_trailing_null(data); - if (!data.empty()) { if (isPathVar) @@ -468,14 +369,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" til::compare_string_ordinal(input, os2LibPath) == CSTR_EQUAL; } - static void strip_trailing_null(std::wstring& str) noexcept - { - if (!str.empty() && str.back() == L'\0') - { - str.pop_back(); - } - } - void parse(const wchar_t* lastCh) { for (; *lastCh != '\0'; ++lastCh) @@ -537,13 +430,20 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" void regenerate() { // Generally replicates the behavior of shell32!RegenerateUserEnvironment + // The difference is that we don't + // * handle autoexec.bat (intentionally unsupported). + // * call LookupAccountSidW to get the USERNAME/USERDOMAIN variables. + // Windows sets these as values of the "Volatile Environment" key at each login. + // We don't expect our process to impersonate another user so we can get them from the PEB. + // * call GetComputerNameW to get the COMPUTERNAME variable, for the same reason. get(til::details::vars::system_root); get(til::details::vars::system_drive); get(til::details::vars::all_users_profile); get(til::details::vars::public_var); get(til::details::vars::program_data); - get_computer_name(); - get_user_name_and_domain(); + get(til::details::vars::computer_name); + get(til::details::vars::user_name); + get(til::details::vars::user_domain); get(til::details::vars::user_dns_domain); get(til::details::vars::home_drive); get(til::details::vars::home_share); @@ -562,104 +462,17 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" std::wstring to_string() const { std::wstring result; - for (const auto& [name, value] : _envMap) + for (const auto& [k, v] : _envMap) { - result += name; - result += L"="; - result += value; - result.append(L"\0", 1); // Override string's natural propensity to stop at \0 + result.append(k); + result.push_back(L'='); + result.append(v); + result.push_back(L'\0'); } - result.append(L"\0", 1); + result.push_back(L'\0'); 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/til/ut_til/EnvTests.cpp b/src/til/ut_til/EnvTests.cpp index 1a9fd53c603..f0d2e97cd95 100644 --- a/src/til/ut_til/EnvTests.cpp +++ b/src/til/ut_til/EnvTests.cpp @@ -9,7 +9,7 @@ using namespace WEX::Common; using namespace WEX::Logging; using namespace WEX::TestExecution; -BOOL APIENTRY RegenerateUserEnvironment(void** pNewEnv, BOOL bSetCurrentEnv); +BOOL APIENTRY RegenerateUserEnvironment(wchar_t** pNewEnv, BOOL bSetCurrentEnv); class EnvTests { @@ -26,9 +26,11 @@ class EnvTests wil::unique_hmodule shell32{ LoadLibraryW(L"shell32.dll") }; auto func = GetProcAddressByFunctionDeclaration(shell32.get(), RegenerateUserEnvironment); + wchar_t* newEnvPtr = nullptr; + VERIFY_WIN32_BOOL_SUCCEEDED(func(&newEnvPtr, FALSE)); + // Use a WIL pointer to free it on scope exit. - wil::unique_environstrings_ptr newEnv; - VERIFY_WIN32_BOOL_SUCCEEDED(func((void**)&newEnv, FALSE)); + const wil::unique_environstrings_ptr newEnv{ newEnvPtr }; // Parse the string into our environment table. til::env expected{ newEnv.get() }; @@ -38,12 +40,15 @@ class EnvTests actual.regenerate(); VERIFY_ARE_EQUAL(expected.as_map().size(), actual.as_map().size()); - for (const auto& [expectedKey, expectedValue] : expected.as_map()) + + const auto expectedEnd = expected.as_map().end(); + auto expectedIt = expected.as_map().begin(); + auto actualIt = actual.as_map().begin(); + + for (; expectedIt != expectedEnd; ++expectedIt, ++actualIt) { - Log::Comment(String().Format(L"Environment Variable: '%s'", expectedKey.data())); - const auto actualValue = actual.as_map()[expectedKey]; - VERIFY_IS_TRUE(actual.as_map().find(expectedKey) != actual.as_map().end()); - VERIFY_ARE_EQUAL(expectedValue, actual.as_map()[expectedKey]); + VERIFY_ARE_EQUAL(expectedIt->first, actualIt->first); + VERIFY_ARE_EQUAL(expectedIt->second, actualIt->second); } } @@ -54,16 +59,10 @@ class EnvTests environment.as_map().insert_or_assign(L"B", L"Banana"); environment.as_map().insert_or_assign(L"C", L"Cassowary"); - wchar_t expectedArray[] = L"A=Apple\0B=Banana\0C=Cassowary\0"; - - const std::wstring expected{ expectedArray, ARRAYSIZE(expectedArray) }; + static constexpr wchar_t expectedArray[] = L"A=Apple\0B=Banana\0C=Cassowary\0"; + static constexpr std::wstring_view expected{ expectedArray, std::size(expectedArray) }; const auto& actual = environment.to_string(); - - VERIFY_ARE_EQUAL(expected.size(), actual.size()); - for (size_t i = 0; i < expected.size(); ++i) - { - VERIFY_ARE_EQUAL(expected[i], actual[i]); - } + VERIFY_ARE_EQUAL(expected, actual); } TEST_METHOD(TestExpandEnvironmentStrings) From 08f30330d15e22adf23ee04164fc2474f290348a Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 18 Oct 2023 17:47:19 -0700 Subject: [PATCH 09/15] COOKED_READ: Fix reference counting woes (#16187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This restores the original code from before 821ae3a where the `.GetMainBuffer()` call was accidentally removed. Closes #16158 ## Validation Steps Performed * Run this Python script: ```py import sys while True: sys.stdout.write("\033[?1049h") sys.stdout.flush() sys.stdin.readline() sys.stdout.write("\033[?1049l") ``` * Press enter repeatedly * Doesn't crash ✅ --- .github/actions/spelling/expect/expect.txt | 1 + src/host/ft_host/API_BufferTests.cpp | 37 ++++++++++++++++++++++ src/host/readDataCooked.cpp | 6 +++- 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 4339b17eb04..31775457fff 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -303,6 +303,7 @@ coordnew COPYCOLOR CORESYSTEM cotaskmem +countof CPG cpinfo CPINFOEX diff --git a/src/host/ft_host/API_BufferTests.cpp b/src/host/ft_host/API_BufferTests.cpp index 362b8353aef..242427e2b9a 100644 --- a/src/host/ft_host/API_BufferTests.cpp +++ b/src/host/ft_host/API_BufferTests.cpp @@ -3,6 +3,8 @@ #include "precomp.h" +#include "../../types/inc/IInputEvent.hpp" + extern "C" IMAGE_DOS_HEADER __ImageBase; using namespace WEX::Logging; @@ -17,6 +19,7 @@ class BufferTests END_TEST_CLASS() TEST_METHOD(TestSetConsoleActiveScreenBufferInvalid); + TEST_METHOD(TestCookedReadBufferReferenceCount); TEST_METHOD(TestCookedReadOnNonShareableScreenBuffer); @@ -83,6 +86,40 @@ void BufferTests::TestCookedReadOnNonShareableScreenBuffer() VERIFY_IS_TRUE(IsConsoleStillRunning()); } +// This test ensures that COOKED_READ_DATA properly holds onto the screen buffer it is +// reading from for the whole duration of the read. It's important that we hold a handle +// to the main instead of the alt buffer even if this cooked read targets the latter, +// because alt buffers are fake SCREEN_INFORMATION objects that are owned by the main buffer. +void BufferTests::TestCookedReadBufferReferenceCount() +{ + static constexpr int loops = 5; + + const auto in = GetStdInputHandle(); + const auto out = GetStdOutputHandle(); + + DWORD inMode = 0; + GetConsoleMode(out, &inMode); + inMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_PROCESSED_OUTPUT; + SetConsoleMode(out, inMode); + + INPUT_RECORD newlines[loops]; + DWORD written = 0; + std::fill_n(&newlines[0], loops, SynthesizeKeyEvent(true, 1, L'\r', 0, L'\r', 0)); + WriteConsoleInputW(in, &newlines[0], loops, &written); + + for (int i = 0; i < loops; ++i) + { + VERIFY_SUCCEEDED(WriteConsoleW(out, L"\033[?1049h", 8, nullptr, nullptr)); + + wchar_t buffer[16]; + DWORD read = 0; + VERIFY_SUCCEEDED(ReadConsoleW(in, &buffer[0], _countof(buffer), &read, nullptr)); + VERIFY_ARE_EQUAL(2u, read); + + VERIFY_SUCCEEDED(WriteConsoleW(out, L"\033[?1049l", 8, nullptr, nullptr)); + } +} + void BufferTests::TestWritingInactiveScreenBuffer() { bool useVtOutput; diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp index d05502dc0d0..36d54583fbb 100644 --- a/src/host/readDataCooked.cpp +++ b/src/host/readDataCooked.cpp @@ -62,7 +62,11 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer, // We need to ensure that it stays alive for the duration of the read. // Coincidentally this serves another important purpose: It checks whether we're allowed to read from // the given buffer in the first place. If it's missing the FILE_SHARE_READ flag, we can't read from it. - THROW_IF_FAILED(_screenInfo.AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle)); + // + // GH#16158: It's important that we hold a handle to the main instead of the alt buffer + // even if this cooked read targets the latter, because alt buffers are fake + // SCREEN_INFORMATION objects that are owned by the main buffer. + THROW_IF_FAILED(_screenInfo.GetMainBuffer().AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle)); #endif if (!initialData.empty()) From d496a5fb80d5ec002077d6be27a35ec7494cb520 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 20 Oct 2023 18:10:31 +0200 Subject: [PATCH 10/15] Fix rectangular clipboard copying initiated from the app menu (#16197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cd6b083 had 2 issues: * Improper testing with Ctrl+M instead of Edit > Mark. * Wrong SelectionState function being used. When the selection is initiated without keyboard or mouse, `IsKeyboardMarkSelection` returns false. The proper function to use is `IsLineSelection`. Closes #15153 ## Validation Steps Performed * Run Far * Start selection via Edit>Mark * Hold Alt while dragging to make a rectangular selection * Right click * Clipboard contains a rectangular copy ✅ --- src/interactivity/win32/Clipboard.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interactivity/win32/Clipboard.cpp b/src/interactivity/win32/Clipboard.cpp index 30138f6dad6..be97047404c 100644 --- a/src/interactivity/win32/Clipboard.cpp +++ b/src/interactivity/win32/Clipboard.cpp @@ -248,7 +248,7 @@ void Clipboard::StoreSelectionToClipboard(const bool copyFormatting) trimTrailingWhitespace, selectionRects, GetAttributeColors, - selection.IsKeyboardMarkSelection()); + !selection.IsLineSelection()); CopyTextToSystemClipboard(text, copyFormatting); } From e1c69a99cef98f293a17100752f67ab7e6584868 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 23 Oct 2023 17:27:01 -0700 Subject: [PATCH 11/15] Fix UIA and marks regressions due to cooked read (#16105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial cooked read (= conhost readline) rewrite had two flaws: * Using viewport scrolls under ConPTY to avoid emitting newlines resulted in various bugs around marks, coloring, etc. It's still somewhat unclear why this happened, but the next issue is related and much worse. * Rewriting the input line every time causes problems with accessibility tools, as they'll re-announce unchanged parts again and again. The solution to these is to simply stop writing the unchanged parts of the prompt. To do this, code was added to measure the size of text without actually inserting them into the buffer. Since this meant that the "interactive" mode of `WriteCharsLegacy` would need to be duplicated for the new code, I instead moved those parts into `COOKED_READ_DATA`. That way we can now have the interactive transform of the prompt (= Ctrl+C -> ^C) and the two text functions (measure text & actually write text) are now agnostic to this transformation. Closes #16034 Closes #16044 ## Validation Steps Performed * A vision impaired user checked it out and it seemed fine ✅ --- .github/actions/spelling/expect/expect.txt | 1 + src/buffer/out/textBuffer.cpp | 82 +++ src/buffer/out/textBuffer.hpp | 1 + .../ConptyRoundtripTests.cpp | 4 +- src/common.build.pre.props | 2 +- src/host/_stream.cpp | 112 ++--- src/host/_stream.h | 2 +- src/host/ft_fuzzer/fuzzmain.cpp | 2 +- src/host/readDataCooked.cpp | 470 ++++++++++++------ src/host/readDataCooked.hpp | 46 +- src/host/ut_host/ScreenBufferTests.cpp | 10 +- 11 files changed, 492 insertions(+), 240 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 31775457fff..3ea1b7e958f 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1776,6 +1776,7 @@ stdafx STDAPI stdc stdcpp +STDEXT STDMETHODCALLTYPE STDMETHODIMP STGM diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index c4c66ca92e4..52a5c6e3b72 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -416,6 +416,88 @@ size_t TextBuffer::GraphemePrev(const std::wstring_view& chars, size_t position) return til::utf16_iterate_prev(chars, position); } +// Ever wondered how much space a piece of text needs before inserting it? This function will tell you! +// It fundamentally behaves identical to the various ROW functions around `RowWriteState`. +// +// Set `columnLimit` to the amount of space that's available (e.g. `buffer_width - cursor_position.x`) +// and it'll return the amount of characters that fit into this space. The out parameter `columns` +// will contain the amount of columns this piece of text has actually used. +// +// Just like with `RowWriteState` one special case is when not all text fits into the given space: +// In that case, this function always returns exactly `columnLimit`. This distinction is important when "inserting" +// a wide glyph but there's only 1 column left. That 1 remaining column is supposed to be padded with whitespace. +size_t TextBuffer::FitTextIntoColumns(const std::wstring_view& chars, til::CoordType columnLimit, til::CoordType& columns) noexcept +{ + columnLimit = std::max(0, columnLimit); + + const auto beg = chars.begin(); + const auto end = chars.end(); + auto it = beg; + const auto asciiEnd = beg + std::min(chars.size(), gsl::narrow_cast(columnLimit)); + + // ASCII fast-path: 1 char always corresponds to 1 column. + for (; it != asciiEnd && *it < 0x80; ++it) + { + } + + const auto dist = gsl::narrow_cast(it - beg); + auto col = gsl::narrow_cast(dist); + + if (it == asciiEnd) [[likely]] + { + columns = col; + return dist; + } + + // Unicode slow-path where we need to count text and columns separately. + for (;;) + { + auto ptr = &*it; + const auto wch = *ptr; + size_t len = 1; + + col++; + + // Even in our slow-path we can avoid calling IsGlyphFullWidth if the current character is ASCII. + // It also allows us to skip the surrogate pair decoding at the same time. + if (wch >= 0x80) + { + if (til::is_surrogate(wch)) + { + const auto it2 = it + 1; + if (til::is_leading_surrogate(wch) && it2 != end && til::is_trailing_surrogate(*it2)) + { + len = 2; + } + else + { + ptr = &UNICODE_REPLACEMENT; + } + } + + col += IsGlyphFullWidth({ ptr, len }); + } + + // If we ran out of columns, we need to always return `columnLimit` and not `cols`, + // because if we tried inserting a wide glyph into just 1 remaining column it will + // fail to fit, but that remaining column still has been used up. When the caller sees + // `columns == columnLimit` they will line-wrap and continue inserting into the next row. + if (col > columnLimit) + { + columns = columnLimit; + return gsl::narrow_cast(it - beg); + } + + // But if we simply ran out of text we just need to return the actual number of columns. + it += len; + if (it == end) + { + columns = col; + return chars.size(); + } + } +} + // Pretend as if `position` is a regular cursor in the TextBuffer. // This function will then pretend as if you pressed the left/right arrow // keys `distance` amount of times (negative = left, positive = right). diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index a9d2e1714b6..8ba16f97e75 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -137,6 +137,7 @@ class TextBuffer final static size_t GraphemeNext(const std::wstring_view& chars, size_t position) noexcept; static size_t GraphemePrev(const std::wstring_view& chars, size_t position) noexcept; + static size_t FitTextIntoColumns(const std::wstring_view& chars, til::CoordType columnLimit, til::CoordType& columns) noexcept; til::point NavigateCursor(til::point position, til::CoordType distance) const; diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 99c2bd02ec9..48eeacf53c6 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -3243,7 +3243,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottom() } else if (writingMethod == PrintWithWriteCharsLegacy) { - WriteCharsLegacy(si, str, false, nullptr); + WriteCharsLegacy(si, str, nullptr); } }; @@ -3401,7 +3401,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS() } else if (writingMethod == PrintWithWriteCharsLegacy) { - WriteCharsLegacy(si, str, false, nullptr); + WriteCharsLegacy(si, str, nullptr); } }; diff --git a/src/common.build.pre.props b/src/common.build.pre.props index 9404c24c4bd..a1cf37adbda 100644 --- a/src/common.build.pre.props +++ b/src/common.build.pre.props @@ -133,7 +133,7 @@ We didn't and it breaks WRL and large parts of conhost code. --> 4201;4312;4467;5105;26434;26445;26456;%(DisableSpecificWarnings) - _WINDOWS;EXTERNAL_BUILD;%(PreprocessorDefinitions) + _WINDOWS;EXTERNAL_BUILD;_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;%(PreprocessorDefinitions) true precomp.h ProgramDatabase diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp index bf9e32ed38b..8d884e7136e 100644 --- a/src/host/_stream.cpp +++ b/src/host/_stream.cpp @@ -39,7 +39,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine; // - coordCursor - New location of cursor. // - fKeepCursorVisible - TRUE if changing window origin desirable when hit right edge // Return Value: -static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, const bool interactive, _Inout_opt_ til::CoordType* psScrollY) +static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, _Inout_opt_ til::CoordType* psScrollY) { const auto bufferSize = screenInfo.GetBufferSize().Dimensions(); if (coordCursor.x < 0) @@ -70,42 +70,19 @@ static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point if (coordCursor.y >= bufferSize.height) { - const auto vtIo = ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo(); - const auto renderer = ServiceLocator::LocateGlobals().pRender; - const auto needsConPTYWorkaround = interactive && vtIo->IsUsingVt(); auto& buffer = screenInfo.GetTextBuffer(); - const auto isActiveBuffer = buffer.IsActiveBuffer(); + buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes()); - // ConPTY translates scrolling into newlines. We don't want that during cooked reads (= "cmd.exe prompts") - // because the entire prompt is supposed to fit into the VT viewport, with no scrollback. If we didn't do that, - // any prompt larger than the viewport will cause >1 lines to be added to scrollback, even if typing backspaces. - // You can test this by removing this branch, launch Windows Terminal, and fill the entire viewport with text in cmd.exe. - if (needsConPTYWorkaround) + if (buffer.IsActiveBuffer()) { - buffer.SetAsActiveBuffer(false); - buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes()); - buffer.SetAsActiveBuffer(isActiveBuffer); - - if (isActiveBuffer && renderer) + if (const auto notifier = ServiceLocator::LocateAccessibilityNotifier()) { - renderer->TriggerRedrawAll(); + notifier->NotifyConsoleUpdateScrollEvent(0, -1); } - } - else - { - buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes()); - - if (isActiveBuffer) + if (const auto renderer = ServiceLocator::LocateGlobals().pRender) { - if (const auto notifier = ServiceLocator::LocateAccessibilityNotifier()) - { - notifier->NotifyConsoleUpdateScrollEvent(0, -1); - } - if (renderer) - { - static constexpr til::point delta{ 0, -1 }; - renderer->TriggerScroll(&delta); - } + static constexpr til::point delta{ 0, -1 }; + renderer->TriggerScroll(&delta); } } @@ -128,15 +105,11 @@ static void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point LOG_IF_FAILED(screenInfo.SetViewportOrigin(false, WindowOrigin, true)); } - if (interactive) - { - screenInfo.MakeCursorVisible(coordCursor); - } - LOG_IF_FAILED(screenInfo.SetCursorPosition(coordCursor, interactive)); + LOG_IF_FAILED(screenInfo.SetCursorPosition(coordCursor, false)); } // As the name implies, this writes text without processing its control characters. -static void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, const bool interactive, til::CoordType* psScrollY) +void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, til::CoordType* psScrollY) { const auto wrapAtEOL = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_WRAP_AT_EOL_OUTPUT); const auto hasAccessibilityEventing = screenInfo.HasAccessibilityEventing(); @@ -165,18 +138,19 @@ static void _writeCharsLegacyUnprocessed(SCREEN_INFORMATION& screenInfo, const s screenInfo.NotifyAccessibilityEventing(state.columnBegin, cursorPosition.y, state.columnEnd - 1, cursorPosition.y); } - AdjustCursorPosition(screenInfo, cursorPosition, interactive, psScrollY); + AdjustCursorPosition(screenInfo, cursorPosition, psScrollY); } } // This routine writes a string to the screen while handling control characters. // `interactive` exists for COOKED_READ_DATA which uses it to transform control characters into visible text like "^X". // Similarly, `psScrollY` is also used by it to track whether the underlying buffer circled. It requires this information to know where the input line moved to. -void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, const bool interactive, til::CoordType* psScrollY) +void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& text, til::CoordType* psScrollY) { static constexpr wchar_t tabSpaces[8]{ L' ', L' ', L' ', L' ', L' ', L' ', L' ', L' ' }; auto& textBuffer = screenInfo.GetTextBuffer(); + const auto width = textBuffer.GetSize().Width(); auto& cursor = textBuffer.GetCursor(); const auto wrapAtEOL = WI_IsFlagSet(screenInfo.OutputMode, ENABLE_WRAP_AT_EOL_OUTPUT); auto it = text.begin(); @@ -197,15 +171,15 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t { pos.x = 0; pos.y++; - AdjustCursorPosition(screenInfo, pos, interactive, psScrollY); + AdjustCursorPosition(screenInfo, pos, psScrollY); } } // If ENABLE_PROCESSED_OUTPUT is set we search for C0 control characters and handle them like backspace, tab, etc. - // If it's not set, we can just straight up give everything to _writeCharsLegacyUnprocessed. + // If it's not set, we can just straight up give everything to WriteCharsLegacyUnprocessed. if (WI_IsFlagClear(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)) { - _writeCharsLegacyUnprocessed(screenInfo, { it, end }, interactive, psScrollY); + _writeCharsLegacyUnprocessed(screenInfo, { it, end }, psScrollY); it = end; } @@ -214,7 +188,7 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t const auto nextControlChar = std::find_if(it, end, [](const auto& wch) { return !IS_GLYPH_CHAR(wch); }); if (nextControlChar != it) { - _writeCharsLegacyUnprocessed(screenInfo, { it, nextControlChar }, interactive, psScrollY); + _writeCharsLegacyUnprocessed(screenInfo, { it, nextControlChar }, psScrollY); it = nextControlChar; } @@ -223,35 +197,24 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t switch (*it) { case UNICODE_NULL: - if (interactive) - { - break; - } - _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], 1 }, interactive, psScrollY); + _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], 1 }, psScrollY); continue; case UNICODE_BELL: - if (interactive) - { - break; - } std::ignore = screenInfo.SendNotifyBeep(); continue; case UNICODE_BACKSPACE: { - // Backspace handling for interactive mode should happen in COOKED_READ_DATA - // where it has full control over the text and can delete it directly. - // Otherwise handling backspacing tabs/whitespace can turn up complex and bug-prone. - assert(!interactive); auto pos = cursor.GetPosition(); pos.x = textBuffer.GetRowByOffset(pos.y).NavigateToPrevious(pos.x); - AdjustCursorPosition(screenInfo, pos, interactive, psScrollY); + AdjustCursorPosition(screenInfo, pos, psScrollY); continue; } case UNICODE_TAB: { const auto pos = cursor.GetPosition(); - const auto tabCount = gsl::narrow_cast(8 - (pos.x & 7)); - _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], tabCount }, interactive, psScrollY); + const auto remaining = width - pos.x; + const auto tabCount = gsl::narrow_cast(std::min(remaining, 8 - (pos.x & 7))); + _writeCharsLegacyUnprocessed(screenInfo, { &tabSpaces[0], tabCount }, psScrollY); continue; } case UNICODE_LINEFEED: @@ -264,38 +227,29 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t textBuffer.GetMutableRowByOffset(pos.y).SetWrapForced(false); pos.y = pos.y + 1; - AdjustCursorPosition(screenInfo, pos, interactive, psScrollY); + AdjustCursorPosition(screenInfo, pos, psScrollY); continue; } case UNICODE_CARRIAGERETURN: { auto pos = cursor.GetPosition(); pos.x = 0; - AdjustCursorPosition(screenInfo, pos, interactive, psScrollY); + AdjustCursorPosition(screenInfo, pos, psScrollY); continue; } default: break; } - // In the interactive mode we replace C0 control characters (0x00-0x1f) with ASCII representations like ^C (= 0x03). - if (interactive && *it < L' ') + // As a special favor to incompetent apps that attempt to display control chars, + // convert to corresponding OEM Glyph Chars + const auto cp = ServiceLocator::LocateGlobals().getConsoleInformation().OutputCP; + const auto ch = gsl::narrow_cast(*it); + wchar_t wch = 0; + const auto result = MultiByteToWideChar(cp, MB_USEGLYPHCHARS, &ch, 1, &wch, 1); + if (result == 1) { - const wchar_t wchs[2]{ L'^', static_cast(*it + L'@') }; - _writeCharsLegacyUnprocessed(screenInfo, { &wchs[0], 2 }, interactive, psScrollY); - } - else - { - // As a special favor to incompetent apps that attempt to display control chars, - // convert to corresponding OEM Glyph Chars - const auto cp = ServiceLocator::LocateGlobals().getConsoleInformation().OutputCP; - const auto ch = gsl::narrow_cast(*it); - wchar_t wch = 0; - const auto result = MultiByteToWideChar(cp, MB_USEGLYPHCHARS, &ch, 1, &wch, 1); - if (result == 1) - { - _writeCharsLegacyUnprocessed(screenInfo, { &wch, 1 }, interactive, psScrollY); - } + _writeCharsLegacyUnprocessed(screenInfo, { &wch, 1 }, psScrollY); } } } @@ -358,7 +312,7 @@ try if (WI_IsAnyFlagClear(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_PROCESSED_OUTPUT)) { - WriteCharsLegacy(screenInfo, str, false, nullptr); + WriteCharsLegacy(screenInfo, str, nullptr); } else { diff --git a/src/host/_stream.h b/src/host/_stream.h index 794591a03f0..79eb84585dc 100644 --- a/src/host/_stream.h +++ b/src/host/_stream.h @@ -19,7 +19,7 @@ Revision History: #include "writeData.hpp" -void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str, bool interactive, til::CoordType* psScrollY); +void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str, til::CoordType* psScrollY); // NOTE: console lock must be held when calling this routine // String has been translated to unicode at this point. diff --git a/src/host/ft_fuzzer/fuzzmain.cpp b/src/host/ft_fuzzer/fuzzmain.cpp index cd3708d59ed..59ee6ce05c8 100644 --- a/src/host/ft_fuzzer/fuzzmain.cpp +++ b/src/host/ft_fuzzer/fuzzmain.cpp @@ -132,6 +132,6 @@ extern "C" __declspec(dllexport) int LLVMFuzzerTestOneInput(const uint8_t* data, til::CoordType scrollY{}; gci.LockConsole(); auto u = wil::scope_exit([&]() { gci.UnlockConsole(); }); - WriteCharsLegacy(gci.GetActiveOutputBuffer(), u16String, true, &scrollY); + WriteCharsLegacy(gci.GetActiveOutputBuffer(), u16String, &scrollY); return 0; } diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp index 36d54583fbb..3644dfe78db 100644 --- a/src/host/readDataCooked.cpp +++ b/src/host/readDataCooked.cpp @@ -28,6 +28,84 @@ constexpr int integerLog10(uint32_t v) 0; } +const std::wstring& COOKED_READ_DATA::BufferState::Get() const noexcept +{ + return _buffer; +} + +void COOKED_READ_DATA::BufferState::Replace(size_t offset, size_t remove, const wchar_t* input, size_t count) +{ + const auto size = _buffer.size(); + offset = std::min(offset, size); + remove = std::min(remove, size - offset); + + _buffer.replace(offset, remove, input, count); + _cursor = offset + count; + _dirtyBeg = std::min(_dirtyBeg, offset); +} + +void COOKED_READ_DATA::BufferState::Replace(const std::wstring_view& str) +{ + _buffer.assign(str); + _cursor = _buffer.size(); + _dirtyBeg = 0; +} + +size_t COOKED_READ_DATA::BufferState::GetCursorPosition() const noexcept +{ + return _cursor; +} + +void COOKED_READ_DATA::BufferState::SetCursorPosition(size_t pos) noexcept +{ + const auto size = _buffer.size(); + _cursor = std::min(pos, size); + // This ensures that _dirtyBeg isn't npos, which ensures that IsClean() returns false. + _dirtyBeg = std::min(_dirtyBeg, size); +} + +bool COOKED_READ_DATA::BufferState::IsClean() const noexcept +{ + return _dirtyBeg == npos; +} + +void COOKED_READ_DATA::BufferState::MarkEverythingDirty() noexcept +{ + _dirtyBeg = 0; +} + +void COOKED_READ_DATA::BufferState::MarkAsClean() noexcept +{ + _dirtyBeg = npos; +} + +std::wstring_view COOKED_READ_DATA::BufferState::GetUnmodifiedTextBeforeCursor() const noexcept +{ + return _slice(0, std::min(_dirtyBeg, _cursor)); +} + +std::wstring_view COOKED_READ_DATA::BufferState::GetUnmodifiedTextAfterCursor() const noexcept +{ + return _slice(_cursor, _dirtyBeg); +} + +std::wstring_view COOKED_READ_DATA::BufferState::GetModifiedTextBeforeCursor() const noexcept +{ + return _slice(_dirtyBeg, _cursor); +} + +std::wstring_view COOKED_READ_DATA::BufferState::GetModifiedTextAfterCursor() const noexcept +{ + return _slice(std::max(_dirtyBeg, _cursor), npos); +} + +std::wstring_view COOKED_READ_DATA::BufferState::_slice(size_t from, size_t to) const noexcept +{ + to = std::min(to, _buffer.size()); + from = std::min(from, to); + return std::wstring_view{ _buffer.data() + from, to - from }; +} + // Routine Description: // - Constructs cooked read data class to hold context across key presses while a user is modifying their 'input line'. // Arguments: @@ -71,9 +149,7 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer, if (!initialData.empty()) { - _buffer.assign(initialData); - _bufferCursor = _buffer.size(); - _bufferDirty = !_buffer.empty(); + _buffer.Replace(initialData); // The console API around `nInitialChars` in `CONSOLE_READCONSOLE_CONTROL` is pretty weird. // The way it works is that cmd.exe does a ReadConsole() with a `dwCtrlWakeupMask` that includes \t, @@ -232,9 +308,9 @@ void COOKED_READ_DATA::EraseBeforeResize() if (_distanceEnd) { - _unwindCursorPosition(_distanceCursor); + _offsetCursorPosition(-_distanceCursor); _erase(_distanceEnd); - _unwindCursorPosition(_distanceEnd); + _offsetCursorPosition(-_distanceEnd); _distanceCursor = 0; _distanceEnd = 0; } @@ -243,7 +319,7 @@ void COOKED_READ_DATA::EraseBeforeResize() // The counter-part to EraseBeforeResize(). void COOKED_READ_DATA::RedrawAfterResize() { - _markAsDirty(); + _buffer.MarkEverythingDirty(); _flushBuffer(); } @@ -254,7 +330,7 @@ void COOKED_READ_DATA::SetInsertMode(bool insertMode) noexcept bool COOKED_READ_DATA::IsEmpty() const noexcept { - return _buffer.empty() && _popups.empty(); + return _buffer.Get().empty() && _popups.empty(); } bool COOKED_READ_DATA::PresentingPopup() const noexcept @@ -367,8 +443,7 @@ void COOKED_READ_DATA::_handleChar(wchar_t wch, const DWORD modifiers) // Press tab past the "f" in the string "foo" and you'd get "f\to " (a trailing whitespace; the initial contents of the buffer back then). // It's unclear whether the original intention was to write at the end of the buffer at all times or to implement an insert mode. // I went with insert mode. - _buffer.insert(_bufferCursor, 1, wch); - _bufferCursor++; + _buffer.Replace(_buffer.GetCursorPosition(), 0, &wch, 1); _controlKeyState = modifiers; _transitionState(State::DoneWithWakeupMask); @@ -380,8 +455,7 @@ void COOKED_READ_DATA::_handleChar(wchar_t wch, const DWORD modifiers) case UNICODE_CARRIAGERETURN: { // NOTE: Don't append newlines to the buffer just yet! See _handlePostCharInputLoop for more information. - _bufferCursor = _buffer.size(); - _markAsDirty(); + _buffer.SetCursorPosition(npos); _transitionState(State::DoneWithCarriageReturn); return; } @@ -389,29 +463,9 @@ void COOKED_READ_DATA::_handleChar(wchar_t wch, const DWORD modifiers) case UNICODE_BACKSPACE: if (WI_IsFlagSet(_pInputBuffer->InputMode, ENABLE_PROCESSED_INPUT)) { - size_t pos; - if (wch == EXTKEY_ERASE_PREV_WORD) - { - pos = _wordPrev(_buffer, _bufferCursor); - } - else - { - pos = TextBuffer::GraphemePrev(_buffer, _bufferCursor); - } - - _buffer.erase(pos, _bufferCursor - pos); - _bufferCursor = pos; - _markAsDirty(); - - // Notify accessibility to read the backspaced character. - // See GH:12735, MSFT:31748387 - if (_screenInfo.HasAccessibilityEventing()) - { - if (const auto pConsoleWindow = ServiceLocator::LocateConsoleWindow()) - { - LOG_IF_FAILED(pConsoleWindow->SignalUia(UIA_Text_TextChangedEventId)); - } - } + const auto cursor = _buffer.GetCursorPosition(); + const auto pos = wch == EXTKEY_ERASE_PREV_WORD ? _wordPrev(_buffer.Get(), cursor) : TextBuffer::GraphemePrev(_buffer.Get(), cursor); + _buffer.Replace(pos, cursor - pos, nullptr, 0); return; } // If processed mode is disabled, control characters like backspace are treated like any other character. @@ -420,20 +474,16 @@ void COOKED_READ_DATA::_handleChar(wchar_t wch, const DWORD modifiers) break; } - if (_insertMode) - { - _buffer.insert(_bufferCursor, 1, wch); - } - else + size_t remove = 0; + if (!_insertMode) { // TODO GH#15875: If the input grapheme is >1 char, then this will replace >1 grapheme // --> We should accumulate input text as much as possible and then call _processInput with wstring_view. - const auto nextGraphemeLength = TextBuffer::GraphemeNext(_buffer, _bufferCursor) - _bufferCursor; - _buffer.replace(_bufferCursor, nextGraphemeLength, 1, wch); + const auto cursor = _buffer.GetCursorPosition(); + remove = TextBuffer::GraphemeNext(_buffer.Get(), cursor) - cursor; } - _bufferCursor++; - _markAsDirty(); + _buffer.Replace(_buffer.GetCursorPosition(), remove, &wch, 1); } // Handles non-character input for _readCharInputLoop() when no popups exist. @@ -445,68 +495,62 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) switch (vkey) { case VK_ESCAPE: - if (!_buffer.empty()) + if (!_buffer.Get().empty()) { - _buffer.clear(); - _bufferCursor = 0; - _markAsDirty(); + _buffer.Replace(0, npos, nullptr, 0); } break; case VK_HOME: - if (_bufferCursor > 0) + if (_buffer.GetCursorPosition() > 0) { if (ctrlPressed) { - _buffer.erase(0, _bufferCursor); + _buffer.Replace(0, _buffer.GetCursorPosition(), nullptr, 0); } - _bufferCursor = 0; - _markAsDirty(); + _buffer.SetCursorPosition(0); } break; case VK_END: - if (_bufferCursor < _buffer.size()) + if (_buffer.GetCursorPosition() < _buffer.Get().size()) { if (ctrlPressed) { - _buffer.erase(_bufferCursor); + _buffer.Replace(_buffer.GetCursorPosition(), npos, nullptr, 0); } - _bufferCursor = _buffer.size(); - _markAsDirty(); + _buffer.SetCursorPosition(npos); } break; case VK_LEFT: - if (_bufferCursor != 0) + if (_buffer.GetCursorPosition() != 0) { if (ctrlPressed) { - _bufferCursor = _wordPrev(_buffer, _bufferCursor); + _buffer.SetCursorPosition(_wordPrev(_buffer.Get(), _buffer.GetCursorPosition())); } else { - _bufferCursor = TextBuffer::GraphemePrev(_buffer, _bufferCursor); + _buffer.SetCursorPosition(TextBuffer::GraphemePrev(_buffer.Get(), _buffer.GetCursorPosition())); } - _markAsDirty(); } break; case VK_F1: case VK_RIGHT: - if (_bufferCursor != _buffer.size()) + if (_buffer.GetCursorPosition() != _buffer.Get().size()) { if (ctrlPressed && vkey == VK_RIGHT) { - _bufferCursor = _wordNext(_buffer, _bufferCursor); + _buffer.SetCursorPosition(_wordNext(_buffer.Get(), _buffer.GetCursorPosition())); } else { - _bufferCursor = TextBuffer::GraphemeNext(_buffer, _bufferCursor); + _buffer.SetCursorPosition(TextBuffer::GraphemeNext(_buffer.Get(), _buffer.GetCursorPosition())); } - _markAsDirty(); } else if (_history) { // Traditionally pressing right at the end of an input line would paste characters from the previous command. const auto cmd = _history->GetLastCommand(); - const auto bufferSize = _buffer.size(); + const auto bufferSize = _buffer.Get().size(); const auto cmdSize = cmd.size(); size_t bufferBeg = 0; size_t cmdBeg = 0; @@ -519,13 +563,11 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) if (bufferBeg >= bufferSize) { - _buffer.append(cmd, cmdBeg, cmdEnd - cmdBeg); - _bufferCursor = _buffer.size(); - _markAsDirty(); + _buffer.Replace(npos, 0, cmd.data() + cmdBeg, cmdEnd - cmdBeg); break; } - bufferBeg = TextBuffer::GraphemeNext(_buffer, bufferBeg); + bufferBeg = TextBuffer::GraphemeNext(_buffer.Get(), bufferBeg); cmdBeg = cmdEnd; } } @@ -533,38 +575,38 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) case VK_INSERT: _insertMode = !_insertMode; _screenInfo.SetCursorDBMode(_insertMode != ServiceLocator::LocateGlobals().getConsoleInformation().GetInsertMode()); - _markAsDirty(); break; case VK_DELETE: - if (_bufferCursor < _buffer.size()) + if (_buffer.GetCursorPosition() < _buffer.Get().size()) { - _buffer.erase(_bufferCursor, TextBuffer::GraphemeNext(_buffer, _bufferCursor) - _bufferCursor); - _markAsDirty(); + const auto beg = _buffer.GetCursorPosition(); + const auto end = TextBuffer::GraphemeNext(_buffer.Get(), beg); + _buffer.Replace(beg, end - beg, nullptr, 0); } break; case VK_UP: case VK_F5: if (_history && !_history->AtFirstCommand()) { - _replaceBuffer(_history->Retrieve(CommandHistory::SearchDirection::Previous)); + _buffer.Replace(_history->Retrieve(CommandHistory::SearchDirection::Previous)); } break; case VK_DOWN: if (_history && !_history->AtLastCommand()) { - _replaceBuffer(_history->Retrieve(CommandHistory::SearchDirection::Next)); + _buffer.Replace(_history->Retrieve(CommandHistory::SearchDirection::Next)); } break; case VK_PRIOR: if (_history && !_history->AtFirstCommand()) { - _replaceBuffer(_history->RetrieveNth(0)); + _buffer.Replace(_history->RetrieveNth(0)); } break; case VK_NEXT: if (_history && !_history->AtLastCommand()) { - _replaceBuffer(_history->RetrieveNth(INT_MAX)); + _buffer.Replace(_history->RetrieveNth(INT_MAX)); } break; case VK_F2: @@ -577,12 +619,10 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) if (_history) { const auto last = _history->GetLastCommand(); - if (last.size() > _bufferCursor) + if (last.size() > _buffer.GetCursorPosition()) { - const auto count = last.size() - _bufferCursor; - _buffer.replace(_bufferCursor, count, last, _bufferCursor, count); - _bufferCursor += count; - _markAsDirty(); + const auto count = last.size() - _buffer.GetCursorPosition(); + _buffer.Replace(_buffer.GetCursorPosition(), npos, last.data() + _buffer.GetCursorPosition(), count); } } break; @@ -616,12 +656,12 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) if (_history) { CommandHistory::Index index = 0; - const auto prefix = std::wstring_view{ _buffer }.substr(0, _bufferCursor); + const auto cursorPos = _buffer.GetCursorPosition(); + const auto prefix = std::wstring_view{ _buffer.Get() }.substr(0, cursorPos); if (_history->FindMatchingCommand(prefix, _history->LastDisplayed, index, CommandHistory::MatchOptions::None)) { - _buffer.assign(_history->RetrieveNth(index)); - _bufferCursor = std::min(_bufferCursor, _buffer.size()); - _markAsDirty(); + _buffer.Replace(_history->RetrieveNth(index)); + _buffer.SetCursorPosition(cursorPos); } } break; @@ -649,7 +689,8 @@ void COOKED_READ_DATA::_handleVkey(uint16_t vkey, DWORD modifiers) void COOKED_READ_DATA::_handlePostCharInputLoop(const bool isUnicode, size_t& numBytes, ULONG& controlKeyState) { auto writer = _userBuffer; - std::wstring_view input{ _buffer }; + auto buffer = _buffer.Extract(); + std::wstring_view input{ buffer }; size_t lineCount = 1; if (_state == State::DoneWithCarriageReturn) @@ -676,7 +717,7 @@ void COOKED_READ_DATA::_handlePostCharInputLoop(const bool isUnicode, size_t& nu // It'll print "hello" but the previous prompt will say "echo hello bar" because the _distanceEnd // ended up being well over 14 leading it to believe that "bar" got overwritten during WriteCharsLegacy(). - WriteCharsLegacy(_screenInfo, newlineSuffix, true, nullptr); + WriteCharsLegacy(_screenInfo, newlineSuffix, nullptr); if (WI_IsFlagSet(_pInputBuffer->InputMode, ENABLE_ECHO_INPUT)) { @@ -692,14 +733,14 @@ void COOKED_READ_DATA::_handlePostCharInputLoop(const bool isUnicode, size_t& nu if (!alias.empty()) { - _buffer = std::move(alias); + buffer = std::move(alias); } else { - _buffer.append(newlineSuffix); + buffer.append(newlineSuffix); } - input = std::wstring_view{ _buffer }; + input = std::wstring_view{ buffer }; // doskey aliases may result in multiple lines of output (for instance `doskey test=echo foo$Techo bar$Techo baz`). // We need to emit them as multiple cooked reads as well, so that each read completes at a \r\n. @@ -720,7 +761,7 @@ void COOKED_READ_DATA::_handlePostCharInputLoop(const bool isUnicode, size_t& nu // We've truncated the `input` slice and now we need to restore it. const auto inputSizeAfter = input.size(); const auto amountConsumed = inputSizeBefore - inputSizeAfter; - input = std::wstring_view{ _buffer }; + input = std::wstring_view{ buffer }; input = input.substr(std::min(input.size(), amountConsumed)); GetInputReadHandleData()->SaveMultilinePendingInput(input); } @@ -746,48 +787,70 @@ void COOKED_READ_DATA::_transitionState(State state) noexcept _state = state; } -// Signals to _flushBuffer() that the contents of _buffer are stale and need to be redrawn. -// ALL _buffer and _bufferCursor changes must be flagged with _markAsDirty(). -// -// By using _bufferDirty to avoid redrawing the buffer unless needed, we turn the amortized time complexity of _readCharInputLoop() -// from O(n^2) (n(n+1)/2 redraws) into O(n). Pasting text would quickly turn into "accidentally quadratic" meme material otherwise. -void COOKED_READ_DATA::_markAsDirty() -{ - _bufferDirty = true; -} - // Draws the contents of _buffer onto the screen. +// +// By using _buffer._dirtyBeg to avoid redrawing the buffer unless needed, we turn the amortized +// time complexity of _readCharInputLoop() from O(n^2) (n(n+1)/2 redraws) into O(n). +// Pasting text would quickly turn into "accidentally quadratic" meme material otherwise. +// // NOTE: Don't call _flushBuffer() after appending newlines to the buffer! See _handlePostCharInputLoop for more information. void COOKED_READ_DATA::_flushBuffer() { - // _flushBuffer() is called often and is a good place to assert() that our _bufferCursor is still in bounds. - assert(_bufferCursor <= _buffer.size()); - _bufferCursor = std::min(_bufferCursor, _buffer.size()); - - if (!_bufferDirty) + if (_buffer.IsClean() || WI_IsFlagClear(_pInputBuffer->InputMode, ENABLE_ECHO_INPUT)) { return; } - if (WI_IsFlagSet(_pInputBuffer->InputMode, ENABLE_ECHO_INPUT)) + // `_buffer` is split up by two different indices: + // * `_buffer._cursor`: Text before the `_buffer._cursor` index must be accumulated + // into `distanceBeforeCursor` and the other half into `distanceAfterCursor`. + // This helps us figure out where the cursor is positioned on the screen. + // * `_buffer._dirtyBeg`: Text before `_buffer._dirtyBeg` must be written with SuppressMSAA + // and the other half without. Any text before `_buffer._dirtyBeg` is considered unchanged, + // and this split prevents us from announcing text that hasn't actually changed + // to accessibility tools via MSAA (or UIA, but UIA is robust against this anyways). + // + // This results in 2*2 = 4 writes of which always at least one of the middle two is empty, + // depending on whether _buffer._cursor > _buffer._dirtyBeg or _buffer._cursor < _buffer._dirtyBeg. + // slice() returns an empty string-view when `from` index is greater than the `to` index. + + ptrdiff_t distanceBeforeCursor = 0; + ptrdiff_t distanceAfterCursor = 0; { - _unwindCursorPosition(_distanceCursor); + // _distanceCursor might be larger than the entire viewport (= a really long input line). + // _offsetCursorPosition() with such an offset will end up clamping the cursor position to (0,0). + // To make this implementation behave a little bit more consistent in this case without + // writing a more thorough and complex readline implementation, we pass _measureChars() + // the relative "distance" to the current actual cursor position. That way _measureChars() + // can still figure out what the logical cursor position is, when it handles tabs, etc. + auto dirtyBegDistance = -_distanceCursor; + + distanceBeforeCursor = _measureChars(_buffer.GetUnmodifiedTextBeforeCursor(), dirtyBegDistance); + dirtyBegDistance += distanceBeforeCursor; + distanceAfterCursor = _measureChars(_buffer.GetUnmodifiedTextAfterCursor(), dirtyBegDistance); + dirtyBegDistance += distanceAfterCursor; + + _offsetCursorPosition(dirtyBegDistance); + } - const std::wstring_view view{ _buffer }; - const auto distanceBeforeCursor = _writeChars(view.substr(0, _bufferCursor)); - const auto distanceAfterCursor = _writeChars(view.substr(_bufferCursor)); - const auto distanceEnd = distanceBeforeCursor + distanceAfterCursor; - const auto eraseDistance = std::max(0, _distanceEnd - distanceEnd); + // Now we can finally write the parts of _buffer that have actually changed (or moved). + distanceBeforeCursor += _writeChars(_buffer.GetModifiedTextBeforeCursor()); + distanceAfterCursor += _writeChars(_buffer.GetModifiedTextAfterCursor()); - // If the contents of _buffer became shorter we'll have to erase the previously printed contents. - _erase(eraseDistance); - _unwindCursorPosition(distanceAfterCursor + eraseDistance); + const auto distanceEnd = distanceBeforeCursor + distanceAfterCursor; + const auto eraseDistance = std::max(0, _distanceEnd - distanceEnd); - _distanceCursor = distanceBeforeCursor; - _distanceEnd = distanceEnd; - } + // If the contents of _buffer became shorter we'll have to erase the previously printed contents. + _erase(eraseDistance); + _offsetCursorPosition(-eraseDistance - distanceAfterCursor); + + _buffer.MarkAsClean(); + _distanceCursor = distanceBeforeCursor; + _distanceEnd = distanceEnd; - _bufferDirty = false; + const auto pos = _screenInfo.GetTextBuffer().GetCursor().GetPosition(); + _screenInfo.MakeCursorVisible(pos); + std::ignore = _screenInfo.SetCursorPosition(pos, true); } // This is just a small helper to fill the next N cells starting at the current cursor position with whitespace. @@ -815,10 +878,118 @@ void COOKED_READ_DATA::_erase(ptrdiff_t distance) const } while (remaining != 0); } +// A helper to calculate the number of cells `text` would take up if it were written. +// `cursorOffset` allows the caller to specify a "logical" cursor position relative to the actual cursor position. +// This allows the function to track in which column it currently is, which is needed to implement tabs for instance. +ptrdiff_t COOKED_READ_DATA::_measureChars(const std::wstring_view& text, ptrdiff_t cursorOffset) const +{ + if (text.empty()) + { + return 0; + } + return _writeCharsImpl(text, true, cursorOffset); +} + // A helper to write text and calculate the number of cells we've written. // _unwindCursorPosition then allows us to go that many cells back. Tracking cells instead of explicit // buffer positions allows us to pay no further mind to whether the buffer scrolled up or not. ptrdiff_t COOKED_READ_DATA::_writeChars(const std::wstring_view& text) const +{ + if (text.empty()) + { + return 0; + } + return _writeCharsImpl(text, false, 0); +} + +ptrdiff_t COOKED_READ_DATA::_writeCharsImpl(const std::wstring_view& text, const bool measureOnly, const ptrdiff_t cursorOffset) const +{ + const auto width = _screenInfo.GetTextBuffer().GetSize().Width(); + auto it = text.begin(); + const auto end = text.end(); + ptrdiff_t distance = 0; + + for (;;) + { + const auto nextControlChar = std::find_if(it, end, [](const auto& wch) { return wch < L' '; }); + if (nextControlChar != it) + { + if (measureOnly) + { + distance += _measureCharsUnprocessed({ it, nextControlChar }, distance + cursorOffset); + } + else + { + distance += _writeCharsUnprocessed({ it, nextControlChar }); + } + it = nextControlChar; + } + if (nextControlChar == end) + { + break; + } + + wchar_t buf[2]; + size_t len = 0; + + const auto wch = *it; + if (wch == UNICODE_TAB) + { + const auto col = _getColumnAtRelativeCursorPosition(distance + cursorOffset); + const auto remaining = width - col; + distance += std::min(remaining, 8 - (col & 7)); + buf[0] = L'\t'; + len = 1; + } + else + { + // In the interactive mode we replace C0 control characters (0x00-0x1f) with ASCII representations like ^C (= 0x03). + distance += 2; + buf[0] = L'^'; + buf[1] = gsl::narrow_cast(wch + L'@'); + len = 2; + } + + if (!measureOnly) + { + distance += _writeCharsUnprocessed({ &buf[0], len }); + } + + ++it; + } + + return distance; +} + +ptrdiff_t COOKED_READ_DATA::_measureCharsUnprocessed(const std::wstring_view& text, ptrdiff_t cursorOffset) const +{ + if (text.empty()) + { + return 0; + } + + auto& textBuffer = _screenInfo.GetTextBuffer(); + const auto width = textBuffer.GetSize().Width(); + auto columnLimit = width - _getColumnAtRelativeCursorPosition(cursorOffset); + + size_t offset = 0; + ptrdiff_t distance = 0; + + while (offset < text.size()) + { + til::CoordType columns = 0; + offset += textBuffer.FitTextIntoColumns(text.substr(offset), columnLimit, columns); + distance += columns; + columnLimit = width; + } + + return distance; +} + +// A helper to write text and calculate the number of cells we've written. +// _unwindCursorPosition then allows us to go that many cells back. Tracking cells instead of explicit +// buffer positions allows us to pay no further mind to whether the buffer scrolled up or not. +ptrdiff_t COOKED_READ_DATA::_writeCharsUnprocessed(const std::wstring_view& text) const { if (text.empty()) { @@ -831,7 +1002,7 @@ ptrdiff_t COOKED_READ_DATA::_writeChars(const std::wstring_view& text) const const auto initialCursorPos = cursor.GetPosition(); til::CoordType scrollY = 0; - WriteCharsLegacy(_screenInfo, text, true, &scrollY); + WriteCharsLegacy(_screenInfo, text, &scrollY); const auto finalCursorPos = cursor.GetPosition(); const auto distance = (finalCursorPos.y - initialCursorPos.y + scrollY) * width + finalCursorPos.x - initialCursorPos.x; @@ -842,6 +1013,11 @@ ptrdiff_t COOKED_READ_DATA::_writeChars(const std::wstring_view& text) const // Moves the given point by the given distance inside the text buffer, as if moving a cursor with the left/right arrow keys. til::point COOKED_READ_DATA::_offsetPosition(til::point pos, ptrdiff_t distance) const { + if (distance == 0) + { + return pos; + } + const auto size = _screenInfo.GetTextBuffer().GetSize().Dimensions(); const auto w = static_cast(size.width); const auto h = static_cast(size.height); @@ -859,30 +1035,35 @@ til::point COOKED_READ_DATA::_offsetPosition(til::point pos, ptrdiff_t distance) // This moves the cursor `distance`-many cells back up in the buffer. // It's intended to be used in combination with _writeChars. -void COOKED_READ_DATA::_unwindCursorPosition(ptrdiff_t distance) const +void COOKED_READ_DATA::_offsetCursorPosition(ptrdiff_t distance) const { - if (distance <= 0) + if (distance == 0) { - // If all the code in this file works correctly, negative distances should not occur. - // If they do occur it would indicate a bug that would need to be fixed urgently. - assert(distance == 0); return; } const auto& textBuffer = _screenInfo.GetTextBuffer(); const auto& cursor = textBuffer.GetCursor(); - const auto pos = _offsetPosition(cursor.GetPosition(), -distance); + const auto pos = _offsetPosition(cursor.GetPosition(), distance); std::ignore = _screenInfo.SetCursorPosition(pos, true); _screenInfo.MakeCursorVisible(pos); } -// Just a simple helper to replace the entire buffer contents. -void COOKED_READ_DATA::_replaceBuffer(const std::wstring_view& str) +til::CoordType COOKED_READ_DATA::_getColumnAtRelativeCursorPosition(ptrdiff_t distance) const { - _buffer.assign(str); - _bufferCursor = _buffer.size(); - _markAsDirty(); + const auto& textBuffer = _screenInfo.GetTextBuffer(); + const auto size = _screenInfo.GetTextBuffer().GetSize().Dimensions(); + const auto& cursor = textBuffer.GetCursor(); + const auto pos = cursor.GetPosition(); + + auto x = gsl::narrow_cast((pos.x + distance) % size.width); + if (x < 0) + { + x += size.width; + } + + return x; } // If the viewport is large enough to fit a popup, this function prepares everything for @@ -1109,17 +1290,16 @@ void COOKED_READ_DATA::_popupHandleCopyToCharInput(Popup& /*popup*/, const wchar { // See PopupKind::CopyToChar for more information about this code. const auto cmd = _history->GetLastCommand(); - const auto idx = cmd.find(wch, _bufferCursor); + const auto cursor = _buffer.GetCursorPosition(); + const auto idx = cmd.find(wch, cursor); if (idx != decltype(cmd)::npos) { - // When we enter this if condition it's guaranteed that _bufferCursor must be + // When we enter this if condition it's guaranteed that _buffer.GetCursorPosition() must be // smaller than idx, which in turn implies that it's smaller than cmd.size(). // As such, calculating length is safe and str.size() == length. - const auto count = idx - _bufferCursor; - _buffer.replace(_bufferCursor, count, cmd, _bufferCursor, count); - _bufferCursor += count; - _markAsDirty(); + const auto count = idx - cursor; + _buffer.Replace(cursor, count, cmd.data() + cursor, count); } _popupsDone(); @@ -1138,9 +1318,10 @@ void COOKED_READ_DATA::_popupHandleCopyFromCharInput(Popup& /*popup*/, const wch else { // See PopupKind::CopyFromChar for more information about this code. - const auto idx = _buffer.find(wch, _bufferCursor); - _buffer.erase(_bufferCursor, std::min(idx, _buffer.size()) - _bufferCursor); - _markAsDirty(); + const auto cursor = _buffer.GetCursorPosition(); + auto idx = _buffer.Get().find(wch, cursor); + idx = std::min(idx, _buffer.Get().size()); + _buffer.Replace(cursor, idx - cursor, nullptr, 0); _popupsDone(); } } @@ -1159,7 +1340,7 @@ void COOKED_READ_DATA::_popupHandleCommandNumberInput(Popup& popup, const wchar_ if (wch == UNICODE_CARRIAGERETURN) { popup.commandNumber.buffer[popup.commandNumber.bufferSize++] = L'\0'; - _replaceBuffer(_history->RetrieveNth(std::stoi(popup.commandNumber.buffer.data()))); + _buffer.Replace(_history->RetrieveNth(std::stoi(popup.commandNumber.buffer.data()))); _popupsDone(); return; } @@ -1198,7 +1379,7 @@ void COOKED_READ_DATA::_popupHandleCommandListInput(Popup& popup, const wchar_t if (wch == UNICODE_CARRIAGERETURN) { - _buffer.assign(_history->RetrieveNth(cl.selected)); + _buffer.Replace(_history->RetrieveNth(cl.selected)); _popupsDone(); _handleChar(UNICODE_CARRIAGERETURN, modifiers); return; @@ -1222,7 +1403,7 @@ void COOKED_READ_DATA::_popupHandleCommandListInput(Popup& popup, const wchar_t break; case VK_LEFT: case VK_RIGHT: - _replaceBuffer(_history->RetrieveNth(cl.selected)); + _buffer.Replace(_history->RetrieveNth(cl.selected)); _popupsDone(); return; case VK_UP: @@ -1261,7 +1442,6 @@ void COOKED_READ_DATA::_popupHandleCommandListInput(Popup& popup, const wchar_t } _popupDrawCommandList(popup); - return; } void COOKED_READ_DATA::_popupDrawPrompt(const Popup& popup, const UINT id) const diff --git a/src/host/readDataCooked.hpp b/src/host/readDataCooked.hpp index 803724b6f52..9502eaa5c2d 100644 --- a/src/host/readDataCooked.hpp +++ b/src/host/readDataCooked.hpp @@ -40,6 +40,7 @@ class COOKED_READ_DATA final : public ReadData private: static constexpr uint8_t CommandNumberMaxInputLength = 5; + static constexpr size_t npos = static_cast(-1); enum class State : uint8_t { @@ -48,6 +49,38 @@ class COOKED_READ_DATA final : public ReadData DoneWithCarriageReturn, }; + // A helper struct to ensure we keep track of _dirtyBeg while the + // underlying _buffer is being modified by COOKED_READ_DATA. + struct BufferState + { + const std::wstring& Get() const noexcept; + std::wstring Extract() noexcept + { + return std::move(_buffer); + } + void Replace(size_t offset, size_t remove, const wchar_t* input, size_t count); + void Replace(const std::wstring_view& str); + + size_t GetCursorPosition() const noexcept; + void SetCursorPosition(size_t pos) noexcept; + + bool IsClean() const noexcept; + void MarkEverythingDirty() noexcept; + void MarkAsClean() noexcept; + + std::wstring_view GetUnmodifiedTextBeforeCursor() const noexcept; + std::wstring_view GetUnmodifiedTextAfterCursor() const noexcept; + std::wstring_view GetModifiedTextBeforeCursor() const noexcept; + std::wstring_view GetModifiedTextAfterCursor() const noexcept; + + private: + std::wstring_view _slice(size_t from, size_t to) const noexcept; + + std::wstring _buffer; + size_t _dirtyBeg = npos; + size_t _cursor = 0; + }; + enum class PopupKind { // Copies text from the previous command between the current cursor position and the first instance @@ -118,13 +151,16 @@ class COOKED_READ_DATA final : public ReadData void _handleVkey(uint16_t vkey, DWORD modifiers); void _handlePostCharInputLoop(bool isUnicode, size_t& numBytes, ULONG& controlKeyState); void _transitionState(State state) noexcept; - void _markAsDirty(); void _flushBuffer(); void _erase(ptrdiff_t distance) const; + ptrdiff_t _measureChars(const std::wstring_view& text, ptrdiff_t cursorOffset) const; ptrdiff_t _writeChars(const std::wstring_view& text) const; + ptrdiff_t _writeCharsImpl(const std::wstring_view& text, bool measureOnly, ptrdiff_t cursorOffset) const; + ptrdiff_t _measureCharsUnprocessed(const std::wstring_view& text, ptrdiff_t cursorOffset) const; + ptrdiff_t _writeCharsUnprocessed(const std::wstring_view& text) const; til::point _offsetPosition(til::point pos, ptrdiff_t distance) const; - void _unwindCursorPosition(ptrdiff_t distance) const; - void _replaceBuffer(const std::wstring_view& str); + void _offsetCursorPosition(ptrdiff_t distance) const; + til::CoordType _getColumnAtRelativeCursorPosition(ptrdiff_t distance) const; void _popupPush(PopupKind kind); void _popupsDone(); @@ -145,15 +181,13 @@ class COOKED_READ_DATA final : public ReadData ULONG _controlKeyState = 0; std::unique_ptr _tempHandle; - std::wstring _buffer; - size_t _bufferCursor = 0; + BufferState _buffer; // _distanceCursor is the distance between the start of the prompt and the // current cursor location in columns (including wide glyph padding columns). ptrdiff_t _distanceCursor = 0; // _distanceEnd is the distance between the start of the prompt and its last // glyph at the end in columns (including wide glyph padding columns). ptrdiff_t _distanceEnd = 0; - bool _bufferDirty = false; bool _insertMode = false; State _state = State::Accumulating; diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index b7ea13e6dc6..985fcadf343 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -2927,13 +2927,13 @@ void ScreenBufferTests::BackspaceDefaultAttrsWriteCharsLegacy() if (writeSingly) { - WriteCharsLegacy(si, L"X", false, nullptr); - WriteCharsLegacy(si, L"X", false, nullptr); - WriteCharsLegacy(si, L"\x08", false, nullptr); + WriteCharsLegacy(si, L"X", nullptr); + WriteCharsLegacy(si, L"X", nullptr); + WriteCharsLegacy(si, L"\x08", nullptr); } else { - WriteCharsLegacy(si, L"XX\x08", false, nullptr); + WriteCharsLegacy(si, L"XX\x08", nullptr); } TextAttribute expectedDefaults{}; @@ -7188,7 +7188,7 @@ void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt() Log::Comment(L"Now write several lines of content using WriteCharsLegacy"); const auto content = L"1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n"; - WriteCharsLegacy(si, content, false, nullptr); + WriteCharsLegacy(si, content, nullptr); Log::Comment(L"Confirm that the cursor position has moved down 10 lines"); const auto newCursorPos = til::point{ initialCursorPos.x, initialCursorPos.y + 10 }; From d8c7719bfb4f6ab68e8047c116efb6eeac0b559d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 24 Oct 2023 13:28:59 -0500 Subject: [PATCH 12/15] Clear the system menu when we refrigerate a window (#16225) As in the title. Also fixes a crash for refrigeration with the rainbow border. Closes #16211 Tested by manually forcing us into Windows 10 mode (to refrigerate the window). That immediately repros the bug, which was simple enough to fix. --- src/cascadia/WindowsTerminal/AppHost.cpp | 2 +- src/cascadia/WindowsTerminal/IslandWindow.cpp | 8 ++++++++ src/cascadia/WindowsTerminal/IslandWindow.h | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 36d47211b79..5af3a80d50a 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -484,7 +484,7 @@ void AppHost::_revokeWindowCallbacks() // I suspect WinUI wouldn't like that very much. As such unregister all event handlers first. _revokers = {}; _showHideWindowThrottler.reset(); - + _stopFrameTimer(); _revokeWindowCallbacks(); // DO NOT CLOSE THE WINDOW diff --git a/src/cascadia/WindowsTerminal/IslandWindow.cpp b/src/cascadia/WindowsTerminal/IslandWindow.cpp index 9bea05acfe9..d7934ea3012 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp @@ -73,6 +73,8 @@ void IslandWindow::Refrigerate() noexcept // This pointer will get re-set in _warmInitialize SetWindowLongPtr(_window.get(), GWLP_USERDATA, 0); + _resetSystemMenu(); + _pfnCreateCallback = nullptr; _pfnSnapDimensionCallback = nullptr; @@ -1917,6 +1919,12 @@ void IslandWindow::RemoveFromSystemMenu(const winrt::hstring& itemLabel) _systemMenuItems.erase(it->first); } +void IslandWindow::_resetSystemMenu() +{ + // GetSystemMenu(..., true) will revert the menu to the default state. + GetSystemMenu(_window.get(), TRUE); +} + void IslandWindow::UseDarkTheme(const bool v) { const BOOL attribute = v ? TRUE : FALSE; diff --git a/src/cascadia/WindowsTerminal/IslandWindow.h b/src/cascadia/WindowsTerminal/IslandWindow.h index eee013c529a..b0aaf0b0ae3 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.h +++ b/src/cascadia/WindowsTerminal/IslandWindow.h @@ -159,6 +159,7 @@ class IslandWindow : std::unordered_map _systemMenuItems; UINT _systemMenuNextItemId; + void _resetSystemMenu(); private: // This minimum width allows for width the tabs fit From d0d3039963cedc9f3de38c3772a672e82955fa86 Mon Sep 17 00:00:00 2001 From: js324 Date: Tue, 24 Oct 2023 17:46:56 -0400 Subject: [PATCH 13/15] add single quotes to WSL drag and drop (#16214) Wrap single quotes to drag and dropped paths in WSL ## References and Relevant Issues #15646 , #8109 ## Detailed Description of the Pull Request / Additional comments First time contributor, from what I understand from reading #15646 and #8109 , issue is asking for single quotes added to a drag and dropped path always, regardless of whitespace and special characters, in WSL. ## Validation Steps Performed Tested drag and drop changes in WSL and non WSL sources. Closes #15646 --- src/cascadia/TerminalControl/TermControl.cpp | 22 ++++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 4406d364c92..4c59d027a52 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2908,7 +2908,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation // However, it's likely that the control layer may need to // know about the source anyways in the future, to support // GH#3158 - if (_interactivity.ManglePathsForWsl()) + const auto isWSL = _interactivity.ManglePathsForWsl(); + + if (isWSL) { std::replace(fullPath.begin(), fullPath.end(), L'\\', L'/'); @@ -2942,17 +2944,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation } } - const auto containsSpaces = std::find(fullPath.begin(), - fullPath.end(), - L' ') != fullPath.end(); + const auto quotesNeeded = isWSL || fullPath.find(L' ') != std::wstring::npos; + const auto quotesChar = isWSL ? L'\'' : L'"'; - if (containsSpaces) + // Append fullPath and also wrap it in quotes if needed + if (quotesNeeded) { - fullPath.insert(0, L"\""); - fullPath += L"\""; + allPathsString.push_back(quotesChar); + } + allPathsString.append(fullPath); + if (quotesNeeded) + { + allPathsString.push_back(quotesChar); } - - allPathsString += fullPath; } _pasteTextWithBroadcast(winrt::hstring{ allPathsString }); From 58e8f3c11ccdeb3194b58ad29ad5f738a4c6c684 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Oct 2023 09:37:23 -0500 Subject: [PATCH 14/15] mostly nits --- src/cascadia/TerminalApp/Pane.cpp | 11 ++++------- src/cascadia/TerminalApp/Pane.h | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 4 ++++ .../TerminalApp/TerminalPaneContent.cpp | 18 +++++++++--------- src/cascadia/TerminalApp/TerminalPaneContent.h | 10 +++++----- src/cascadia/TerminalApp/TerminalTab.cpp | 6 +++--- 6 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 237c71dafb2..22927afdd38 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1351,7 +1351,7 @@ std::shared_ptr Pane::DetachPane(std::shared_ptr pane) auto detached = isFirstChild ? _firstChild : _secondChild; // Remove the child from the tree, replace the current node with the // other child. - _CloseChild(isFirstChild, true); + _CloseChild(isFirstChild); // Update the borders on this pane and any children to match if we have // no parent. @@ -1380,12 +1380,9 @@ std::shared_ptr Pane::DetachPane(std::shared_ptr pane) // Arguments: // - closeFirst: if true, the first child should be closed, and the second // should be preserved, and vice-versa for false. -// - isDetaching: if true, then the pane event handlers for the closed child -// should be kept, this way they don't have to be recreated when it is later -// reattached to a tree somewhere as the control moves with the pane. // Return Value: // - -void Pane::_CloseChild(const bool closeFirst, const bool /*isDetaching*/) +void Pane::_CloseChild(const bool closeFirst) { // If we're a leaf, then chances are both our children closed in close // succession. We waited on the lock while the other child was closed, so @@ -1601,7 +1598,7 @@ void Pane::_CloseChildRoutine(const bool closeFirst) // this one doesn't seem to. if (!animationsEnabledInOS || !animationsEnabledInApp || eitherChildZoomed) { - _CloseChild(closeFirst, false); + _CloseChild(closeFirst); return; } @@ -1704,7 +1701,7 @@ void Pane::_CloseChildRoutine(const bool closeFirst) { // We don't need to manually undo any of the above trickiness. // We're going to re-parent the child's content into us anyways - pane->_CloseChild(closeFirst, false); + pane->_CloseChild(closeFirst); } }); } diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index de9aad13616..ec4754c1373 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -295,7 +295,7 @@ class Pane : public std::enable_shared_from_this const winrt::Microsoft::Terminal::Settings::Model::FocusDirection& direction, const PanePoint offset); - void _CloseChild(const bool closeFirst, const bool isDetaching); + void _CloseChild(const bool closeFirst); void _CloseChildRoutine(const bool closeFirst); void _Focus(); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d2c129f072d..44fac10722e 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -3192,6 +3192,10 @@ namespace winrt::TerminalApp::implementation const TerminalApp::TerminalPaneContent& paneContent, const winrt::Windows::Foundation::IInspectable&) { + // Note: callers are likely passing in `nullptr` as the args here, as + // the TermControl.RestartTerminalRequested event doesn't actually pass + // any args upwards itself. If we ever change this, make sure you check + // for nulls if (const auto& connection{ _duplicateConnectionForRestart(paneContent) }) { paneContent.GetTerminal().Connection(connection); diff --git a/src/cascadia/TerminalApp/TerminalPaneContent.cpp b/src/cascadia/TerminalApp/TerminalPaneContent.cpp index 3863a377824..9d97788c6e7 100644 --- a/src/cascadia/TerminalApp/TerminalPaneContent.cpp +++ b/src/cascadia/TerminalApp/TerminalPaneContent.cpp @@ -25,10 +25,10 @@ namespace winrt::TerminalApp::implementation void TerminalPaneContent::_setupControlEvents() { - _controlEvents._ConnectionStateChanged = _control.ConnectionStateChanged(winrt::auto_revoke, { this, &TerminalPaneContent::_ControlConnectionStateChangedHandler }); - _controlEvents._WarningBell = _control.WarningBell(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_ControlWarningBellHandler }); - _controlEvents._CloseTerminalRequested = _control.CloseTerminalRequested(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_CloseTerminalRequestedHandler }); - _controlEvents._RestartTerminalRequested = _control.RestartTerminalRequested(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_RestartTerminalRequestedHandler }); + _controlEvents._ConnectionStateChanged = _control.ConnectionStateChanged(winrt::auto_revoke, { this, &TerminalPaneContent::_controlConnectionStateChangedHandler }); + _controlEvents._WarningBell = _control.WarningBell(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_controlWarningBellHandler }); + _controlEvents._CloseTerminalRequested = _control.CloseTerminalRequested(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_closeTerminalRequestedHandler }); + _controlEvents._RestartTerminalRequested = _control.RestartTerminalRequested(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_restartTerminalRequestedHandler }); _controlEvents._TitleChanged = _control.TitleChanged(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_controlTitleChanged }); _controlEvents._TabColorChanged = _control.TabColorChanged(winrt::auto_revoke, { get_weak(), &TerminalPaneContent::_controlTabColorChanged }); @@ -79,7 +79,7 @@ namespace winrt::TerminalApp::implementation NewTerminalArgs TerminalPaneContent::GetNewTerminalArgs(const bool asContent) const { NewTerminalArgs args{}; - auto controlSettings = _control.Settings(); + const auto& controlSettings = _control.Settings(); args.Profile(controlSettings.ProfileName()); // If we know the user's working directory use it instead of the profile. @@ -160,7 +160,7 @@ namespace winrt::TerminalApp::implementation // - // Return Value: // - - winrt::fire_and_forget TerminalPaneContent::_ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, + winrt::fire_and_forget TerminalPaneContent::_controlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args) { ConnectionStateChanged.raise(sender, args); @@ -228,7 +228,7 @@ namespace winrt::TerminalApp::implementation // has the 'visual' flag set // Arguments: // - - void TerminalPaneContent::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, + void TerminalPaneContent::_controlWarningBellHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*eventArgs*/) { if (_profile) @@ -292,13 +292,13 @@ namespace winrt::TerminalApp::implementation } } } - void TerminalPaneContent::_CloseTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, + void TerminalPaneContent::_closeTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/) { Close(); } - void TerminalPaneContent::_RestartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, + void TerminalPaneContent::_restartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/) { RestartTerminalRequested.raise(*this, nullptr); diff --git a/src/cascadia/TerminalApp/TerminalPaneContent.h b/src/cascadia/TerminalApp/TerminalPaneContent.h index 953b3d8879d..2d26e5b1924 100644 --- a/src/cascadia/TerminalApp/TerminalPaneContent.h +++ b/src/cascadia/TerminalApp/TerminalPaneContent.h @@ -77,10 +77,10 @@ namespace winrt::TerminalApp::implementation winrt::fire_and_forget _playBellSound(winrt::Windows::Foundation::Uri uri); - winrt::fire_and_forget _ControlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); - void _ControlWarningBellHandler(const winrt::Windows::Foundation::IInspectable& sender, + winrt::fire_and_forget _controlConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); + void _controlWarningBellHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& e); - void _ControlReadOnlyChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& e); + void _controlReadOnlyChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& e); void _controlTitleChanged(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args); void _controlTabColorChanged(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args); @@ -88,7 +88,7 @@ namespace winrt::TerminalApp::implementation void _controlReadOnlyChanged(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args); void _controlFocusFollowMouseRequested(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args); - void _CloseTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); - void _RestartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); + void _closeTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); + void _restartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& /*args*/); }; } diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index 843e8672f75..0adcca4eb6e 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -909,9 +909,9 @@ namespace winrt::TerminalApp::implementation auto it = _contentEvents.find(paneId); if (it != _contentEvents.end()) { - auto& events = it->second; - events = {}; - + // revoke the event handlers by resetting the event struct + it->second = {}; + // and remove it from the map _contentEvents.erase(paneId); } } From 7bc1457d4247c559110fcebd35c116f27bc50975 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Oct 2023 11:03:41 -0500 Subject: [PATCH 15/15] nits and such --- src/cascadia/TerminalApp/AppActionHandlers.cpp | 4 ++-- src/cascadia/TerminalApp/ScratchpadContent.cpp | 3 +-- src/features.xml | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index d5202a79dbd..39cadb146d1 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -1426,14 +1426,14 @@ namespace winrt::TerminalApp::implementation { if (Feature_ScratchpadPane::IsEnabled()) { - auto scratchPane{ winrt::make_self() }; + const auto& scratchPane{ winrt::make_self() }; // This is maybe a little wacky - add our key event handler to the pane // we made. So that we can get actions for keys that the content didn't // handle. scratchPane->GetRoot().KeyDown({ this, &TerminalPage::_KeyDownHandler }); - auto resultPane = std::make_shared(*scratchPane); + const auto resultPane = std::make_shared(*scratchPane); _SplitPane(_GetFocusedTabImpl(), SplitDirection::Automatic, 0.5f, resultPane); args.Handled(true); } diff --git a/src/cascadia/TerminalApp/ScratchpadContent.cpp b/src/cascadia/TerminalApp/ScratchpadContent.cpp index 6dc690448e8..bb821f98c7d 100644 --- a/src/cascadia/TerminalApp/ScratchpadContent.cpp +++ b/src/cascadia/TerminalApp/ScratchpadContent.cpp @@ -15,8 +15,7 @@ namespace winrt::TerminalApp::implementation ScratchpadContent::ScratchpadContent() { _root = winrt::Windows::UI::Xaml::Controls::Grid{}; - _root.VerticalAlignment(VerticalAlignment::Stretch); - _root.HorizontalAlignment(HorizontalAlignment::Stretch); + // Vertical and HorizontalAlignment are Stretch by default auto res = Windows::UI::Xaml::Application::Current().Resources(); auto bg = res.Lookup(winrt::box_value(L"UnfocusedBorderBrush")); diff --git a/src/features.xml b/src/features.xml index af144d202cd..47b00ee5ede 100644 --- a/src/features.xml +++ b/src/features.xml @@ -194,6 +194,7 @@ AlwaysDisabled Dev + Canary