From 642a2aa41e089ad84210fbe44d89c65edac9a8a8 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 20 Aug 2025 10:43:26 -0700 Subject: [PATCH 1/2] [Conhost] Fix off-by-1 error when copying and coloring selections (#19259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fixes a bug where copying and coloring selected text would be off by one. This was introduced in #18106 when selection was updated to be stored as an exclusive range. `Selection::_RegenerateSelectionSpans()` was updated then, but copying text and coloring selection didn't rely on selection spans. Copying text relies on `GetSelectionAnchors()`. This function has now been updated to increment the bottom-right point of the selection. This way, `GetTextSpans()` operates on the expected _exclusive_ range. Coloring selection relies on `TextBuffer::SearchText()`, `TextBuffer::GetTextRects` and `GetSelectionSpans()`. Both `Selection::ColorSelection()` were updated to use `rect` over `inclusive_rect` to emphasize that they are exclusive ranges. Converting between the two improves clarity and fixes the bug. ## References and Relevant Issues Introduced in #18106 ## Validation Steps Performed Copying text works in the following scenarios: ✅ single line, left-to-right and right-to-left ✅ multi-line, diagonal directions ✅ block selection Coloring text works in the following scenarios: ✅ctrl+# --> color instance ✅ctrl+shift+# --> color all instances Closes #19053 --- src/host/selection.cpp | 12 ++++++------ src/host/selection.hpp | 2 +- src/host/selectionInput.cpp | 2 +- src/host/selectionState.cpp | 17 +++++++++++++++++ 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/host/selection.cpp b/src/host/selection.cpp index 5bc077e9ab..fd815e082a 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -428,9 +428,9 @@ void Selection::ClearSelection(const bool fStartingNewSelection) // - This does not validate whether there is a valid selection right now or not. // It is assumed to already be in a proper selecting state and the given rectangle should be highlighted with the given color unconditionally. // Arguments: -// - psrRect - Rectangular area to fill with color +// - psrRect - Rectangular area to fill with color (exclusive) // - attr - The color attributes to apply -void Selection::ColorSelection(const til::inclusive_rect& srRect, const TextAttribute attr) +void Selection::ColorSelection(const til::rect& srRect, const TextAttribute attr) { auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); @@ -438,8 +438,8 @@ void Selection::ColorSelection(const til::inclusive_rect& srRect, const TextAttr auto& screenInfo = gci.GetActiveOutputBuffer(); til::point coordTargetSize; - coordTargetSize.x = CalcWindowSizeX(srRect); - coordTargetSize.y = CalcWindowSizeY(srRect); + coordTargetSize.x = srRect.width(); + coordTargetSize.y = srRect.height(); til::point coordTarget; coordTarget.x = srRect.left; @@ -475,9 +475,9 @@ void Selection::ColorSelection(const til::point coordSelectionStart, const til:: const auto& screenInfo = gci.GetActiveOutputBuffer(); const auto rectangles = screenInfo.GetTextBuffer().GetTextRects(coordSelectionStart, coordSelectionEnd, false, true); - for (const auto& rect : rectangles) + for (const auto& inclusiveRect : rectangles) { - ColorSelection(rect, attr); + ColorSelection(til::rect{ inclusiveRect }, attr); } } CATCH_LOG(); diff --git a/src/host/selection.hpp b/src/host/selection.hpp index 8be598651a..7ff876929f 100644 --- a/src/host/selection.hpp +++ b/src/host/selection.hpp @@ -58,7 +58,7 @@ public: void ClearSelection(); void ClearSelection(const bool fStartingNewSelection); - void ColorSelection(const til::inclusive_rect& srRect, const TextAttribute attr); + void ColorSelection(const til::rect& srRect, const TextAttribute attr); void ColorSelection(const til::point coordSelectionStart, const til::point coordSelectionEnd, const TextAttribute attr); // delete these or we can accidentally get copies of the singleton diff --git a/src/host/selectionInput.cpp b/src/host/selectionInput.cpp index 9a118ba08c..d7f08d28bf 100644 --- a/src/host/selectionInput.cpp +++ b/src/host/selectionInput.cpp @@ -698,7 +698,7 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo) for (auto&& sp : selection) { sp.iterate_rows(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) { - ColorSelection({ beg, row, end, row }, selectionAttr); + ColorSelection({ beg, row, end, row + 1 }, selectionAttr); }); } ClearSelection(); diff --git a/src/host/selectionState.cpp b/src/host/selectionState.cpp index 2504800bd9..c74e21dddd 100644 --- a/src/host/selectionState.cpp +++ b/src/host/selectionState.cpp @@ -220,6 +220,23 @@ std::pair Selection::GetSelectionAnchors() const noexcep endSelectionAnchor.x = (_d->coordSelectionAnchor.x == _d->srSelectionRect.left) ? _d->srSelectionRect.right : _d->srSelectionRect.left; endSelectionAnchor.y = (_d->coordSelectionAnchor.y == _d->srSelectionRect.top) ? _d->srSelectionRect.bottom : _d->srSelectionRect.top; + // GH #18106: Conhost and Terminal share most of the selection code. + // Both now store the selection data as a half-open range [start, end), + // where "end" is the bottom-right-most point. + // Conhost operates as an inclusive range, so we need to adjust the "end" endpoint by incrementing it by one. + const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + const auto& bufferSize = gci.GetActiveOutputBuffer().GetTextBuffer().GetSize(); + if (IsLineSelection()) + { + // General comparison for line selection. + bufferSize.IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); + } + else + { + // Compare x-values when we're in block selection! + bufferSize.IncrementInExclusiveBounds(startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor); + } + if (startSelectionAnchor > endSelectionAnchor) { return { endSelectionAnchor, startSelectionAnchor }; From 68b723c16c12a7ef2c34f91d47869c0b1611dd39 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 20 Aug 2025 17:03:47 -0500 Subject: [PATCH 2/2] Remove ProfileName from the surface of CoreSettings (#19261) You know how much I hate squirreling away information on objects we have to pass halfway across the universe just to get back. In this case, `StartingTitle` will always be the name of the profile. We only used ProfileName in places where we _needed a Title_, so this makes it much more obvious what we're doing. --- src/cascadia/TerminalApp/Pane.cpp | 8 +++++--- src/cascadia/TerminalApp/Tab.cpp | 4 ++-- src/cascadia/TerminalControl/IControlSettings.idl | 2 -- src/cascadia/TerminalControl/TermControl.cpp | 4 ++-- src/cascadia/TerminalControl/TermControl.h | 2 +- .../TerminalControl/TermControlAutomationPeer.cpp | 6 +++--- src/cascadia/TerminalSettingsModel/TerminalSettings.cpp | 3 --- src/cascadia/TerminalSettingsModel/TerminalSettings.h | 2 -- src/cascadia/inc/ControlProperties.h | 1 - 9 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 1afe0d118a..ed02e5250e 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1161,10 +1161,12 @@ void Pane::SetActive() // focused, else the profile of the last control to be focused Profile Pane::GetFocusedProfile() { - auto lastFocused = GetActivePane(); - if (const auto& terminalPane{ lastFocused->_getTerminalContent() }) + if (auto lastFocused{ GetActivePane() }) { - return terminalPane.GetProfile(); + if (const auto& terminalPane{ lastFocused->_getTerminalContent() }) + { + return terminalPane.GetProfile(); + } } return nullptr; } diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index bd86dcc009..64abdf5c00 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -2127,9 +2127,9 @@ namespace winrt::TerminalApp::implementation // - The value to populate in the title run of the tool tip winrt::hstring Tab::_CreateToolTipTitle() { - if (const auto& control{ GetActiveTerminalControl() }) + if (const auto profile{ GetFocusedProfile() }) { - const auto profileName{ control.Settings().ProfileName() }; + const auto profileName{ profile.Name() }; if (profileName != Title()) { return winrt::hstring{ fmt::format(FMT_COMPILE(L"{}: {}"), profileName, Title()) }; diff --git a/src/cascadia/TerminalControl/IControlSettings.idl b/src/cascadia/TerminalControl/IControlSettings.idl index c273574ed7..19355a84a5 100644 --- a/src/cascadia/TerminalControl/IControlSettings.idl +++ b/src/cascadia/TerminalControl/IControlSettings.idl @@ -38,8 +38,6 @@ namespace Microsoft.Terminal.Control interface IControlSettings requires Microsoft.Terminal.Core.ICoreSettings, Microsoft.Terminal.Control.IControlAppearance { - String ProfileName; - Boolean EnableUnfocusedAcrylic { get; }; Guid SessionId { get; }; ScrollbarState ScrollState { get; }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index a2580db435..01ebc90d86 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2610,9 +2610,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation return _core.Title(); } - hstring TermControl::GetProfileName() const + hstring TermControl::GetStartingTitle() const { - return _core.Settings().ProfileName(); + return _core.Settings().StartingTitle(); } hstring TermControl::WorkingDirectory() const diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 0aaae9c70f..ccb98db51c 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -58,7 +58,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation uint64_t ContentId() const; - hstring GetProfileName() const; + hstring GetStartingTitle() const; bool CopySelectionToClipboard(bool dismissSelection, bool singleLine, bool withControlSequences, const CopyFormat formats); void PasteTextFromClipboard(); diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp index 90ad7b9876..b978efd842 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp @@ -308,14 +308,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation // fall back to title if profile name is empty if (auto control{ _termControl.get() }) { - const auto profileName = control->GetProfileName(); - if (profileName.empty()) + const auto originalName = control->GetStartingTitle(); + if (originalName.empty()) { return control->Title(); } else { - return profileName; + return originalName; } } diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp b/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp index 41b50788cd..c6e9359411 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp @@ -297,9 +297,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _AltGrAliasing = profile.AltGrAliasing(); _AnswerbackMessage = profile.AnswerbackMessage(); - // Fill in the remaining properties from the profile - _ProfileName = profile.Name(); - const auto fontInfo = profile.FontInfo(); _FontFace = fontInfo.FontFace(); _FontSize = fontInfo.FontSize(); diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.h b/src/cascadia/TerminalSettingsModel/TerminalSettings.h index a2f20bd4b2..4d6a61b116 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalSettings.h +++ b/src/cascadia/TerminalSettingsModel/TerminalSettings.h @@ -121,8 +121,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // ------------------------ End of Core Settings ----------------------- - INHERITABLE_SETTING(Model::TerminalSettings, hstring, ProfileName); - INHERITABLE_SETTING(Model::TerminalSettings, guid, SessionId); INHERITABLE_SETTING(Model::TerminalSettings, bool, EnableUnfocusedAcrylic, false); INHERITABLE_SETTING(Model::TerminalSettings, bool, UseAcrylic, false); diff --git a/src/cascadia/inc/ControlProperties.h b/src/cascadia/inc/ControlProperties.h index d4e12a94b4..b71548ddd1 100644 --- a/src/cascadia/inc/ControlProperties.h +++ b/src/cascadia/inc/ControlProperties.h @@ -60,7 +60,6 @@ // --------------------------- Control Settings --------------------------- // All of these settings are defined in IControlSettings. #define CONTROL_SETTINGS(X) \ - X(winrt::hstring, ProfileName) \ X(winrt::guid, SessionId) \ X(bool, EnableUnfocusedAcrylic, false) \ X(winrt::hstring, Padding, DEFAULT_PADDING) \