From 64b446abb0ba5796f5561583f8c3e13582858711 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Tue, 25 Feb 2020 16:28:32 -0800 Subject: [PATCH 01/13] Fix the build on VS 2019 Update 5 (#4722) This commit introduces two fixes for C5205 (delete of an abtract class without a virtual dtor) and one fix for a very hopeful VS version gating that didn't pan out. --- src/cascadia/CascadiaPackage/CascadiaPackage.wapproj | 4 +--- src/terminal/adapter/adaptDefaults.hpp | 1 + src/terminal/adapter/conGetSet.hpp | 1 + 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/CascadiaPackage/CascadiaPackage.wapproj b/src/cascadia/CascadiaPackage/CascadiaPackage.wapproj index f415febc03..e909dc7d19 100644 --- a/src/cascadia/CascadiaPackage/CascadiaPackage.wapproj +++ b/src/cascadia/CascadiaPackage/CascadiaPackage.wapproj @@ -93,9 +93,7 @@ roll up our subproject resources. We have to suppress that rule but keep part of its logic, because that rule is where the AppxPackagePayload items are created. --> - - - + $([MSBuild]::Unescape('$(WapProjBeforeGenerateAppxManifestDependsOn.Replace('_RemoveAllNonWapUWPItems', '_OpenConsoleRemoveAllNonWapUWPItems'))')) diff --git a/src/terminal/adapter/adaptDefaults.hpp b/src/terminal/adapter/adaptDefaults.hpp index 63a7b0eb86..239c16d20c 100644 --- a/src/terminal/adapter/adaptDefaults.hpp +++ b/src/terminal/adapter/adaptDefaults.hpp @@ -21,6 +21,7 @@ namespace Microsoft::Console::VirtualTerminal class AdaptDefaults { public: + virtual ~AdaptDefaults() = default; virtual void Print(const wchar_t wch) = 0; // These characters need to be mutable so that they can be processed by the TerminalInput translater. virtual void PrintString(const std::wstring_view string) = 0; diff --git a/src/terminal/adapter/conGetSet.hpp b/src/terminal/adapter/conGetSet.hpp index 0cb34f6d3c..b7e214f3df 100644 --- a/src/terminal/adapter/conGetSet.hpp +++ b/src/terminal/adapter/conGetSet.hpp @@ -27,6 +27,7 @@ namespace Microsoft::Console::VirtualTerminal class ConGetSet { public: + virtual ~ConGetSet() = default; virtual bool GetConsoleCursorInfo(CONSOLE_CURSOR_INFO& cursorInfo) const = 0; virtual bool GetConsoleScreenBufferInfoEx(CONSOLE_SCREEN_BUFFER_INFOEX& screenBufferInfo) const = 0; virtual bool SetConsoleScreenBufferInfoEx(const CONSOLE_SCREEN_BUFFER_INFOEX& screenBufferInfo) = 0; From 0efdc8f0046c2a43636375217b4b03c7f1e2253d Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Tue, 25 Feb 2020 17:10:07 -0800 Subject: [PATCH 02/13] TermControl: don't use narrow when narrow_cast will do (#4721) This will help one cause of #4623, which looks like it may be caused by floats being truncated when stuffed into LONGs for UIA purposes. --- src/cascadia/TerminalControl/TermControl.cpp | 12 ++++++------ .../TerminalControl/TermControlAutomationPeer.cpp | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index d850817143..486941f4c3 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -331,7 +331,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Initialize our font information. const auto fontFace = _settings.FontFace(); - const short fontHeight = gsl::narrow(_settings.FontSize()); + const short fontHeight = gsl::narrow_cast(_settings.FontSize()); // The font width doesn't terribly matter, we'll only be using the // height to look it up // The other params here also largely don't matter. @@ -1508,7 +1508,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation try { // Make sure we have a non-zero font size - const auto newSize = std::max(gsl::narrow(fontSize), static_cast(1)); + const auto newSize = std::max(gsl::narrow_cast(fontSize), 1); const auto fontFace = _settings.FontFace(); _actualFont = { fontFace, 0, 10, { 0, newSize }, CP_UTF8, false }; _desiredFont = { _actualFont }; @@ -1860,7 +1860,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // Initialize our font information. const auto fontFace = settings.FontFace(); - const short fontHeight = gsl::narrow(settings.FontSize()); + const short fontHeight = gsl::narrow_cast(settings.FontSize()); // The font width doesn't terribly matter, we'll only be using the // height to look it up // The other params here also largely don't matter. @@ -1894,8 +1894,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // the Terminal). At runtime, this is fine, as we'll transform // everything by our scaling, so it'll work out. However, right now we // need to get the exact pixel count. - const float fFontWidth = gsl::narrow(fontSize.X * scale); - const float fFontHeight = gsl::narrow(fontSize.Y * scale); + const float fFontWidth = gsl::narrow_cast(fontSize.X * scale); + const float fFontHeight = gsl::narrow_cast(fontSize.Y * scale); // UWP XAML scrollbars aren't guaranteed to be the same size as the // ComCtl scrollbars, but it's certainly close enough. @@ -2141,7 +2141,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void TermControl::_CurrentCursorPositionHandler(const IInspectable& /*sender*/, const CursorPositionEventArgs& eventArgs) { const COORD cursorPos = _terminal->GetCursorPosition(); - Windows::Foundation::Point p = { gsl::narrow(cursorPos.X), gsl::narrow(cursorPos.Y) }; + Windows::Foundation::Point p = { gsl::narrow_cast(cursorPos.X), gsl::narrow_cast(cursorPos.Y) }; eventArgs.CurrentPosition(p); } diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp index 5f99c9cab4..27a7b88557 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp @@ -171,10 +171,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { auto rect = GetBoundingRectangle(); return { - gsl::narrow(rect.X), - gsl::narrow(rect.Y), - gsl::narrow(rect.X + rect.Width), - gsl::narrow(rect.Y + rect.Height) + gsl::narrow_cast(rect.X), + gsl::narrow_cast(rect.Y), + gsl::narrow_cast(rect.X + rect.Width), + gsl::narrow_cast(rect.Y + rect.Height) }; } From 31c9d19a72ef5fce19ab480c0a5e407064e15941 Mon Sep 17 00:00:00 2001 From: Leon Liang <57155886+leonMSFT@users.noreply.github.com> Date: Wed, 26 Feb 2020 10:36:02 -0800 Subject: [PATCH 03/13] Display Emojis, Kaomojis, and symbols while in IME mode (#4688) ## Summary of the Pull Request Currently, while in IME mode, selections with the Emoji/Kaomoji/Symbol Picker (which is brought up with win+.) are not displayed until the user starts a new composition. This is due to the fact that we hide the TextBlock when we receive a CompositionCompleted event, and we only show the TextBlock when we receive a CompositionStarted event. Input from the picker does not count as a composition, so we were never showing the text box, even if the symbols were thrown into the inputBuffer. In addition, we weren't receiving CompositionStarted events when we expected to. We should be showing the TextBlock when we receive _any_ text, so we should make the TextBlock visible inside of `TextUpdatingHandler`. Furthermore, some really helpful discussion in #3745 around wrapping the `NotifyTextChanged` call with a `NotifyFocusLeave` and a `NotifyFocusEnter` allowed the control to much more consistently determine when a CompositionStarted and a CompositionEnded. I've also went around and replaced casts with saturating casts, and have removed the line that sets the `textBlock.Width()` so that it would automatically set its width. This resolves the issue where while composing a sentence, the textBlock would be too small to contain all the text, so it would be cut off, but the composition is still valid and still able to continue. ## PR Checklist * [x] Closes #4148 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed ## Validation Steps Performed Tested picking emojis, kaomojis, and symbols with numerous different languages. --- .../TerminalControl/TSFInputControl.cpp | 97 ++++++++++++------- .../TerminalControl/TSFInputControl.h | 3 + src/cascadia/WinRTUtils/inc/Utils.h | 10 +- 3 files changed, 70 insertions(+), 40 deletions(-) diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index 5fe2485c40..2e020f1c93 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -17,7 +17,8 @@ using namespace winrt::Windows::UI::Xaml; namespace winrt::Microsoft::Terminal::TerminalControl::implementation { TSFInputControl::TSFInputControl() : - _editContext{ nullptr } + _editContext{ nullptr }, + _inComposition{ false } { _Create(); } @@ -138,32 +139,31 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Get the cursor position in text buffer position auto cursorArgs = winrt::make_self(); _CurrentCursorPositionHandlers(*this, *cursorArgs); - const COORD cursorPos = { gsl::narrow_cast(cursorArgs->CurrentPosition().X), gsl::narrow_cast(cursorArgs->CurrentPosition().Y) }; + const COORD cursorPos = { ::base::ClampedNumeric(cursorArgs->CurrentPosition().X), ::base::ClampedNumeric(cursorArgs->CurrentPosition().Y) }; // Get Font Info as we use this is the pixel size for characters in the display auto fontArgs = winrt::make_self(); _CurrentFontInfoHandlers(*this, *fontArgs); - const float fontWidth = fontArgs->FontSize().Width; - const float fontHeight = fontArgs->FontSize().Height; + const auto fontWidth = fontArgs->FontSize().Width; + const auto fontHeight = fontArgs->FontSize().Height; // Convert text buffer cursor position to client coordinate position within the window COORD clientCursorPos; - COORD screenCursorPos; - THROW_IF_FAILED(ShortMult(cursorPos.X, gsl::narrow(fontWidth), &clientCursorPos.X)); - THROW_IF_FAILED(ShortMult(cursorPos.Y, gsl::narrow(fontHeight), &clientCursorPos.Y)); + clientCursorPos.X = ::base::ClampMul(cursorPos.X, ::base::ClampedNumeric(fontWidth)); + clientCursorPos.Y = ::base::ClampMul(cursorPos.Y, ::base::ClampedNumeric(fontHeight)); // Convert from client coordinate to screen coordinate by adding window position - THROW_IF_FAILED(ShortAdd(clientCursorPos.X, gsl::narrow_cast(windowBounds.X), &screenCursorPos.X)); - THROW_IF_FAILED(ShortAdd(clientCursorPos.Y, gsl::narrow_cast(windowBounds.Y), &screenCursorPos.Y)); + COORD screenCursorPos; + screenCursorPos.X = ::base::ClampAdd(clientCursorPos.X, ::base::ClampedNumeric(windowBounds.X)); + screenCursorPos.Y = ::base::ClampAdd(clientCursorPos.Y, ::base::ClampedNumeric(windowBounds.Y)); // get any offset (margin + tabs, etc..) of the control within the window const auto offsetPoint = this->TransformToVisual(nullptr).TransformPoint(winrt::Windows::Foundation::Point(0, 0)); // add the margin offsets if any - const auto currentMargin = this->Margin(); - THROW_IF_FAILED(ShortAdd(screenCursorPos.X, gsl::narrow_cast(offsetPoint.X), &screenCursorPos.X)); - THROW_IF_FAILED(ShortAdd(screenCursorPos.Y, gsl::narrow_cast(offsetPoint.Y), &screenCursorPos.Y)); + screenCursorPos.X = ::base::ClampAdd(screenCursorPos.X, ::base::ClampedNumeric(offsetPoint.X)); + screenCursorPos.Y = ::base::ClampAdd(screenCursorPos.Y, ::base::ClampedNumeric(offsetPoint.Y)); // Get scale factor for view const double scaleFactor = DisplayInformation::GetForCurrentView().RawPixelsPerViewPixel(); @@ -178,12 +178,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // position textblock to cursor position _canvas.SetLeft(_textBlock, clientCursorPos.X); - _canvas.SetTop(_textBlock, static_cast(clientCursorPos.Y)); + _canvas.SetTop(_textBlock, ::base::ClampedNumeric(clientCursorPos.Y)); - // width is cursor to end of canvas - _textBlock.Width(200); // TODO GitHub #3640: Determine proper Width _textBlock.Height(fontHeight); - // calculate FontSize in pixels from DIPs const double fontSizePx = (fontHeight * 72) / USER_DEFAULT_SCREEN_DPI; _textBlock.FontSize(fontSizePx); @@ -201,8 +198,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - void TSFInputControl::_compositionStartedHandler(CoreTextEditContext sender, CoreTextCompositionStartedEventArgs const& /*args*/) { - _canvas.Visibility(Visibility::Visible); - _textBlock.Visibility(Visibility::Visible); + _inComposition = true; } // Method Description: @@ -215,30 +211,19 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - void TSFInputControl::_compositionCompletedHandler(CoreTextEditContext sender, CoreTextCompositionCompletedEventArgs const& /*args*/) { + _inComposition = false; + // only need to do work if the current buffer has text if (!_inputBuffer.empty()) { - // call event handler with data handled by parent - _compositionCompletedHandlers(_inputBuffer); - - // clear the buffer for next round - const auto bufferLength = gsl::narrow_cast(_inputBuffer.length()); - _inputBuffer.clear(); - _textBlock.Text(L""); - - // indicate text is now 0 - _editContext.NotifyTextChanged({ 0, bufferLength }, 0, { 0, 0 }); - - // hide the controls until composition starts again - _canvas.Visibility(Visibility::Collapsed); - _textBlock.Visibility(Visibility::Collapsed); + _SendAndClearText(); } } // Method Description: // - Handler for FocusRemoved event by CoreEditContext responsible // for removing focus for the TSFInputControl control accordingly - // when focus was forcibly removed from text input control. (TODO GitHub #3644) + // when focus was forcibly removed from text input control. // NOTE: Documentation says application should handle this event // Arguments: // - sender: CoreTextEditContext sending the request. Not used in method. @@ -265,7 +250,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation try { - const auto textRequested = _inputBuffer.substr(range.StartCaretPosition, static_cast(range.EndCaretPosition) - static_cast(range.StartCaretPosition)); + const auto textEnd = ::base::ClampMin(range.EndCaretPosition, _inputBuffer.length()); + const auto length = ::base::ClampSub(textEnd, range.StartCaretPosition); + const auto textRequested = _inputBuffer.substr(range.StartCaretPosition, length); args.Request().Text(textRequested); } @@ -315,13 +302,24 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation try { + _canvas.Visibility(Visibility::Visible); + _textBlock.Visibility(Visibility::Visible); + + const auto length = ::base::ClampSub(range.EndCaretPosition, range.StartCaretPosition); _inputBuffer = _inputBuffer.replace( range.StartCaretPosition, - static_cast(range.EndCaretPosition) - static_cast(range.StartCaretPosition), + length, text); _textBlock.Text(_inputBuffer); + // If we receive tabbed IME input like emoji, kaomojis, and symbols, send it to the terminal immediately. + // They aren't composition, so we don't want to wait for the user to start and finish a composition to send the text. + if (!_inComposition) + { + _SendAndClearText(); + } + // Notify the TSF that the update succeeded args.Result(CoreTextTextUpdatingResult::Succeeded); } @@ -334,6 +332,35 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } } + // Method Description: + // - Sends the currently held text in the input buffer to the parent and + // clears the input buffer and text block for the next round of input. + // Then hides the text block control until the next time text received. + // Arguments: + // - + // Return Value: + // - + void TSFInputControl::_SendAndClearText() + { + // call event handler with data handled by parent + _compositionCompletedHandlers(_inputBuffer); + + // clear the buffer for next round + const auto bufferLength = ::base::ClampedNumeric(_inputBuffer.length()); + _inputBuffer.clear(); + _textBlock.Text(L""); + + // Leaving focus before NotifyTextChanged seems to guarantee that the next + // composition will send us a CompositionStarted event. + _editContext.NotifyFocusLeave(); + _editContext.NotifyTextChanged({ 0, bufferLength }, 0, { 0, 0 }); + _editContext.NotifyFocusEnter(); + + // hide the controls until text input starts again + _canvas.Visibility(Visibility::Collapsed); + _textBlock.Visibility(Visibility::Collapsed); + } + // Method Description: // - Handler for FormatUpdating event by CoreEditContext responsible // for handling different format updates for a particular range of text. diff --git a/src/cascadia/TerminalControl/TSFInputControl.h b/src/cascadia/TerminalControl/TSFInputControl.h index d6e7b2501d..8438d37bbb 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.h +++ b/src/cascadia/TerminalControl/TSFInputControl.h @@ -75,6 +75,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation std::wstring _inputBuffer; void _Create(); + + bool _inComposition; + void _SendAndClearText(); }; } namespace winrt::Microsoft::Terminal::TerminalControl::factory_implementation diff --git a/src/cascadia/WinRTUtils/inc/Utils.h b/src/cascadia/WinRTUtils/inc/Utils.h index 08c2c079ba..2cbffd71cd 100644 --- a/src/cascadia/WinRTUtils/inc/Utils.h +++ b/src/cascadia/WinRTUtils/inc/Utils.h @@ -28,10 +28,10 @@ inline winrt::Windows::UI::Color ColorRefToColor(const COLORREF& colorref) // - Rect scaled by scale inline winrt::Windows::Foundation::Rect ScaleRect(winrt::Windows::Foundation::Rect rect, double scale) { - const float scaleLocal = gsl::narrow_cast(scale); - rect.X *= scaleLocal; - rect.Y *= scaleLocal; - rect.Width *= scaleLocal; - rect.Height *= scaleLocal; + const auto scaleLocal = base::ClampedNumeric(scale); + rect.X = base::ClampMul(rect.X, scaleLocal); + rect.Y = base::ClampMul(rect.Y, scaleLocal); + rect.Width = base::ClampMul(rect.Width, scaleLocal); + rect.Height = base::ClampMul(rect.Height, scaleLocal); return rect; } From 61e5917fe8a3af57cb7711b315cf7c2eba9b2c74 Mon Sep 17 00:00:00 2001 From: Leon Liang <57155886+leonMSFT@users.noreply.github.com> Date: Wed, 26 Feb 2020 11:28:01 -0800 Subject: [PATCH 04/13] Allow IME input in Search Box (#4723) ## Summary of the Pull Request Currently, when the user attempts to type using IME while the Search Box is focused, the input goes to the terminal instead. This is due to the fact that the `TSFInputControl` assumes it's in control whenever TermControl gets focus. So, it'll intercept IME input before the Search Box receives it. We simply need to modify `TermControl::GotFocus` to check if the SearchBox has focus. If it does, `TSFInputControl::NotifyFocusEnter` shouldn't be called. As a small side fix, I've also disabled the terminal cursor blinking when the Search Box has focus. Thinking a little further, if we have more features in the future that behave like search box (i.e. advanced tab switcher, or any other XAML controls that pop up) and require text input, we might need to create a sort of "AnyOtherTextControlInFocus" function to see if TSFInputControl should receive focus or not. ## PR Checklist * [x] Closes #4434 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed ## Validation Steps Performed Search works as expected with IME. Composition picker appears underneath Search Box when typing IME in the Search Box. Clicking outside of the Search Box still returns control to TSFInputControl/TermControl. Terminal Cursor blinks when it has focus, and doesn't when the Search Box has focus. --- src/cascadia/TerminalControl/TermControl.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 486941f4c3..25b86aa7e3 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1384,8 +1384,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { return; } + _focused = true; + // If the searchbox is focused, we don't want TSFInputControl to think + // it has focus so it doesn't intercept IME input. We also don't want the + // terminal's cursor to start blinking. So, we'll just return quickly here. + if (_searchBox && _searchBox->ContainsFocus()) + { + return; + } + if (_uiaEngine.get()) { THROW_IF_FAILED(_uiaEngine->Enable()); From d7ea526c3c950b4342c0c204d946174c5871ffe4 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 26 Feb 2020 13:55:22 -0800 Subject: [PATCH 05/13] Don't split surrogate pairs when breaking runs for scaling. Affects emoji rendering. #4704 (#4731) ## Summary of the Pull Request - Surrogate pairs are being split in half with the run splitting check. ## References - Related to #4708 but not going to fix it. ## PR Checklist * [x] Closes #4704 * [x] I work here. * [x] I am a core contributor. ## Detailed Description of the Pull Request / Additional comments - The adjustment of the run heights in the correction function reports back a text index and a scaling factor. However, I didn't remember at the time that the text is being stored as UTF-16. So the index given can be pointing to the high surrogate of a pair. Thus adding 1 to split "after" the text character, then backing up by 1 isn't valid in if the index given was for a high surrogate. The quick fix is to advance by two if it's a high surrogate and one otherwise. ## Validation Steps Performed - Used the sample code from #4704 to print the house emoji in various situations into the buffer. --- src/renderer/dx/CustomTextLayout.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp index ea809fe956..834f9ac88d 100644 --- a/src/renderer/dx/CustomTextLayout.cpp +++ b/src/renderer/dx/CustomTextLayout.cpp @@ -361,13 +361,16 @@ CustomTextLayout::CustomTextLayout(gsl::not_null const factory // If scale corrections were needed, we need to split the run. for (auto [index, scale] : _glyphScaleCorrections) { + // Don't split in the middle of a surrogate pair. + const auto after = IS_HIGH_SURROGATE(_text.at(index)) ? 2 : 1; + // Split after the adjustment first so it // takes a copy of all the run properties before we modify them. // GH 4665: This is the other half of the potential future perf item. // If glyphs needing the same scale are coalesced, we could // break fewer times and have fewer runs. - _SetCurrentRun(index + 1); - _SplitCurrentRun(index + 1); + _SetCurrentRun(index + after); + _SplitCurrentRun(index + after); // Now split just this glyph off. _SetCurrentRun(index); From 9f53107f000166d8579964aa3baa3f4254e27d09 Mon Sep 17 00:00:00 2001 From: Zoey Riordan Date: Wed, 26 Feb 2020 16:30:05 -0800 Subject: [PATCH 06/13] Fix PublicTerminalCore APIset usage on Windows 7 (#4733) The new UIA code in PublicTerminalCore broke windows 7 support by referring to API set dlls for UIA. This PR changes the link line to link to UIAutomationcore.dll directly. --- src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj b/src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj index b67df7d335..c1507c0702 100644 --- a/src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj +++ b/src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj @@ -54,7 +54,7 @@ instead of APISet forwarders for easier Windows 7 compatibility. --> - onecoreuap.lib;%(AdditionalDependencies) + Uiautomationcore.lib;onecoreuap.lib;%(AdditionalDependencies) From 1de07aa3aba4e857db2c32427760413488816166 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Wed, 26 Feb 2020 16:35:16 -0800 Subject: [PATCH 07/13] Convert TermControl to .xaml (#4729) This commit removes all of the custom UI initialization code in TermControl and replaces it with a xaml file. Some dead or reundant code was removed as part of this refactoring. It also fixes two (quasi-related) issues: * The search box, on first launch, was offset by the scrollbar even if the scrollbar was invisible. * The scrollbar state wasn't hot-reloadable. --- src/cascadia/TerminalControl/TermControl.cpp | 332 ++++++------------ src/cascadia/TerminalControl/TermControl.h | 23 +- src/cascadia/TerminalControl/TermControl.xaml | 81 +++++ .../TerminalControl/TerminalControl.vcxproj | 11 +- 4 files changed, 198 insertions(+), 249 deletions(-) create mode 100644 src/cascadia/TerminalControl/TermControl.xaml diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 25b86aa7e3..d141855dc5 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -18,6 +18,7 @@ using namespace ::Microsoft::Console::Types; using namespace ::Microsoft::Terminal::Core; using namespace winrt::Windows::UI::Xaml; +using namespace winrt::Windows::UI::Xaml::Input; using namespace winrt::Windows::UI::Xaml::Automation::Peers; using namespace winrt::Windows::UI::Core; using namespace winrt::Windows::System; @@ -53,8 +54,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation TermControl::TermControl(Settings::IControlSettings settings, TerminalConnection::ITerminalConnection connection) : _connection{ connection }, _initializedTerminal{ false }, - _root{ nullptr }, - _swapChainPanel{ nullptr }, _settings{ settings }, _closing{ false }, _isTerminalInitiatedScroll{ false }, @@ -69,56 +68,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _lastMouseClick{}, _lastMouseClickPos{}, _searchBox{ nullptr }, - _tsfInputControl{ nullptr }, _unfocusedClickPos{ std::nullopt }, _isClickDragSelection{ false } { _EnsureStaticInitialization(); - _Create(); - } - - void TermControl::_Create() - { - Controls::Grid container; - - Controls::ColumnDefinition contentColumn{}; - Controls::ColumnDefinition scrollbarColumn{}; - contentColumn.Width(GridLength{ 1.0, GridUnitType::Star }); - scrollbarColumn.Width(GridLength{ 1.0, GridUnitType::Auto }); - - container.ColumnDefinitions().Append(contentColumn); - container.ColumnDefinitions().Append(scrollbarColumn); - - _scrollBar = Controls::Primitives::ScrollBar{}; - _scrollBar.Orientation(Controls::Orientation::Vertical); - _scrollBar.IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::MouseIndicator); - _scrollBar.HorizontalAlignment(HorizontalAlignment::Right); - _scrollBar.VerticalAlignment(VerticalAlignment::Stretch); - - // Initialize the scrollbar with some placeholder values. - // The scrollbar will be updated with real values on _Initialize - _scrollBar.Maximum(1); - _scrollBar.ViewportSize(10); - _scrollBar.IsTabStop(false); - _scrollBar.SmallChange(1); - _scrollBar.LargeChange(4); - _scrollBar.Visibility(Visibility::Visible); - - _tsfInputControl = TSFInputControl(); - _tsfInputControl.CompositionCompleted({ this, &TermControl::_CompositionCompleted }); - _tsfInputControl.CurrentCursorPosition({ this, &TermControl::_CurrentCursorPositionHandler }); - _tsfInputControl.CurrentFontInfo({ this, &TermControl::_FontInfoHandler }); - container.Children().Append(_tsfInputControl); - - // Create the SwapChainPanel that will display our content - Controls::SwapChainPanel swapChainPanel; - - _sizeChangedRevoker = swapChainPanel.SizeChanged(winrt::auto_revoke, { this, &TermControl::_SwapChainSizeChanged }); - _compositionScaleChangedRevoker = swapChainPanel.CompositionScaleChanged(winrt::auto_revoke, { this, &TermControl::_SwapChainScaleChanged }); + InitializeComponent(); // Initialize the terminal only once the swapchainpanel is loaded - that // way, we'll be able to query the real pixel size it got on layout - _layoutUpdatedRevoker = swapChainPanel.LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { + _layoutUpdatedRevoker = SwapChainPanel().LayoutUpdated(winrt::auto_revoke, [this](auto /*s*/, auto /*e*/) { // This event fires every time the layout changes, but it is always the last one to fire // in any layout change chain. That gives us great flexibility in finding the right point // at which to initialize our renderer (and our terminal). @@ -131,74 +89,29 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } }); - container.Children().Append(swapChainPanel); - container.Children().Append(_scrollBar); - Controls::Grid::SetColumn(swapChainPanel, 0); - Controls::Grid::SetColumn(_scrollBar, 1); - - Controls::Grid root{}; - Controls::Image bgImageLayer{}; - root.Children().Append(bgImageLayer); - root.Children().Append(container); - - _root = root; - _bgImageLayer = bgImageLayer; - - _swapChainPanel = swapChainPanel; - this->Content(_root); - - _ApplyUISettings(); - - // These are important: - // 1. When we get tapped, focus us - _tappedRevoker = this->Tapped(winrt::auto_revoke, [this](auto&, auto& e) { - Focus(FocusState::Pointer); - e.Handled(true); - }); - // 2. Make sure we can be focused (why this isn't `Focusable` I'll never know) - this->IsTabStop(true); - // 3. Actually not sure about this one. Maybe it isn't necessary either. - this->AllowFocusOnInteraction(true); - - // DON'T CALL _InitializeTerminal here - wait until the swap chain is loaded to do that. - // Subscribe to the connection's disconnected event and call our connection closed handlers. _connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) { _ConnectionStateChangedHandlers(*this, nullptr); }); - _root.AllowDrop(true); - _root.Drop({ get_weak(), &TermControl::_DragDropHandler }); - _root.DragOver({ get_weak(), &TermControl::_DragOverHandler }); + _ApplyUISettings(); } // Method Description: - // - Create the SearchBoxControl object, and attach it - // to the Terminal Control root - // Arguments: - // - - // Return Value: - // - + // - Loads the search box from the xaml UI and focuses it. void TermControl::CreateSearchBoxControl() { - if (!_searchBox) + // Lazy load the search box control. + if (auto loadedSearchBox{ FindName(L"SearchBox") }) { - _searchBox = winrt::make_self(); - _searchBox->HorizontalAlignment(HorizontalAlignment::Right); - _searchBox->VerticalAlignment(VerticalAlignment::Top); - // We need to make sure the searchbox does not overlap - // with the scroll bar - Thickness searchBoxPadding = { 0, 0, _scrollBar.ActualWidth(), 0 }; - _searchBox->Margin(searchBoxPadding); - - _root.Children().Append(*_searchBox); - - // Event handlers - _searchBox->Search({ get_weak(), &TermControl::_Search }); - _searchBox->Closed({ get_weak(), &TermControl::_CloseSearchBoxControl }); + if (auto searchBox{ loadedSearchBox.try_as<::winrt::Microsoft::Terminal::TerminalControl::SearchBoxControl>() }) + { + // get at its private implementation + _searchBox.copy_from(winrt::get_self(searchBox)); + _searchBox->Visibility(Visibility::Visible); + _searchBox->SetFocusOnTextbox(); + } } - - _searchBox->SetFocusOnTextbox(); } // Method Description: @@ -238,8 +151,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Method Description: // - The handler for the close button or pressing "Esc" when focusing on the // search dialog. - // This removes the SearchBoxControl object from the XAML tree, - // reset smart pointer and set focus back to Terminal // Arguments: // - IInspectable: not used // - RoutedEventArgs: not used @@ -247,11 +158,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - void TermControl::_CloseSearchBoxControl(const winrt::Windows::Foundation::IInspectable& /*sender*/, RoutedEventArgs const& /*args*/) { - unsigned int idx; - _root.Children().IndexOf(*_searchBox, idx); - _root.Children().RemoveAt(idx); - - _searchBox = nullptr; + _searchBox->Visibility(Visibility::Collapsed); // Set focus back to terminal control this->Focus(FocusState::Programmatic); @@ -270,7 +177,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Dispatch a call to the UI thread to apply the new settings to the // terminal. - co_await winrt::resume_foreground(_root.Dispatcher()); + co_await winrt::resume_foreground(Dispatcher()); // If 'weakThis' is locked, then we can safely work with 'this' if (auto control{ weakThis.get() }) @@ -287,8 +194,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Refresh our font with the renderer _UpdateFont(); - const auto width = _swapChainPanel.ActualWidth(); - const auto height = _swapChainPanel.ActualHeight(); + const auto width = SwapChainPanel().ActualWidth(); + const auto height = SwapChainPanel().ActualHeight(); if (width != 0 && height != 0) { // If the font size changed, or the _swapchainPanel's size changed @@ -297,11 +204,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto lock = _terminal->LockForWriting(); _DoResize(width, height); } - - // set TSF Foreground - Media::SolidColorBrush foregroundBrush{}; - foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground())); - _tsfInputControl.Foreground(foregroundBrush); } } @@ -326,8 +228,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Apply padding as swapChainPanel's margin auto newMargin = _ParseThicknessFromPadding(_settings.Padding()); - auto existingMargin = _swapChainPanel.Margin(); - _swapChainPanel.Margin(newMargin); + SwapChainPanel().Margin(newMargin); // Initialize our font information. const auto fontFace = _settings.FontFace(); @@ -344,8 +245,24 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // set TSF Foreground Media::SolidColorBrush foregroundBrush{}; foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground())); - _tsfInputControl.Foreground(foregroundBrush); - _tsfInputControl.Margin(newMargin); + TSFInputControl().Foreground(foregroundBrush); + TSFInputControl().Margin(newMargin); + + // Apply settings for scrollbar + if (_settings.ScrollState() == ScrollbarState::Hidden) + { + // In the scenario where the user has turned off the OS setting to automatically hide scollbars, the + // Terminal scrollbar would still be visible; so, we need to set the control's visibility accordingly to + // achieve the intended effect. + ScrollBar().IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::None); + ScrollBar().Visibility(Visibility::Collapsed); + } + else // (default or Visible) + { + // Default behavior + ScrollBar().IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::MouseIndicator); + ScrollBar().Visibility(Visibility::Visible); + } // set number of rows to scroll at a time _rowsToScroll = _settings.RowsToScroll(); @@ -371,7 +288,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // See if we've already got an acrylic background brush // to avoid the flicker when setting up a new one - auto acrylic = _root.Background().try_as(); + auto acrylic = RootGrid().Background().try_as(); // Instantiate a brush if there's not already one there if (acrylic == nullptr) @@ -396,15 +313,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation acrylic.TintOpacity(_settings.TintOpacity()); // Apply brush to control if it's not already there - if (_root.Background() != acrylic) + if (RootGrid().Background() != acrylic) { - _root.Background(acrylic); + RootGrid().Background(acrylic); } } else { Media::SolidColorBrush solidColor{}; - _root.Background(solidColor); + RootGrid().Background(solidColor); } if (!_settings.BackgroundImage().empty()) @@ -414,7 +331,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Check if the image brush is already pointing to the image // in the modified settings; if it isn't (or isn't there), // set a new image source for the brush - auto imageSource = _bgImageLayer.Source().try_as(); + auto imageSource = BackgroundImage().Source().try_as(); if (imageSource == nullptr || imageSource.UriSource() == nullptr || @@ -425,18 +342,18 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // may well be both large and somewhere out on the // internet. Media::Imaging::BitmapImage image(imageUri); - _bgImageLayer.Source(image); + BackgroundImage().Source(image); } // Apply stretch, opacity and alignment settings - _bgImageLayer.Stretch(_settings.BackgroundImageStretchMode()); - _bgImageLayer.Opacity(_settings.BackgroundImageOpacity()); - _bgImageLayer.HorizontalAlignment(_settings.BackgroundImageHorizontalAlignment()); - _bgImageLayer.VerticalAlignment(_settings.BackgroundImageVerticalAlignment()); + BackgroundImage().Stretch(_settings.BackgroundImageStretchMode()); + BackgroundImage().Opacity(_settings.BackgroundImageOpacity()); + BackgroundImage().HorizontalAlignment(_settings.BackgroundImageHorizontalAlignment()); + BackgroundImage().VerticalAlignment(_settings.BackgroundImageVerticalAlignment()); } else { - _bgImageLayer.Source(nullptr); + BackgroundImage().Source(nullptr); } } @@ -450,7 +367,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { auto weakThis{ get_weak() }; - co_await winrt::resume_foreground(_root.Dispatcher()); + co_await winrt::resume_foreground(Dispatcher()); if (auto control{ weakThis.get() }) { @@ -464,12 +381,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation bgColor.B = B; bgColor.A = 255; - if (auto acrylic = _root.Background().try_as()) + if (auto acrylic = RootGrid().Background().try_as()) { acrylic.FallbackColor(bgColor); acrylic.TintColor(bgColor); } - else if (auto solidColor = _root.Background().try_as()) + else if (auto solidColor = RootGrid().Background().try_as()) { solidColor.Color(bgColor); } @@ -522,9 +439,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return _actualFont; } - const Windows::UI::Xaml::Thickness TermControl::GetPadding() const + const Windows::UI::Xaml::Thickness TermControl::GetPadding() { - return _swapChainPanel.Margin(); + return SwapChainPanel().Margin(); } TerminalConnection::ConnectionState TermControl::ConnectionState() const @@ -542,13 +459,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto chain = _renderEngine->GetSwapChain(); auto weakThis{ get_weak() }; - co_await winrt::resume_foreground(_swapChainPanel.Dispatcher()); + co_await winrt::resume_foreground(Dispatcher()); // If 'weakThis' is locked, then we can safely work with 'this' if (auto control{ weakThis.get() }) { auto lock = _terminal->LockForWriting(); - auto nativePanel = _swapChainPanel.as(); + auto nativePanel = SwapChainPanel().as(); nativePanel->SetSwapChain(chain.Get()); } } @@ -558,12 +475,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto chain = _renderEngine->GetSwapChain(); auto weakThis{ get_weak() }; - co_await winrt::resume_foreground(_swapChainPanel.Dispatcher()); + co_await winrt::resume_foreground(Dispatcher()); if (auto control{ weakThis.get() }) { _terminal->LockConsole(); - auto nativePanel = _swapChainPanel.as(); + auto nativePanel = SwapChainPanel().as(); nativePanel->SetSwapChain(chain.Get()); _terminal->UnlockConsole(); } @@ -576,8 +493,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return false; } - const auto windowWidth = _swapChainPanel.ActualWidth(); // Width() and Height() are NaN? - const auto windowHeight = _swapChainPanel.ActualHeight(); + const auto windowWidth = SwapChainPanel().ActualWidth(); // Width() and Height() are NaN? + const auto windowHeight = SwapChainPanel().ActualHeight(); if (windowWidth == 0 || windowHeight == 0) { @@ -672,62 +589,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto bottom = _terminal->GetViewport().BottomExclusive(); auto bufferHeight = bottom; - const auto originalMaximum = _scrollBar.Maximum(); - const auto originalMinimum = _scrollBar.Minimum(); - const auto originalValue = _scrollBar.Value(); - const auto originalViewportSize = _scrollBar.ViewportSize(); - - _scrollBar.Maximum(bufferHeight - bufferHeight); - _scrollBar.Minimum(0); - _scrollBar.Value(0); - _scrollBar.ViewportSize(bufferHeight); - _scrollBar.ValueChanged({ this, &TermControl::_ScrollbarChangeHandler }); - _scrollBar.PointerPressed({ this, &TermControl::_CapturePointer }); - _scrollBar.PointerReleased({ this, &TermControl::_ReleasePointerCapture }); - - // Apply settings for scrollbar - if (_settings.ScrollState() == ScrollbarState::Visible) - { - _scrollBar.IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::MouseIndicator); - } - else if (_settings.ScrollState() == ScrollbarState::Hidden) - { - _scrollBar.IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::None); - - // In the scenario where the user has turned off the OS setting to automatically hide scollbars, the - // Terminal scrollbar would still be visible; so, we need to set the control's visibility accordingly to - // achieve the intended effect. - _scrollBar.Visibility(Visibility::Collapsed); - } - else - { - // Default behavior - _scrollBar.IndicatorMode(Controls::Primitives::ScrollingIndicatorMode::MouseIndicator); - } - - _root.PointerWheelChanged({ this, &TermControl::_MouseWheelHandler }); - - // These need to be hooked up to the SwapChainPanel because we don't want the scrollbar to respond to pointer events (GitHub #950) - _swapChainPanel.PointerPressed({ this, &TermControl::_PointerPressedHandler }); - _swapChainPanel.PointerMoved({ this, &TermControl::_PointerMovedHandler }); - _swapChainPanel.PointerReleased({ this, &TermControl::_PointerReleasedHandler }); + ScrollBar().Maximum(bufferHeight - bufferHeight); + ScrollBar().Minimum(0); + ScrollBar().Value(0); + ScrollBar().ViewportSize(bufferHeight); localPointerToThread->EnablePainting(); - // No matter what order these guys are in, The KeyDown's will fire - // before the CharacterReceived, so we can't easily get characters - // first, then fallback to getting keys from vkeys. - // TODO: This apparently handles keys and characters correctly, though - // I'd keep an eye on it, and test more. - // I presume that the characters that aren't translated by terminalInput - // just end up getting ignored, and the rest of the input comes - // through CharacterReceived. - // I don't believe there's a difference between KeyDown and - // PreviewKeyDown for our purposes - // These two handlers _must_ be on this, not _root. - this->PreviewKeyDown({ this, &TermControl::_KeyDownHandler }); - this->CharacterReceived({ this, &TermControl::_CharacterHandler }); - auto pfnTitleChanged = std::bind(&TermControl::_TerminalTitleChanged, this, std::placeholders::_1); _terminal->SetTitleChangedCallback(pfnTitleChanged); @@ -760,10 +628,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // import value from WinUser (convert from milli-seconds to micro-seconds) _multiClickTimer = GetDoubleClickTime() * 1000; - _gotFocusRevoker = this->GotFocus(winrt::auto_revoke, { this, &TermControl::_GotFocusHandler }); - _lostFocusRevoker = this->LostFocus(winrt::auto_revoke, { this, &TermControl::_LostFocusHandler }); - - // Focus the control here. If we do it up above (in _Create_), then the + // Focus the control here. If we do it during control initialization, then // focus won't actually get passed to us. I believe this is because // we're not technically a part of the UI tree yet, so focusing us // becomes a no-op. @@ -892,6 +757,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return handled; } + // Method Description: + // - handle a tap event by taking focus + // Arguments: + // - sender: the XAML element responding to the tap event + // - args: event data + void TermControl::_TappedHandler(const IInspectable& /*sender*/, const TappedRoutedEventArgs& e) + { + Focus(FocusState::Pointer); + e.Handled(true); + } + // Method Description: // - handle a mouse click event. Begin selection process. // Arguments: @@ -903,7 +779,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _CapturePointer(sender, args); const auto ptr = args.Pointer(); - const auto point = args.GetCurrentPoint(_root); + const auto point = args.GetCurrentPoint(*this); if (!_focused) { @@ -1007,7 +883,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation Input::PointerRoutedEventArgs const& args) { const auto ptr = args.Pointer(); - const auto point = args.GetCurrentPoint(_root); + const auto point = args.GetCurrentPoint(*this); if (ptr.PointerDeviceType() == Windows::Devices::Input::PointerDeviceType::Mouse || ptr.PointerDeviceType() == Windows::Devices::Input::PointerDeviceType::Pen) { @@ -1027,8 +903,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation const auto cursorPosition = point.Position(); _SetEndSelectionPointAtCursor(cursorPosition); - const double cursorBelowBottomDist = cursorPosition.Y - _swapChainPanel.Margin().Top - _swapChainPanel.ActualHeight(); - const double cursorAboveTopDist = -1 * cursorPosition.Y + _swapChainPanel.Margin().Top; + const double cursorBelowBottomDist = cursorPosition.Y - SwapChainPanel().Margin().Top - SwapChainPanel().ActualHeight(); + const double cursorAboveTopDist = -1 * cursorPosition.Y + SwapChainPanel().Margin().Top; constexpr double MinAutoScrollDist = 2.0; // Arbitrary value double newAutoScrollVelocity = 0.0; @@ -1071,10 +947,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // panning down) const float numRows = -1.0f * (dy / fontHeight); - const auto currentOffset = ::base::ClampedNumeric(_scrollBar.Value()); + const auto currentOffset = ::base::ClampedNumeric(ScrollBar().Value()); const auto newValue = numRows + currentOffset; - _scrollBar.Value(newValue); + ScrollBar().Value(newValue); // Use this point as our new scroll anchor. _touchAnchor = newTouchPoint; @@ -1135,7 +1011,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void TermControl::_MouseWheelHandler(Windows::Foundation::IInspectable const& /*sender*/, Input::PointerRoutedEventArgs const& args) { - const auto point = args.GetCurrentPoint(_root); + const auto point = args.GetCurrentPoint(*this); const auto delta = point.Properties().MouseWheelDelta(); // Get the state of the Ctrl & Shift keys @@ -1173,7 +1049,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { try { - auto acrylicBrush = _root.Background().as(); + auto acrylicBrush = RootGrid().Background().as(); acrylicBrush.TintOpacity(acrylicBrush.TintOpacity() + effectiveDelta); } CATCH_LOG(); @@ -1216,7 +1092,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - mouseDelta: the mouse wheel delta that triggered this event. void TermControl::_MouseScrollHandler(const double mouseDelta, Windows::UI::Input::PointerPoint const& pointerPoint) { - const auto currentOffset = _scrollBar.Value(); + const auto currentOffset = ScrollBar().Value(); // negative = down, positive = up // However, for us, the signs are flipped. @@ -1229,7 +1105,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // The scroll bar's ValueChanged handler will actually move the viewport // for us. - _scrollBar.Value(newValue); + ScrollBar().Value(newValue); if (_terminal->IsSelectionActive() && pointerPoint.Properties().IsLeftButtonPressed()) { @@ -1360,7 +1236,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { static constexpr double microSecPerSec = 1000000.0; const double deltaTime = std::chrono::duration_cast(timeNow - _lastAutoScrollUpdateTime.value()).count() / microSecPerSec; - _scrollBar.Value(_scrollBar.Value() + _autoScrollVelocity * deltaTime); + ScrollBar().Value(ScrollBar().Value() + _autoScrollVelocity * deltaTime); if (_autoScrollingPointerPoint.has_value()) { @@ -1400,9 +1276,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation THROW_IF_FAILED(_uiaEngine->Enable()); } - if (_tsfInputControl != nullptr) + if (TSFInputControl() != nullptr) { - _tsfInputControl.NotifyFocusEnter(); + TSFInputControl().NotifyFocusEnter(); } if (_cursorTimer.has_value()) @@ -1432,9 +1308,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation THROW_IF_FAILED(_uiaEngine->Disable()); } - if (_tsfInputControl != nullptr) + if (TSFInputControl() != nullptr) { - _tsfInputControl.NotifyFocusLeave(); + TSFInputControl().NotifyFocusLeave(); } if (_cursorTimer.has_value()) @@ -1498,7 +1374,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { auto lock = _terminal->LockForWriting(); - const int newDpi = static_cast(static_cast(USER_DEFAULT_SCREEN_DPI) * _swapChainPanel.CompositionScaleX()); + const int newDpi = static_cast(static_cast(USER_DEFAULT_SCREEN_DPI) * SwapChainPanel().CompositionScaleX()); // TODO: MSFT:20895307 If the font doesn't exist, this doesn't // actually fail. We need a way to gracefully fallback. @@ -1529,7 +1405,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // problems (like what happens when you change the font size while the // window is maximized?) auto lock = _terminal->LockForWriting(); - _DoResize(_swapChainPanel.ActualWidth(), _swapChainPanel.ActualHeight()); + _DoResize(SwapChainPanel().ActualWidth(), SwapChainPanel().ActualHeight()); } CATCH_LOG(); } @@ -1706,7 +1582,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto weakThis{ get_weak() }; - co_await winrt::resume_foreground(_scrollBar.Dispatcher()); + co_await winrt::resume_foreground(Dispatcher()); // Even if we weren't closed/closing few lines above, we might be // while waiting for this block of code to be dispatched. @@ -1716,7 +1592,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (!_closing.load()) { // Update our scrollbar - _ScrollbarUpdater(_scrollBar, viewTop, viewHeight, bufferSize); + _ScrollbarUpdater(ScrollBar(), viewTop, viewHeight, bufferSize); } } } @@ -1802,7 +1678,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _connection.TerminalOutput(_connectionOutputEventToken); _connectionStateChangedRevoker.revoke(); - _tsfInputControl.Close(); // Disconnect the TSF input control so it doesn't receive EditContext events. + TSFInputControl().Close(); // Disconnect the TSF input control so it doesn't receive EditContext events. if (auto localConnection{ std::exchange(_connection, nullptr) }) { @@ -1834,7 +1710,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - viewTop: the viewTop to scroll to void TermControl::ScrollViewport(int viewTop) { - _scrollBar.Value(viewTop); + ScrollBar().Value(viewTop); } int TermControl::GetScrollOffset() @@ -1949,7 +1825,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Return Value: // - The minimum size that this terminal control can be resized to and still // have a visible character. - winrt::Windows::Foundation::Size TermControl::MinimumSize() const + winrt::Windows::Foundation::Size TermControl::MinimumSize() { const auto fontSize = _actualFont.GetSize(); double width = fontSize.X; @@ -1957,11 +1833,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Reserve additional space if scrollbar is intended to be visible if (_settings.ScrollState() == ScrollbarState::Visible) { - width += _scrollBar.ActualWidth(); + width += ScrollBar().ActualWidth(); } // Account for the size of any padding - const auto padding = _swapChainPanel.Margin(); + const auto padding = SwapChainPanel().Margin(); width += padding.Left + padding.Right; height += padding.Top + padding.Bottom; @@ -1976,19 +1852,19 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - dimension: a dimension (width or height) to be snapped // Return Value: // - A dimension that would be aligned to the character grid. - float TermControl::SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const + float TermControl::SnapDimensionToGrid(const bool widthOrHeight, const float dimension) { const auto fontSize = _actualFont.GetSize(); const auto fontDimension = widthOrHeight ? fontSize.X : fontSize.Y; - const auto padding = _swapChainPanel.Margin(); + const auto padding = SwapChainPanel().Margin(); auto nonTerminalArea = gsl::narrow_cast(widthOrHeight ? padding.Left + padding.Right : padding.Top + padding.Bottom); if (widthOrHeight && _settings.ScrollState() == ScrollbarState::Visible) { - nonTerminalArea += gsl::narrow_cast(_scrollBar.ActualWidth()); + nonTerminalArea += gsl::narrow_cast(ScrollBar().ActualWidth()); } const auto gridSize = dimension - nonTerminalArea; @@ -2113,8 +1989,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // Exclude padding from cursor position calculation COORD terminalPosition = { - static_cast(cursorPosition.X - _swapChainPanel.Margin().Left), - static_cast(cursorPosition.Y - _swapChainPanel.Margin().Top) + static_cast(cursorPosition.X - SwapChainPanel().Margin().Left), + static_cast(cursorPosition.Y - SwapChainPanel().Margin().Top) }; const auto fontSize = _actualFont.GetSize(); diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index ce4991eaca..ffa7fb6797 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -66,8 +66,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void PasteTextFromClipboard(); void Close(); Windows::Foundation::Size CharacterDimensions() const; - Windows::Foundation::Size MinimumSize() const; - float SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const; + Windows::Foundation::Size MinimumSize(); + float SnapDimensionToGrid(const bool widthOrHeight, const float dimension); void ScrollViewport(int viewTop); int GetScrollOffset(); @@ -85,7 +85,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation Windows::UI::Xaml::Automation::Peers::AutomationPeer OnCreateAutomationPeer(); ::Microsoft::Console::Types::IUiaData* GetUiaData() const; const FontInfo GetActualFont() const; - const Windows::UI::Xaml::Thickness GetPadding() const; + const Windows::UI::Xaml::Thickness GetPadding(); TerminalConnection::ConnectionState ConnectionState() const; @@ -105,20 +105,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // clang-format on private: + friend struct TermControlT; // friend our parent so it can bind private event handlers TerminalConnection::ITerminalConnection _connection; bool _initializedTerminal; - Windows::UI::Xaml::Controls::Grid _root; - Windows::UI::Xaml::Controls::Image _bgImageLayer; - Windows::UI::Xaml::Controls::SwapChainPanel _swapChainPanel; - Windows::UI::Xaml::Controls::Primitives::ScrollBar _scrollBar; - winrt::com_ptr _searchBox; - TSFInputControl _tsfInputControl; - event_token _connectionOutputEventToken; - TermControl::Tapped_revoker _tappedRevoker; TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker; std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal; @@ -165,21 +158,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation std::optional _unfocusedClickPos; bool _isClickDragSelection; - // Event revokers -- we need to deregister ourselves before we die, - // lest we get callbacks afterwards. - winrt::Windows::UI::Xaml::Controls::Control::SizeChanged_revoker _sizeChangedRevoker; - winrt::Windows::UI::Xaml::Controls::SwapChainPanel::CompositionScaleChanged_revoker _compositionScaleChangedRevoker; winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker; - winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker; - winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker; - void _Create(); void _ApplyUISettings(); void _InitializeBackgroundBrush(); winrt::fire_and_forget _BackgroundColorChanged(const uint32_t color); bool _InitializeTerminal(); void _UpdateFont(const bool initialUpdate = false); void _SetFontSize(int fontSize); + void _TappedHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::TappedRoutedEventArgs const& e); void _KeyDownHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e); void _CharacterHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::CharacterReceivedRoutedEventArgs const& e); void _PointerPressedHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& e); diff --git a/src/cascadia/TerminalControl/TermControl.xaml b/src/cascadia/TerminalControl/TermControl.xaml new file mode 100644 index 0000000000..734edf938b --- /dev/null +++ b/src/cascadia/TerminalControl/TermControl.xaml @@ -0,0 +1,81 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj b/src/cascadia/TerminalControl/TerminalControl.vcxproj index af6cbe4d98..2ae8ea7e4c 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj @@ -37,7 +37,7 @@ SearchBoxControl.xaml - TermControl.idl + TermControl.xaml TermControlAutomationPeer.idl @@ -56,7 +56,7 @@ SearchBoxControl.xaml - TermControl.idl + TermControl.xaml TSFInputControl.idl @@ -71,7 +71,9 @@ SearchBoxControl.xaml - + + TermControl.xaml + @@ -109,6 +111,9 @@ Designer + + Designer + From e5182fb3e8852bd54f62c2f9b8d3fea903f17807 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 27 Feb 2020 10:40:11 -0600 Subject: [PATCH 08/13] Make Conpty emit wrapped lines as actually wrapped lines (#4415) ## Summary of the Pull Request Changes how conpty emits text to preserve line-wrap state, and additionally adds rudimentary support to the Windows Terminal for wrapped lines. ## References * Does _not_ fix (!) #3088, but that might be lower down in conhost. This makes wt behave like conhost, so at least there's that * Still needs a proper deferred EOL wrap implementation in #780, which is left as a todo * #4200 is the mega bucket with all this work * MSFT:16485846 was the first attempt at this task, which caused the regression MSFT:18123777 so we backed it out. * #4403 - I made sure this worked with that PR before I even sent #4403 ## PR Checklist * [x] Closes #405 * [x] Closes #3367 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments I started with the following implementation: When conpty is about to write the last column, note that we wrapped this line here. If the next character the vt renderer is told to paint get is supposed to be at the start of the following line, then we know that the previous line had wrapped, so we _won't_ emit the usual `\r\n` here, and we'll just continue emitting text. However, this isn't _exactly_ right - if someone fills the row _exactly_ with text, the information that's available to the vt renderer isn't enough to know for sure if this line broke or not. It is possible for the client to write a full line of text, with a `\n` at the end, to manually break the line. So, I had to also add the `lineWrapped` param to the `IRenderEngine` interface, which is about half the files in this changelist. ## Validation Steps Performed * Ran tests * Checked how the Windows Terminal behaves with these changes * Made sure that conhost/inception and gnome-terminal both act as you'd expect with wrapped lines from conpty --- src/cascadia/TerminalCore/Terminal.cpp | 16 ++ .../ConptyRoundtripTests.cpp | 265 +++++++++++++++++- .../TerminalBufferTests.cpp | 76 +++++ .../UnitTests_TerminalCore/TestUtils.h | 4 + src/host/ut_host/VtRendererTests.cpp | 10 +- src/interactivity/onecore/BgfxEngine.cpp | 3 +- src/interactivity/onecore/BgfxEngine.hpp | 3 +- src/renderer/base/renderer.cpp | 16 +- src/renderer/base/renderer.hpp | 3 +- src/renderer/dx/DxRenderer.cpp | 3 +- src/renderer/dx/DxRenderer.hpp | 3 +- src/renderer/gdi/gdirenderer.hpp | 3 +- src/renderer/gdi/paint.cpp | 3 +- src/renderer/inc/IRenderEngine.hpp | 3 +- src/renderer/uia/UiaRenderer.cpp | 3 +- src/renderer/uia/UiaRenderer.hpp | 3 +- src/renderer/vt/XtermEngine.cpp | 24 +- src/renderer/vt/XtermEngine.hpp | 4 +- src/renderer/vt/paint.cpp | 40 ++- src/renderer/vt/tracing.cpp | 45 +++ src/renderer/vt/tracing.hpp | 3 + src/renderer/vt/vtrenderer.hpp | 8 +- src/renderer/wddmcon/WddmConRenderer.cpp | 3 +- src/renderer/wddmcon/WddmConRenderer.hpp | 3 +- 24 files changed, 512 insertions(+), 35 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 4179e19cc3..e4acabbdc9 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -454,6 +454,22 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) // With well behaving shells during normal operation this safeguard should normally not be encountered. proposedCursorPosition.X = 0; proposedCursorPosition.Y++; + + // Try the character again. + i--; + + // Mark the line we're currently on as wrapped + + // TODO: GH#780 - This should really be a _deferred_ newline. If + // the next character to come in is a newline or a cursor + // movement or anything, then we should _not_ wrap this line + // here. + // + // This is more WriteCharsLegacy2ElectricBoogaloo work. I'm + // leaving it like this for now - it'll break for lines that + // _exactly_ wrap, but we can't re-wrap lines now anyways, so it + // doesn't matter. + _buffer->GetRowByOffset(cursorPosBefore.Y).GetCharRow().SetWrapForced(true); } _AdjustCursorPosition(proposedCursorPosition); diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 112cf39643..5c031185c5 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -44,7 +44,7 @@ using namespace Microsoft::Terminal::Core; namespace TerminalCoreUnitTests { - class TerminalBufferTests; + class ConptyRoundtripTests; }; using namespace TerminalCoreUnitTests; @@ -152,8 +152,14 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final TEST_METHOD(SimpleWriteOutputTest); TEST_METHOD(WriteTwoLinesUsesNewline); TEST_METHOD(WriteAFewSimpleLines); + TEST_METHOD(PassthroughClearScrollback); + TEST_METHOD(TestWrappingALongString); + TEST_METHOD(TestAdvancedWrapping); + TEST_METHOD(TestExactWrappingWithoutSpaces); + TEST_METHOD(TestExactWrappingWithSpaces); + TEST_METHOD(MoveCursorAtEOL); private: @@ -348,6 +354,262 @@ void ConptyRoundtripTests::WriteAFewSimpleLines() verifyData(termTb); } +void ConptyRoundtripTests::TestWrappingALongString() +{ + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + _flushFirstFrame(); + _checkConptyOutput = false; + + const auto initialTermView = term->GetViewport(); + + const auto charsToWrite = gsl::narrow_cast(TestUtils::Test100CharsString.size()); + VERIFY_ARE_EQUAL(100, charsToWrite); + + VERIFY_ARE_EQUAL(0, initialTermView.Top()); + VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive()); + + hostSm.ProcessString(TestUtils::Test100CharsString); + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + auto verifyBuffer = [&](const TextBuffer& tb) { + auto& cursor = tb.GetCursor(); + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(charsToWrite % initialTermView.Width(), cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = tb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = tb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(tb, TestUtils::Test100CharsString, { 0, 0 }); + }; + + verifyBuffer(hostTb); + + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb); +} + +void ConptyRoundtripTests::TestAdvancedWrapping() +{ + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + const auto initialTermView = term->GetViewport(); + + _flushFirstFrame(); + + const auto charsToWrite = gsl::narrow_cast(TestUtils::Test100CharsString.size()); + VERIFY_ARE_EQUAL(100, charsToWrite); + + hostSm.ProcessString(TestUtils::Test100CharsString); + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L" "); + hostSm.ProcessString(L"1234567890"); + + auto verifyBuffer = [&](const TextBuffer& tb) { + auto& cursor = tb.GetCursor(); + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(2, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(20, cursor.GetPosition().X); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = tb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = tb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(tb, TestUtils::Test100CharsString, { 0, 0 }); + TestUtils::VerifyExpectedString(tb, L" 1234567890", { 0, 2 }); + }; + + verifyBuffer(hostTb); + + // First write the first 80 characters from the string + expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); + // Without line breaking, write the remaining 20 chars + expectedOutput.push_back(R"(qrstuvwxyz{|}~!"#$%&)"); + // Clear the rest of row 1 + expectedOutput.push_back("\x1b[K"); + // This is the hard line break + expectedOutput.push_back("\r\n"); + // Now write row 2 of the buffer + expectedOutput.push_back(" 1234567890"); + // and clear everything after the text, because the buffer is empty. + expectedOutput.push_back("\x1b[K"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb); +} + +void ConptyRoundtripTests::TestExactWrappingWithoutSpaces() +{ + // This test (and TestExactWrappingWitSpaces) reveals a bug in the old + // implementation. + // + // If a line _exactly_ wraps to the next line, we can't tell if the line + // should really wrap, or manually break. The client app is writing a line + // that's exactly the width of the buffer that manually linebreaked at the + // end of the line, followed by another line. + // + // With the old PaintBufferLine interface, there's no way to know if this + // case is because the line wrapped or not. Hence, the addition of the + // `lineWrapped` parameter + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + + const auto initialTermView = term->GetViewport(); + + _flushFirstFrame(); + + const auto charsToWrite = initialTermView.Width(); + VERIFY_ARE_EQUAL(80, charsToWrite); + + for (auto i = 0; i < charsToWrite; i++) + { + // This is a handy way of just printing the printable characters that + // _aren't_ the space character. + const wchar_t wch = static_cast(33 + (i % 94)); + hostSm.ProcessCharacter(wch); + } + + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L"1234567890"); + + auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) { + auto& cursor = tb.GetCursor(); + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(10, cursor.GetPosition().X); + + // TODO: GH#780 - In the Terminal, neither line should be wrapped. + // Unfortunately, until WriteCharsLegacy2ElectricBoogaloo is complete, + // the Terminal will still treat the first line as wrapped. When #780 is + // implemented, these tests will fail, and should again expect the first + // line to not be wrapped. + + // Verify that we marked the 0th row as _not wrapped_ + const auto& row0 = tb.GetRowByOffset(0); + VERIFY_ARE_EQUAL(isTerminal, row0.GetCharRow().WasWrapForced()); + + const auto& row1 = tb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(tb, LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)", { 0, 0 }); + TestUtils::VerifyExpectedString(tb, L"1234567890", { 0, 1 }); + }; + + verifyBuffer(hostTb, false); + + // First write the first 80 characters from the string + expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); + + // This is the hard line break + expectedOutput.push_back("\r\n"); + // Now write row 2 of the buffer + expectedOutput.push_back("1234567890"); + // and clear everything after the text, because the buffer is empty. + expectedOutput.push_back("\x1b[K"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb, true); +} + +void ConptyRoundtripTests::TestExactWrappingWithSpaces() +{ + // This test is also explained by the comment at the top of TestExactWrappingWithoutSpaces + + auto& g = ServiceLocator::LocateGlobals(); + auto& renderer = *g.pRender; + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& hostSm = si.GetStateMachine(); + auto& hostTb = si.GetTextBuffer(); + auto& termTb = *term->_buffer; + const auto initialTermView = term->GetViewport(); + + _flushFirstFrame(); + + const auto charsToWrite = initialTermView.Width(); + VERIFY_ARE_EQUAL(80, charsToWrite); + + for (auto i = 0; i < charsToWrite; i++) + { + // This is a handy way of just printing the printable characters that + // _aren't_ the space character. + const wchar_t wch = static_cast(33 + (i % 94)); + hostSm.ProcessCharacter(wch); + } + + hostSm.ProcessString(L"\n"); + hostSm.ProcessString(L" "); + hostSm.ProcessString(L"1234567890"); + + auto verifyBuffer = [&](const TextBuffer& tb, const bool isTerminal) { + auto& cursor = tb.GetCursor(); + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + VERIFY_ARE_EQUAL(20, cursor.GetPosition().X); + + // TODO: GH#780 - In the Terminal, neither line should be wrapped. + // Unfortunately, until WriteCharsLegacy2ElectricBoogaloo is complete, + // the Terminal will still treat the first line as wrapped. When #780 is + // implemented, these tests will fail, and should again expect the first + // line to not be wrapped. + + // Verify that we marked the 0th row as _not wrapped_ + const auto& row0 = tb.GetRowByOffset(0); + VERIFY_ARE_EQUAL(isTerminal, row0.GetCharRow().WasWrapForced()); + + const auto& row1 = tb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(tb, LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)", { 0, 0 }); + TestUtils::VerifyExpectedString(tb, L" 1234567890", { 0, 1 }); + }; + + verifyBuffer(hostTb, false); + + // First write the first 80 characters from the string + expectedOutput.push_back(R"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnop)"); + + // This is the hard line break + expectedOutput.push_back("\r\n"); + // Now write row 2 of the buffer + expectedOutput.push_back(" 1234567890"); + // and clear everything after the text, because the buffer is empty. + expectedOutput.push_back("\x1b[K"); + VERIFY_SUCCEEDED(renderer.PaintFrame()); + + verifyBuffer(termTb, true); +} + void ConptyRoundtripTests::MoveCursorAtEOL() { // This is a test for GH#1245 @@ -360,7 +622,6 @@ void ConptyRoundtripTests::MoveCursorAtEOL() auto& hostSm = si.GetStateMachine(); auto& hostTb = si.GetTextBuffer(); auto& termTb = *term->_buffer; - _flushFirstFrame(); Log::Comment(NoThrowString().Format( diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index 57285c4e39..08788414c6 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -29,6 +29,9 @@ class TerminalCoreUnitTests::TerminalBufferTests final TEST_METHOD(TestSimpleBufferWriting); + TEST_METHOD(TestWrappingCharByChar); + TEST_METHOD(TestWrappingALongString); + TEST_METHOD_SETUP(MethodSetup) { // STEP 1: Set up the Terminal @@ -66,3 +69,76 @@ void TerminalBufferTests::TestSimpleBufferWriting() TestUtils::VerifyExpectedString(termTb, L"Hello World", { 0, 0 }); } + +void TerminalBufferTests::TestWrappingCharByChar() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + auto& cursor = termTb.GetCursor(); + + const auto charsToWrite = gsl::narrow_cast(TestUtils::Test100CharsString.size()); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); + + for (auto i = 0; i < charsToWrite; i++) + { + // This is a handy way of just printing the printable characters that + // _aren't_ the space character. + const wchar_t wch = static_cast(33 + (i % 94)); + termSm.ProcessCharacter(wch); + } + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(charsToWrite % initialView.Width(), cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = termTb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = termTb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(termTb, TestUtils::Test100CharsString, { 0, 0 }); +} + +void TerminalBufferTests::TestWrappingALongString() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + auto& cursor = termTb.GetCursor(); + + const auto charsToWrite = gsl::narrow_cast(TestUtils::Test100CharsString.size()); + VERIFY_ARE_EQUAL(100, charsToWrite); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(32, initialView.BottomExclusive()); + + termSm.ProcessString(TestUtils::Test100CharsString); + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, secondView.Top()); + VERIFY_ARE_EQUAL(32, secondView.BottomExclusive()); + + // Verify the cursor wrapped to the second line + VERIFY_ARE_EQUAL(charsToWrite % initialView.Width(), cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + // Verify that we marked the 0th row as _wrapped_ + const auto& row0 = termTb.GetRowByOffset(0); + VERIFY_IS_TRUE(row0.GetCharRow().WasWrapForced()); + + const auto& row1 = termTb.GetRowByOffset(1); + VERIFY_IS_FALSE(row1.GetCharRow().WasWrapForced()); + + TestUtils::VerifyExpectedString(termTb, TestUtils::Test100CharsString, { 0, 0 }); +} diff --git a/src/cascadia/UnitTests_TerminalCore/TestUtils.h b/src/cascadia/UnitTests_TerminalCore/TestUtils.h index eb91544592..f7ddcdac0d 100644 --- a/src/cascadia/UnitTests_TerminalCore/TestUtils.h +++ b/src/cascadia/UnitTests_TerminalCore/TestUtils.h @@ -21,6 +21,10 @@ namespace TerminalCoreUnitTests class TerminalCoreUnitTests::TestUtils { public: + static constexpr std::wstring_view Test100CharsString{ + LR"(!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~!"#$%&)" + }; + // Function Description: // - Helper function to validate that a number of characters in a row are all // the same. Validates that the next end-start characters are all equal to the diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 8b1ea38ee9..0e27578d4f 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -604,7 +604,7 @@ void VtRendererTest::Xterm256TestCursor() clusters.emplace_back(std::wstring_view{ &line[i], 1 }, static_cast(rgWidths[i])); } - VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false)); + VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false, false)); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); VERIFY_SUCCEEDED(engine->_MoveCursor({ 10, 1 })); @@ -1020,7 +1020,7 @@ void VtRendererTest::XtermTestCursor() clusters.emplace_back(std::wstring_view{ &line[i], 1 }, static_cast(rgWidths[i])); } - VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false)); + VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false, false)); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); VERIFY_SUCCEEDED(engine->_MoveCursor({ 10, 1 })); @@ -1250,7 +1250,7 @@ void VtRendererTest::WinTelnetTestCursor() clusters.emplace_back(std::wstring_view{ &line[i], 1 }, static_cast(rgWidths[i])); } - VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false)); + VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters.data(), clusters.size() }, { 1, 1 }, false, false)); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); VERIFY_SUCCEEDED(engine->_MoveCursor({ 10, 1 })); @@ -1315,8 +1315,8 @@ void VtRendererTest::TestWrapping() clusters2.emplace_back(std::wstring_view{ &line2[i], 1 }, static_cast(rgWidths[i])); } - VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters1.data(), clusters1.size() }, { 0, 0 }, false)); - VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters2.data(), clusters2.size() }, { 0, 1 }, false)); + VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters1.data(), clusters1.size() }, { 0, 0 }, false, false)); + VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters2.data(), clusters2.size() }, { 0, 1 }, false, false)); }); } diff --git a/src/interactivity/onecore/BgfxEngine.cpp b/src/interactivity/onecore/BgfxEngine.cpp index 32d52760bd..11af29783c 100644 --- a/src/interactivity/onecore/BgfxEngine.cpp +++ b/src/interactivity/onecore/BgfxEngine.cpp @@ -147,7 +147,8 @@ BgfxEngine::BgfxEngine(PVOID SharedViewBase, LONG DisplayHeight, LONG DisplayWid [[nodiscard]] HRESULT BgfxEngine::PaintBufferLine(const std::basic_string_view clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/interactivity/onecore/BgfxEngine.hpp b/src/interactivity/onecore/BgfxEngine.hpp index 9ce46e3570..dcb20b9973 100644 --- a/src/interactivity/onecore/BgfxEngine.hpp +++ b/src/interactivity/onecore/BgfxEngine.hpp @@ -51,7 +51,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(const std::basic_string_view clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index e1b0b970f8..ae6a25ab36 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -597,15 +597,23 @@ void Renderer::_PaintBufferOutput(_In_ IRenderEngine* const pEngine) // Retrieve the cell information iterator limited to just this line we want to redraw. auto it = buffer.GetCellDataAt(bufferLine.Origin(), bufferLine); + // Calculate if two things are true: + // 1. this row wrapped + // 2. We're painting the last col of the row. + // In that case, set lineWrapped=true for the _PaintBufferOutputHelper call. + const auto lineWrapped = (buffer.GetRowByOffset(bufferLine.Origin().Y).GetCharRow().WasWrapForced()) && + (bufferLine.RightExclusive() == buffer.GetSize().Width()); + // Ask the helper to paint through this specific line. - _PaintBufferOutputHelper(pEngine, it, screenLine.Origin()); + _PaintBufferOutputHelper(pEngine, it, screenLine.Origin(), lineWrapped); } } } void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, TextBufferCellIterator it, - const COORD target) + const COORD target, + const bool lineWrapped) { // If we have valid data, let's figure out how to draw it. if (it) @@ -694,7 +702,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, } while (it); // Do the painting. - THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, trimLeft)); + THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, trimLeft, lineWrapped)); // If we're allowed to do grid drawing, draw that now too (since it will be coupled with the color data) if (_pData->IsGridLineDrawingAllowed()) @@ -843,7 +851,7 @@ void Renderer::_PaintOverlay(IRenderEngine& engine, auto it = overlay.buffer.GetCellLineDataAt(source); - _PaintBufferOutputHelper(&engine, it, target); + _PaintBufferOutputHelper(&engine, it, target, false); } } } diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index c77df20df2..3e02cc7aea 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -96,7 +96,8 @@ namespace Microsoft::Console::Render void _PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, TextBufferCellIterator it, - const COORD target); + const COORD target, + const bool lineWrapped); static IRenderEngine::GridLines s_GetGridlines(const TextAttribute& textAttribute) noexcept; diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index cb6baeb6fa..21898d5f74 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1220,7 +1220,8 @@ void DxEngine::_InvalidOr(RECT rc) noexcept // - S_OK or relevant DirectX error [[nodiscard]] HRESULT DxEngine::PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/dx/DxRenderer.hpp b/src/renderer/dx/DxRenderer.hpp index f0358d8149..c744bcf30e 100644 --- a/src/renderer/dx/DxRenderer.hpp +++ b/src/renderer/dx/DxRenderer.hpp @@ -76,7 +76,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - bool const fTrimLeft) noexcept override; + bool const fTrimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/gdi/gdirenderer.hpp b/src/renderer/gdi/gdirenderer.hpp index 4840a18198..1fb1c09b05 100644 --- a/src/renderer/gdi/gdirenderer.hpp +++ b/src/renderer/gdi/gdirenderer.hpp @@ -44,7 +44,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp index bbce43702a..a6683085ea 100644 --- a/src/renderer/gdi/paint.cpp +++ b/src/renderer/gdi/paint.cpp @@ -286,7 +286,8 @@ using namespace Microsoft::Console::Render; //#define MAX_POLY_LINES 80 [[nodiscard]] HRESULT GdiEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept + const bool trimLeft, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/inc/IRenderEngine.hpp b/src/renderer/inc/IRenderEngine.hpp index aff56a5bb5..455b6e8bb2 100644 --- a/src/renderer/inc/IRenderEngine.hpp +++ b/src/renderer/inc/IRenderEngine.hpp @@ -93,7 +93,8 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT PaintBackground() noexcept = 0; [[nodiscard]] virtual HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool fTrimLeft) noexcept = 0; + const bool fTrimLeft, + const bool lineWrapped) noexcept = 0; [[nodiscard]] virtual HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp index 1d919b9980..f63ccd3db2 100644 --- a/src/renderer/uia/UiaRenderer.cpp +++ b/src/renderer/uia/UiaRenderer.cpp @@ -276,7 +276,8 @@ UiaEngine::UiaEngine(IUiaEventDispatcher* dispatcher) : // - S_FALSE [[nodiscard]] HRESULT UiaEngine::PaintBufferLine(std::basic_string_view const /*clusters*/, COORD const /*coord*/, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { return S_FALSE; } diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp index 056a63b00a..4dfbdccce4 100644 --- a/src/renderer/uia/UiaRenderer.hpp +++ b/src/renderer/uia/UiaRenderer.hpp @@ -53,7 +53,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, COORD const coord, - bool const fTrimLeft) noexcept override; + bool const fTrimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index dfbe434f2d..3a8491c720 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -19,7 +19,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _ColorTable(ColorTable), _cColorTable(cColorTable), _fUseAsciiOnly(fUseAsciiOnly), - _previousLineWrapped(false), _usingUnderLine(false), _needToDisableCursor(false), _lastCursorIsVisible(false), @@ -235,6 +234,8 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { HRESULT hr = S_OK; + _trace.TraceMoveCursor(_lastText, coord); + if (coord.X != _lastText.X || coord.Y != _lastText.Y) { if (coord.X == 0 && coord.Y == 0) @@ -248,8 +249,15 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // If the previous line wrapped, then the cursor is already at this // position, we just don't know it yet. Don't emit anything. - if (_previousLineWrapped) + bool previousLineWrapped = false; + if (_wrappedRow.has_value()) { + previousLineWrapped = coord.Y == _wrappedRow.value() + 1; + } + + if (previousLineWrapped) + { + _trace.TraceWrapped(); hr = S_OK; } else @@ -312,7 +320,11 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _newBottomLine = false; } _deferredCursorPos = INVALID_COORDS; + + _wrappedRow = std::nullopt; + _delayedEolWrap = false; + return hr; } @@ -430,15 +442,19 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - trimLeft - This specifies whether to trim one character width off the left // side of the output. Used for drawing the right-half only of a // double-wide character. +// - lineWrapped: true if this run we're painting is the end of a line that +// wrapped. If we're not painting the last column of a wrapped line, then this +// will be false. // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT XtermEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool lineWrapped) noexcept { return _fUseAsciiOnly ? VtEngine::_PaintAsciiBufferLine(clusters, coord) : - VtEngine::_PaintUtf8BufferLine(clusters, coord); + VtEngine::_PaintUtf8BufferLine(clusters, coord, lineWrapped); } // Method Description: diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 28bdfc4408..e6a0a868c1 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -48,7 +48,8 @@ namespace Microsoft::Console::Render const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT ScrollFrame() noexcept override; [[nodiscard]] HRESULT InvalidateScroll(const COORD* const pcoordDelta) noexcept override; @@ -59,7 +60,6 @@ namespace Microsoft::Console::Render const COLORREF* const _ColorTable; const WORD _cColorTable; const bool _fUseAsciiOnly; - bool _previousLineWrapped; bool _usingUnderLine; bool _needToDisableCursor; bool _lastCursorIsVisible; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 1c20a85a1a..1ca2559423 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -115,11 +115,15 @@ using namespace Microsoft::Console::Types; // - trimLeft - This specifies whether to trim one character width off the left // side of the output. Used for drawing the right-half only of a // double-wide character. +// - lineWrapped: true if this run we're painting is the end of a line that +// wrapped. If we're not painting the last column of a wrapped line, then this +// will be false. // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { return VtEngine::_PaintAsciiBufferLine(clusters, coord); } @@ -149,6 +153,8 @@ using namespace Microsoft::Console::Types; // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::PaintCursor(const IRenderEngine::CursorOptions& options) noexcept { + _trace.TracePaintCursor(options.coordCursor); + // MSFT:15933349 - Send the terminal the updated cursor information, if it's changed. LOG_IF_FAILED(_MoveCursor(options.coordCursor)); @@ -369,15 +375,14 @@ using namespace Microsoft::Console::Types; // Return Value: // - S_OK or suitable HRESULT error from writing pipe. [[nodiscard]] HRESULT VtEngine::_PaintUtf8BufferLine(std::basic_string_view const clusters, - const COORD coord) noexcept + const COORD coord, + const bool lineWrapped) noexcept { if (coord.Y < _virtualTop) { return S_OK; } - RETURN_IF_FAILED(_MoveCursor(coord)); - std::wstring unclusteredString; unclusteredString.reserve(clusters.size()); short totalWidth = 0; @@ -445,10 +450,37 @@ using namespace Microsoft::Console::Types; (totalWidth - numSpaces) : totalWidth; + if (cchActual == 0) + { + // If the previous row wrapped, but this line is empty, then we actually + // do want to move the cursor down. Otherwise, we'll possibly end up + // accidentally erasing the last character from the previous line, as + // the cursor is still waiting on that character for the next character + // to follow it. + _wrappedRow = std::nullopt; + } + + // Move the cursor to the start of this run. + RETURN_IF_FAILED(_MoveCursor(coord)); + // Write the actual text string std::wstring wstr = std::wstring(unclusteredString.data(), cchActual); RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); + // If we've written text to the last column of the viewport, then mark + // that we've wrapped this line. The next time we attempt to move the + // cursor, if we're trying to move it to the start of the next line, + // we'll remember that this line was wrapped, and not manually break the + // line. + // Don't do this if the last character we're writing is a space - The last + // char will always be a space, but if we see that, we shouldn't wrap. + const short lastWrittenChar = base::ClampAdd(_lastText.X, base::ClampSub(totalWidth, numSpaces)); + if (lineWrapped && + lastWrittenChar > _lastViewport.RightInclusive()) + { + _wrappedRow = coord.Y; + } + // Update our internal tracker of the cursor's position. // See MSFT:20266233 (which is also GH#357) // If the cursor is at the rightmost column of the terminal, and we write a diff --git a/src/renderer/vt/tracing.cpp b/src/renderer/vt/tracing.cpp index 805835f135..8246e6d920 100644 --- a/src/renderer/vt/tracing.cpp +++ b/src/renderer/vt/tracing.cpp @@ -225,3 +225,48 @@ void RenderTracing::TraceLastText(const COORD lastTextPos) const UNREFERENCED_PARAMETER(lastTextPos); #endif UNIT_TESTING } +void RenderTracing::TraceMoveCursor(const COORD lastTextPos, const COORD cursor) const +{ +#ifndef UNIT_TESTING + const auto lastTextStr = _CoordToString(lastTextPos); + const auto lastText = lastTextStr.c_str(); + + const auto cursorStr = _CoordToString(cursor); + const auto cursorPos = cursorStr.c_str(); + + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceMoveCursor", + TraceLoggingString(lastText), + TraceLoggingString(cursorPos), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(lastTextPos); + UNREFERENCED_PARAMETER(cursor); +#endif UNIT_TESTING +} + +void RenderTracing::TraceWrapped() const +{ +#ifndef UNIT_TESTING + const auto* const msg = "Wrapped instead of \\r\\n"; + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TraceWrapped", + TraceLoggingString(msg), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else +#endif UNIT_TESTING +} + +void RenderTracing::TracePaintCursor(const COORD coordCursor) const +{ +#ifndef UNIT_TESTING + const auto cursorPosString = _CoordToString(coordCursor); + const auto cursorPos = cursorPosString.c_str(); + TraceLoggingWrite(g_hConsoleVtRendererTraceProvider, + "VtEngine_TracePaintCursor", + TraceLoggingString(cursorPos), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); +#else + UNREFERENCED_PARAMETER(coordCursor); +#endif UNIT_TESTING +} diff --git a/src/renderer/vt/tracing.hpp b/src/renderer/vt/tracing.hpp index 8d923adc81..0fcd1a97aa 100644 --- a/src/renderer/vt/tracing.hpp +++ b/src/renderer/vt/tracing.hpp @@ -29,6 +29,9 @@ namespace Microsoft::Console::VirtualTerminal void TraceString(const std::string_view& str) const; void TraceInvalidate(const Microsoft::Console::Types::Viewport view) const; void TraceLastText(const COORD lastText) const; + void TraceMoveCursor(const COORD lastText, const COORD cursor) const; + void TraceWrapped() const; + void TracePaintCursor(const COORD coordCursor) const; void TraceInvalidateAll(const Microsoft::Console::Types::Viewport view) const; void TraceTriggerCircling(const bool newFrame) const; void TraceStartPaint(const bool quickReturn, diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 5e9cdb1e71..b3a4c47dea 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -65,7 +65,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] virtual HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(const GridLines lines, const COLORREF color, const size_t cchLine, @@ -144,6 +145,8 @@ namespace Microsoft::Console::Render Microsoft::Console::VirtualTerminal::RenderTracing _trace; bool _inResizeRequest{ false }; + std::optional _wrappedRow{ std::nullopt }; + bool _delayedEolWrap{ false }; [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; @@ -214,7 +217,8 @@ namespace Microsoft::Console::Render bool _WillWriteSingleChar() const; [[nodiscard]] HRESULT _PaintUtf8BufferLine(std::basic_string_view const clusters, - const COORD coord) noexcept; + const COORD coord, + const bool lineWrapped) noexcept; [[nodiscard]] HRESULT _PaintAsciiBufferLine(std::basic_string_view const clusters, const COORD coord) noexcept; diff --git a/src/renderer/wddmcon/WddmConRenderer.cpp b/src/renderer/wddmcon/WddmConRenderer.cpp index d9404f9dad..157309071c 100644 --- a/src/renderer/wddmcon/WddmConRenderer.cpp +++ b/src/renderer/wddmcon/WddmConRenderer.cpp @@ -260,7 +260,8 @@ bool WddmConEngine::IsInitialized() [[nodiscard]] HRESULT WddmConEngine::PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool /*trimLeft*/) noexcept + const bool /*trimLeft*/, + const bool /*lineWrapped*/) noexcept { try { diff --git a/src/renderer/wddmcon/WddmConRenderer.hpp b/src/renderer/wddmcon/WddmConRenderer.hpp index c214d585a8..70213ca95f 100644 --- a/src/renderer/wddmcon/WddmConRenderer.hpp +++ b/src/renderer/wddmcon/WddmConRenderer.hpp @@ -43,7 +43,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintBackground() noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, - const bool trimLeft) noexcept override; + const bool trimLeft, + const bool lineWrapped) noexcept override; [[nodiscard]] HRESULT PaintBufferGridLines(GridLines const lines, COLORREF const color, size_t const cchLine, COORD const coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; From 2f60cf0e9138f0d788f0a7bf189ee4116a701a4d Mon Sep 17 00:00:00 2001 From: greg904 <56923875+greg904@users.noreply.github.com> Date: Thu, 27 Feb 2020 19:29:21 +0100 Subject: [PATCH 09/13] Fix RenderThread's notify mechanism (#4698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fix a bug where the `Renderer::PaintFrame` method: 1. is not called until the next `RenderThread::NotifyThread` call but needs to be called because there the terminal was updated (theoretical bug) 2. is called twice but needs to be called only once (verified bug) ## References The bug was introduced by #3511. ## PR Checklist * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA ## Detailed Description of the Pull Request / Additional comments ### Before #### First bug In the original code, `_fNextFrameRequested` is set to `true` in render thread because `std::atomic_flag::test_and_set` is called. This is wrong because it means that the render thread will render the terminal again even if there is no change after the last render. I think the the goal was to load the boolean value for `_fNextFrameRequested` to check whether the thread should sleep or not. The problem is that there is no method on `std::atomic_flag` to load its boolean value. I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value. I guess that this was believed to be equivalent to just a simple load, without doing any change to the value because it restores it at the end. But it's not: this is dangerous because if the value is changed to `true` between the call to `std::atomic_flag::test_and_set` and the call to `std::atomic_flag::clear`, then the value ends up being `false` at the end which is wrong because we don't want to change it! And if that value ends up being `false`, it means that we miss a render because we will wait on `_hEvent` during the next iteration on the render thread. Well actually, here, this not even a problem because when that code is ran, `_fPainting` is `false` which means that the other thread that modifies the `_fNextFrameRequested` value through `RenderThread::NotifyPaint` will not actually modify `_fNextFrameRequested` but rather call `SetEvent` (see the method's body). But wait! There is a problem there too! `std::atomic_flag::test_and_set` is called for `_fPainting` which sets its value to `true`. It was probably unintended. So actually, the next call to `RenderThread::NotifyPaint` _will_ end up modifying `_fNextFrameRequested` which means that the data race I was talking about _might_ happen! #### Second bug Let's go back a little bit in my explanation. I was talking about the fact that: > I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value. The problem is that the reverse was done in the implementation: `std::atomic_flag::clear` is called if the value was _`true`_ originally! So at this point, if the value of `_fNextFrameRequested` was `false`, then `std::atomic_flag::test_and_set` sets its is set to `true` and returns `false`. So for the next iteration, `_fNextFrameRequested` is `true` and the render thread will re-render but that was not needed. ### After I used `std::atomic` instead of `std::atomic_flag` for `_fNextFrameRequested` and the other atomic field because it has a `load` and a `store` method so we can actually load the value without changing it. I also replaced `_fPainting` by `_fWaiting`, which is basically the opposite of `_fPainting` but stays `true` for a little shorter than `_fPainting` would stay `false`. Indeed, I think that it makes more sense to directly wrap/scope _just_ the call to `WaitForSingleObject` by setting my atomic variable to `true` _just_ before and to `false` _just_ after because: * It makes more sense while you're reading the code: it's easier IMO to understand what the purpose of `_fWaiting` is (that is, to call `SetEvent` from `RenderThread::NotifyPaint` if it's `true`). * It's probably a tiny bit better for performance because it will become `true` for a little shorter which means less calls to `SetEvent`. #### Warning I don't really understand [std::memory_order](https://en.cppreference.com/w/cpp/atomic/memory_order)s. So I used the default one (`std::memory_order_seq_cst`) which is the safest. I believe that if no read or write are reordered in the two threads (`RenderThread::NotifyPaint` and `RenderThread::_ThreadProc`), then the code I wrote will behave correctly. I think that `std::memory_order_seq_cst` enforces that so it should be fine, but I'm not sure. ## Validation Steps Performed **I tried to reproduce the second bug that I described in the first section of this PR.** I put a breakpoint on `RenderThread::NotifyPaint` and on `Renderer::PaintFrame`. Initially they are disabled. Then I ran the terminal in Release mode, waited a bit for the prompt to display and the cursor to start blinking. Then I enabled the breakpoints. ### Before Each `RenderThread::NotifyPaint` is followed by 2 `Renderer::PaintFrame` calls. ❌ ### After Each `RenderThread::NotifyPaint` is followed by 1 `Renderer::PaintFrame` call. ✔️ --- src/renderer/base/thread.cpp | 64 ++++++++++++++++++++++++------------ src/renderer/base/thread.hpp | 4 +-- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/src/renderer/base/thread.cpp b/src/renderer/base/thread.cpp index 99efd4b2e3..f05bba26c6 100644 --- a/src/renderer/base/thread.cpp +++ b/src/renderer/base/thread.cpp @@ -15,10 +15,10 @@ RenderThread::RenderThread() : _hEvent(nullptr), _hPaintCompletedEvent(nullptr), _fKeepRunning(true), - _hPaintEnabledEvent(nullptr) + _hPaintEnabledEvent(nullptr), + _fNextFrameRequested(false), + _fWaiting(false) { - _fNextFrameRequested.clear(); - _fPainting.clear(); } RenderThread::~RenderThread() @@ -161,20 +161,45 @@ DWORD WINAPI RenderThread::_ThreadProc() { WaitForSingleObject(_hPaintEnabledEvent, INFINITE); - // Skip waiting if next frame is requested. - if (_fNextFrameRequested.test_and_set(std::memory_order_relaxed)) + if (!_fNextFrameRequested.exchange(false)) { - _fNextFrameRequested.clear(std::memory_order_relaxed); - } - else - { - WaitForSingleObject(_hEvent, INFINITE); + // <-- + // If `NotifyPaint` is called at this point, then it will not + // set the event because `_fWaiting` is not `true` yet so we have + // to check again below. + + _fWaiting.store(true); + + // check again now (see comment above) + if (!_fNextFrameRequested.exchange(false)) + { + // Wait until a next frame is requested. + WaitForSingleObject(_hEvent, INFINITE); + } + + // <-- + // If `NotifyPaint` is called at this point, then it _will_ set + // the event because `_fWaiting` is `true`, but we're not waiting + // anymore! + // This can probably happen quite often: imagine a scenario where + // we are waiting, and the terminal calls `NotifyPaint` twice + // very quickly. + // In that case, both calls might end up calling `SetEvent`. The + // first one will resume this thread and the second one will + // `SetEvent` the event. So the next time we wait, the event will + // already be set and we won't actually wait. + // Because it can happen often, and because rendering is an + // expensive operation, we should reset the event to not render + // again if nothing changed. + + _fWaiting.store(false); + + // see comment above + ResetEvent(_hEvent); } ResetEvent(_hPaintCompletedEvent); - _fPainting.test_and_set(std::memory_order_acquire); - LOG_IF_FAILED(_pRenderer->PaintFrame()); SetEvent(_hPaintCompletedEvent); @@ -184,8 +209,6 @@ DWORD WINAPI RenderThread::_ThreadProc() { Sleep(s_FrameLimitMilliseconds); } - - _fPainting.clear(std::memory_order_release); } return S_OK; @@ -193,15 +216,14 @@ DWORD WINAPI RenderThread::_ThreadProc() void RenderThread::NotifyPaint() { - // If we are currently painting a frame, set _fNextFrameRequested flag - // to indicate we want to paint next frame immediately. - if (_fPainting.test_and_set(std::memory_order_acquire)) + if (_fWaiting.load()) { - _fNextFrameRequested.test_and_set(std::memory_order_relaxed); - return; + SetEvent(_hEvent); + } + else + { + _fNextFrameRequested.store(true); } - - SetEvent(_hEvent); } void RenderThread::EnablePainting() diff --git a/src/renderer/base/thread.hpp b/src/renderer/base/thread.hpp index 3e991f6183..4576bf1706 100644 --- a/src/renderer/base/thread.hpp +++ b/src/renderer/base/thread.hpp @@ -47,7 +47,7 @@ namespace Microsoft::Console::Render IRenderer* _pRenderer; // Non-ownership pointer bool _fKeepRunning; - std::atomic_flag _fNextFrameRequested; - std::atomic_flag _fPainting; + std::atomic _fNextFrameRequested; + std::atomic _fWaiting; }; } From a97048a79808f427eb20465c618c091d578939fb Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 27 Feb 2020 12:53:31 -0800 Subject: [PATCH 10/13] Accept String value for Keybindings's Keys (#4714) ## Summary of the Pull Request `keys` in `keybindings` now accepts a string value. This assumes that you wanted a keychord of size 1. The schema and user docs were properly updated too. This means that the following keybinding is now accepted in your profiles.json: ```json { "command": "copy", "keys": "ctrl+c" } ``` as opposed to... ```json { "command": "copy", "keys": [ "ctrl+c" ] } ``` ## PR Checklist * [X] Closes #4713 * [X] CLA signed. * [X] Tests added/passed * [X] Requires documentation to be updated ## Validation Steps Performed - [X] tested the new schema - [X] added test --- doc/cascadia/profiles.schema.json | 22 ++-- doc/user-docs/UsingJsonSettings.md | 23 +++- .../KeyBindingsTests.cpp | 25 +++++ .../AppKeyBindingsSerialization.cpp | 6 +- src/cascadia/TerminalApp/defaults.json | 100 +++++++++--------- 5 files changed, 115 insertions(+), 61 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 6826b0c2d5..9f586b1198 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -3,6 +3,10 @@ "$schema": "http://json-schema.org/draft-07/schema#", "title": "Microsoft's Windows Terminal Settings Profile Schema'", "definitions": { + "KeyChordSegment": { + "pattern": "^(?(ctrl|alt|shift)\\+?((ctrl|alt|shift)(?[^+\\s]+?)?(?<=[^+\\s])$", + "type": "string" + }, "Color": { "default": "#", "pattern": "^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$", @@ -244,12 +248,18 @@ }, "keys": { "description": "Defines the key combinations used to call the command.", - "items": { - "pattern": "^(?(ctrl|alt|shift)\\+?((ctrl|alt|shift)(?[^+\\s]+?)?(?<=[^+\\s])$", - "type": "string" - }, - "minItems": 1, - "type": "array" + "oneOf": [ + { + "$ref": "#/definitions/KeyChordSegment" + }, + { + "items": { + "$ref": "#/definitions/KeyChordSegment" + }, + "minItems": 1, + "type": "array" + } + ] } }, "required": [ diff --git a/doc/user-docs/UsingJsonSettings.md b/doc/user-docs/UsingJsonSettings.md index 5f74db8e1e..edfef6826c 100644 --- a/doc/user-docs/UsingJsonSettings.md +++ b/doc/user-docs/UsingJsonSettings.md @@ -54,8 +54,8 @@ object under a root property `"globals"`. This is an array of key chords and shortcuts to invoke various commands. Each command can have more than one key binding. -NOTE: Key bindings is a subfield of the global settings and -key bindings apply to all profiles in the same manner. +> 👉 **Note**: Key bindings is a subfield of the global settings and +> key bindings apply to all profiles in the same manner. For example, here's a sample of the default keybindings: @@ -69,9 +69,26 @@ For example, here's a sample of the default keybindings: // etc. ] } - ``` +You can also use a single key chord string as the value of `"keys"`. +It will be treated as a chord of length one. +This will allow you to simplify the above snippet as follows: + +```json +{ + "keybindings": + [ + { "command": "closePane", "keys": "ctrl+shift+w" }, + { "command": "copy", "keys": "ctrl+shift+c" }, + { "command": "newTab", "keys": "ctrl+shift+t" }, + // etc. + ] +} +``` + + + ### Unbinding keys If you ever come across a key binding that you're unhappy with, it's possible to diff --git a/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp index 42160d0b40..ba0c9e686f 100644 --- a/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp @@ -42,6 +42,8 @@ namespace TerminalAppLocalTests TEST_METHOD(TestArbitraryArgs); TEST_METHOD(TestSplitPaneArgs); + TEST_METHOD(TestStringOverload); + TEST_CLASS_SETUP(ClassSetup) { InitializeJsonReader(); @@ -458,4 +460,27 @@ namespace TerminalAppLocalTests } } + void KeyBindingsTests::TestStringOverload() + { + const std::string bindings0String{ R"([ + { "command": "copy", "keys": "ctrl+c" } + ])" }; + + const auto bindings0Json = VerifyParseSucceeded(bindings0String); + + auto appKeyBindings = winrt::make_self(); + VERIFY_IS_NOT_NULL(appKeyBindings); + VERIFY_ARE_EQUAL(0u, appKeyBindings->_keyShortcuts.size()); + appKeyBindings->LayerJson(bindings0Json); + VERIFY_ARE_EQUAL(1u, appKeyBindings->_keyShortcuts.size()); + + { + KeyChord kc{ true, false, false, static_cast('C') }; + auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_IS_TRUE(realArgs.TrimWhitespace()); + } + } } diff --git a/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp b/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp index 69f64d41a9..4fdb965e2e 100644 --- a/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp +++ b/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp @@ -441,11 +441,13 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V if (keys) { - if (!keys.isArray() || keys.size() != 1) + const auto validString = keys.isString(); + const auto validArray = keys.isArray() && keys.size() == 1; + if (!validString && !validArray) { continue; } - const auto keyChordString = winrt::to_hstring(keys[0].asString()); + const auto keyChordString = keys.isString() ? winrt::to_hstring(keys.asString()) : winrt::to_hstring(keys[0].asString()); // Invalid is our placeholder that the action was not parsed. ShortcutAction action = ShortcutAction::Invalid; diff --git a/src/cascadia/TerminalApp/defaults.json b/src/cascadia/TerminalApp/defaults.json index c1d76e3ebb..26e2e0fd3e 100644 --- a/src/cascadia/TerminalApp/defaults.json +++ b/src/cascadia/TerminalApp/defaults.json @@ -205,55 +205,55 @@ ], "keybindings": [ - { "command": "closePane", "keys": [ "ctrl+shift+w" ] }, - { "command": "closeWindow", "keys": [ "alt+f4" ] }, - { "command": "copy", "keys": [ "ctrl+shift+c" ] }, - { "command": "copy", "keys": [ "ctrl+insert" ] }, - { "command": "decreaseFontSize", "keys": [ "ctrl+-" ] }, - { "command": "duplicateTab", "keys": [ "ctrl+shift+d" ] }, - { "command": "increaseFontSize", "keys": [ "ctrl+=" ] }, - { "command": { "action": "moveFocus", "direction": "down" }, "keys": [ "alt+down" ] }, - { "command": { "action": "moveFocus", "direction": "left" }, "keys": [ "alt+left" ] }, - { "command": { "action": "moveFocus", "direction": "right" }, "keys": [ "alt+right" ] }, - { "command": { "action": "moveFocus", "direction": "up" }, "keys": [ "alt+up" ] }, - { "command": "newTab", "keys": [ "ctrl+shift+t" ] }, - { "command": { "action": "newTab", "index": 0 }, "keys": ["ctrl+shift+1"] }, - { "command": { "action": "newTab", "index": 1 }, "keys": ["ctrl+shift+2"] }, - { "command": { "action": "newTab", "index": 2 }, "keys": ["ctrl+shift+3"] }, - { "command": { "action": "newTab", "index": 3 }, "keys": ["ctrl+shift+4"] }, - { "command": { "action": "newTab", "index": 4 }, "keys": ["ctrl+shift+5"] }, - { "command": { "action": "newTab", "index": 5 }, "keys": ["ctrl+shift+6"] }, - { "command": { "action": "newTab", "index": 6 }, "keys": ["ctrl+shift+7"] }, - { "command": { "action": "newTab", "index": 7 }, "keys": ["ctrl+shift+8"] }, - { "command": { "action": "newTab", "index": 8 }, "keys": ["ctrl+shift+9"] }, - { "command": "nextTab", "keys": [ "ctrl+tab" ] }, - { "command": "openNewTabDropdown", "keys": [ "ctrl+shift+space" ] }, - { "command": "openSettings", "keys": [ "ctrl+," ] }, - { "command": "paste", "keys": [ "ctrl+shift+v" ] }, - { "command": "paste", "keys": [ "shift+insert" ] }, - { "command": "prevTab", "keys": [ "ctrl+shift+tab" ] }, - { "command": "resetFontSize", "keys": ["ctrl+0"]}, - { "command": { "action": "resizePane", "direction": "down" }, "keys": [ "alt+shift+down" ] }, - { "command": { "action": "resizePane", "direction": "left" }, "keys": [ "alt+shift+left" ] }, - { "command": { "action": "resizePane", "direction": "right" }, "keys": [ "alt+shift+right" ] }, - { "command": { "action": "resizePane", "direction": "up" }, "keys": [ "alt+shift+up" ] }, - { "command": "scrollDown", "keys": [ "ctrl+shift+down" ] }, - { "command": "scrollDownPage", "keys": [ "ctrl+shift+pgdn" ] }, - { "command": "scrollUp", "keys": [ "ctrl+shift+up" ] }, - { "command": "scrollUpPage", "keys": [ "ctrl+shift+pgup" ] }, - { "command": { "action": "splitPane", "split": "horizontal"}, "keys": [ "alt+shift+-" ] }, - { "command": { "action": "splitPane", "split": "vertical"}, "keys": [ "alt+shift+plus" ] }, - { "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] }, - { "command": { "action": "switchToTab", "index": 1 }, "keys": ["ctrl+alt+2"] }, - { "command": { "action": "switchToTab", "index": 2 }, "keys": ["ctrl+alt+3"] }, - { "command": { "action": "switchToTab", "index": 3 }, "keys": ["ctrl+alt+4"] }, - { "command": { "action": "switchToTab", "index": 4 }, "keys": ["ctrl+alt+5"] }, - { "command": { "action": "switchToTab", "index": 5 }, "keys": ["ctrl+alt+6"] }, - { "command": { "action": "switchToTab", "index": 6 }, "keys": ["ctrl+alt+7"] }, - { "command": { "action": "switchToTab", "index": 7 }, "keys": ["ctrl+alt+8"] }, - { "command": { "action": "switchToTab", "index": 8 }, "keys": ["ctrl+alt+9"] }, - { "command": "find", "keys": [ "ctrl+shift+f" ] }, - { "command": "toggleFullscreen", "keys": [ "alt+enter" ] }, - { "command": "toggleFullscreen", "keys": [ "f11" ] } + { "command": "closePane", "keys": "ctrl+shift+w" }, + { "command": "closeWindow", "keys": "alt+f4" }, + { "command": "copy", "keys": "ctrl+shift+c" }, + { "command": "copy", "keys": "ctrl+insert" }, + { "command": "decreaseFontSize", "keys": "ctrl+-" }, + { "command": "duplicateTab", "keys": "ctrl+shift+d" }, + { "command": "increaseFontSize", "keys": "ctrl+=" }, + { "command": { "action": "moveFocus", "direction": "down" }, "keys": "alt+down" }, + { "command": { "action": "moveFocus", "direction": "left" }, "keys": "alt+left" }, + { "command": { "action": "moveFocus", "direction": "right" }, "keys": "alt+right" }, + { "command": { "action": "moveFocus", "direction": "up" }, "keys": "alt+up" }, + { "command": "newTab", "keys": "ctrl+shift+t" }, + { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+shift+1" }, + { "command": { "action": "newTab", "index": 1 }, "keys": "ctrl+shift+2" }, + { "command": { "action": "newTab", "index": 2 }, "keys": "ctrl+shift+3" }, + { "command": { "action": "newTab", "index": 3 }, "keys": "ctrl+shift+4" }, + { "command": { "action": "newTab", "index": 4 }, "keys": "ctrl+shift+5" }, + { "command": { "action": "newTab", "index": 5 }, "keys": "ctrl+shift+6" }, + { "command": { "action": "newTab", "index": 6 }, "keys": "ctrl+shift+7" }, + { "command": { "action": "newTab", "index": 7 }, "keys": "ctrl+shift+8" }, + { "command": { "action": "newTab", "index": 8 }, "keys": "ctrl+shift+9" }, + { "command": "nextTab", "keys": "ctrl+tab" }, + { "command": "openNewTabDropdown", "keys": "ctrl+shift+space" }, + { "command": "openSettings", "keys": "ctrl+," }, + { "command": "paste", "keys": "ctrl+shift+v" }, + { "command": "paste", "keys": "shift+insert" }, + { "command": "prevTab", "keys": "ctrl+shift+tab" }, + { "command": "resetFontSize", "keys": "ctrl+0" }, + { "command": { "action": "resizePane", "direction": "down" }, "keys": "alt+shift+down" }, + { "command": { "action": "resizePane", "direction": "left" }, "keys": "alt+shift+left" }, + { "command": { "action": "resizePane", "direction": "right" }, "keys": "alt+shift+right" }, + { "command": { "action": "resizePane", "direction": "up" }, "keys": "alt+shift+up" }, + { "command": "scrollDown", "keys": "ctrl+shift+down" }, + { "command": "scrollDownPage", "keys": "ctrl+shift+pgdn" }, + { "command": "scrollUp", "keys": "ctrl+shift+up" }, + { "command": "scrollUpPage", "keys": "ctrl+shift+pgup" }, + { "command": { "action": "splitPane", "split": "horizontal"}, "keys": "alt+shift+-" }, + { "command": { "action": "splitPane", "split": "vertical"}, "keys": "alt+shift+plus" }, + { "command": { "action": "switchToTab", "index": 0 }, "keys": "ctrl+alt+1" }, + { "command": { "action": "switchToTab", "index": 1 }, "keys": "ctrl+alt+2" }, + { "command": { "action": "switchToTab", "index": 2 }, "keys": "ctrl+alt+3" }, + { "command": { "action": "switchToTab", "index": 3 }, "keys": "ctrl+alt+4" }, + { "command": { "action": "switchToTab", "index": 4 }, "keys": "ctrl+alt+5" }, + { "command": { "action": "switchToTab", "index": 5 }, "keys": "ctrl+alt+6" }, + { "command": { "action": "switchToTab", "index": 6 }, "keys": "ctrl+alt+7" }, + { "command": { "action": "switchToTab", "index": 7 }, "keys": "ctrl+alt+8" }, + { "command": { "action": "switchToTab", "index": 8 }, "keys": "ctrl+alt+9" }, + { "command": "find", "keys": "ctrl+shift+f" }, + { "command": "toggleFullscreen", "keys": "alt+enter" }, + { "command": "toggleFullscreen", "keys": "f11" } ] } From 74cd9db383cc6de6d6ab29543944f6920e0bb70d Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 27 Feb 2020 16:37:56 -0800 Subject: [PATCH 11/13] Define Automation Properties for TermControl (#4732) Defines the following automation properties for a Terminal Control: - [**Orientation**](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.getorientationcore): - The orientation of the control - None --> Vertical - [**Name**](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.getnamecore): - The name as used by assistive technology and other Microsoft UI Automation clients. Generally presented by automation clients as the primary way to identify an element (along with the control type) - "" --> - [**HelpText**](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.gethelptextcore): - The help text. Generally presented by automation clients if requested by the user. This would be something that you would normally expect to appear from tooltips. - "" --> - [**LiveSetting**](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.automation.peers.automationpeer.getlivesettingcore): - reports the live setting notification behavior. A representation of how assertive this control should be when content changes. - none --> Polite ## Detailed Description of the Pull Request / Additional comments ProfileName had to be added to the TerminalSettings (IControlSettings) to pass that information along to the automation peer. In the rare event that somebody purposefully decided to make their ProfileName empty, we fallback to the tab title. ## Validation Steps Performed Verified using Accessibility Insights and inspect.exe This is are some examples of the information a general user can expect to receive about a Terminal Control. - Type: Terminal Control - Name: Command Prompt - Help Text (if requested): Command Prompt - ping bing.com - Type: Terminal Control - Name: Ubuntu - Help Text (if requested): cazamor@PC-cazamor:/mnt/c/Users/cazamor$ Note, it is generally read by an automation client as follows: ", " References #2099 - Automation Properties for TerminalControl, Search Box References #2142 - Localization Closes #2142 --- src/cascadia/TerminalApp/Profile.cpp | 1 + .../Resources/Resources.language-en.resw | 97 +++++++++++-------- .../TerminalControl/SearchBoxControl.xaml | 39 +++++--- src/cascadia/TerminalControl/TermControl.cpp | 5 + src/cascadia/TerminalControl/TermControl.h | 1 + .../TermControlAutomationPeer.cpp | 48 ++++++--- .../TermControlAutomationPeer.h | 20 ++-- .../TerminalControl.vcxproj.filters | 8 +- .../TerminalSettings/IControlSettings.idl | 2 + .../TerminalSettings/TerminalSettings.cpp | 11 +++ .../TerminalSettings/terminalsettings.h | 3 + 11 files changed, 166 insertions(+), 69 deletions(-) diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index 28a9897fc7..c69f7759dc 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -183,6 +183,7 @@ TerminalSettings Profile::CreateTerminalSettings(const std::unordered_map - @@ -117,27 +117,48 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - + Match Case - The tooltip text for CaseSensitivityButton + The tooltip text for the case sensitivity button on the search box control. - + Close - The tooltip text for CloseButton + The tooltip text for the close button on the search box control. - + Find Up - The tooltip text for GoBackward Button + The tooltip text for the search backward button. - + Find Down - The tooltip text for GoForward Button + The tooltip text for the search forward button. - + Find... - The placeholder text in the search dialog TextBox + The placeholder text in the search box control. Copy path to file + The displayed caption for dragging a file onto a terminal. - + + Case Sensitivity + The name of the case sensitivity button on the search box control for accessibility. + + + Search Forward + The name of the search forward button for accessibility. + + + Search Backward + The name of the search backward button for accessibility. + + + Search Text + The name of the text box on the search box control for accessibility. + + + terminal + The type of control that the terminal ahderes to. Used to identify how a user can interact with this kind of control. + + \ No newline at end of file diff --git a/src/cascadia/TerminalControl/SearchBoxControl.xaml b/src/cascadia/TerminalControl/SearchBoxControl.xaml index 08a0bd5ef4..2bf7132645 100644 --- a/src/cascadia/TerminalControl/SearchBoxControl.xaml +++ b/src/cascadia/TerminalControl/SearchBoxControl.xaml @@ -153,29 +153,44 @@ - + - + - - + - diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index d141855dc5..72bb372c79 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1606,6 +1606,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return hstr; } + hstring TermControl::GetProfileName() const + { + return _settings.ProfileName(); + } + // Method Description: // - Given a copy-able selection, get the selected text from the buffer and send it to the // Windows Clipboard (CascadiaWin32:main.cpp). diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index ffa7fb6797..0fcc63cde4 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -61,6 +61,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation winrt::fire_and_forget UpdateSettings(Settings::IControlSettings newSettings); hstring Title(); + hstring GetProfileName() const; bool CopySelectionToClipboard(bool trimTrailingWhitespace); void PasteTextFromClipboard(); diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp index 27a7b88557..f50ba2ac6d 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include +#include #include "TermControlAutomationPeer.h" #include "TermControl.h" #include "TermControlAutomationPeer.g.cpp" @@ -77,7 +78,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation }); } - winrt::hstring TermControlAutomationPeer::GetClassNameCore() const + hstring TermControlAutomationPeer::GetClassNameCore() const { return L"TermControl"; } @@ -87,13 +88,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return AutomationControlType::Text; } - winrt::hstring TermControlAutomationPeer::GetLocalizedControlTypeCore() const + hstring TermControlAutomationPeer::GetLocalizedControlTypeCore() const { - // TODO GitHub #2142: Localize string - return L"TerminalControl"; + return RS_(L"TerminalControl_ControlType"); } - winrt::Windows::Foundation::IInspectable TermControlAutomationPeer::GetPatternCore(PatternInterface patternInterface) const + Windows::Foundation::IInspectable TermControlAutomationPeer::GetPatternCore(PatternInterface patternInterface) const { switch (patternInterface) { @@ -105,15 +105,41 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } } + AutomationOrientation TermControlAutomationPeer::GetOrientationCore() const + { + return AutomationOrientation::Vertical; + } + + hstring TermControlAutomationPeer::GetNameCore() const + { + // fallback to title if profile name is empty + auto profileName = _termControl->GetProfileName(); + if (profileName.empty()) + { + return _termControl->Title(); + } + return profileName; + } + + hstring TermControlAutomationPeer::GetHelpTextCore() const + { + return _termControl->Title(); + } + + AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const + { + return AutomationLiveSetting::Polite; + } + #pragma region ITextProvider - winrt::com_array TermControlAutomationPeer::GetSelection() + com_array TermControlAutomationPeer::GetSelection() { SAFEARRAY* pReturnVal; THROW_IF_FAILED(_uiaProvider->GetSelection(&pReturnVal)); return WrapArrayOfTextRangeProviders(pReturnVal); } - winrt::com_array TermControlAutomationPeer::GetVisibleRanges() + com_array TermControlAutomationPeer::GetVisibleRanges() { SAFEARRAY* pReturnVal; THROW_IF_FAILED(_uiaProvider->GetVisibleRanges(&pReturnVal)); @@ -152,7 +178,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return xutr.as(); } - Windows::UI::Xaml::Automation::SupportedTextSelection TermControlAutomationPeer::SupportedTextSelection() + XamlAutomation::SupportedTextSelection TermControlAutomationPeer::SupportedTextSelection() { UIA::SupportedTextSelection returnVal; THROW_IF_FAILED(_uiaProvider->get_SupportedTextSelection(&returnVal)); @@ -214,7 +240,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // - SAFEARRAY of UIA::UiaTextRange (ITextRangeProviders) // Return Value: // - com_array of Xaml Wrapped UiaTextRange (ITextRangeProviders) - winrt::com_array TermControlAutomationPeer::WrapArrayOfTextRangeProviders(SAFEARRAY* textRanges) + com_array TermControlAutomationPeer::WrapArrayOfTextRangeProviders(SAFEARRAY* textRanges) { // transfer ownership of UiaTextRanges to this new vector auto providers = SafeArrayToOwningVector<::Microsoft::Terminal::TermControlUiaTextRange>(textRanges); @@ -225,11 +251,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto parentProvider = this->ProviderFromPeer(*this); for (int i = 0; i < count; i++) { - auto xutr = winrt::make_self(providers[i].detach(), parentProvider); + auto xutr = make_self(providers[i].detach(), parentProvider); vec.emplace_back(xutr.as()); } - winrt::com_array result{ vec }; + com_array result{ vec }; return result; } diff --git a/src/cascadia/TerminalControl/TermControlAutomationPeer.h b/src/cascadia/TerminalControl/TermControlAutomationPeer.h index bf3a95b360..56e12e56b4 100644 --- a/src/cascadia/TerminalControl/TermControlAutomationPeer.h +++ b/src/cascadia/TerminalControl/TermControlAutomationPeer.h @@ -39,12 +39,18 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation ::Microsoft::Console::Types::IControlAccessibilityInfo { public: - TermControlAutomationPeer(winrt::Microsoft::Terminal::TerminalControl::implementation::TermControl* owner); + TermControlAutomationPeer(Microsoft::Terminal::TerminalControl::implementation::TermControl* owner); - winrt::hstring GetClassNameCore() const; - winrt::Windows::UI::Xaml::Automation::Peers::AutomationControlType GetAutomationControlTypeCore() const; - winrt::hstring GetLocalizedControlTypeCore() const; - winrt::Windows::Foundation::IInspectable GetPatternCore(winrt::Windows::UI::Xaml::Automation::Peers::PatternInterface patternInterface) const; +#pragma region FrameworkElementAutomationPeer + hstring GetClassNameCore() const; + Windows::UI::Xaml::Automation::Peers::AutomationControlType GetAutomationControlTypeCore() const; + hstring GetLocalizedControlTypeCore() const; + Windows::Foundation::IInspectable GetPatternCore(Windows::UI::Xaml::Automation::Peers::PatternInterface patternInterface) const; + Windows::UI::Xaml::Automation::Peers::AutomationOrientation GetOrientationCore() const; + hstring GetNameCore() const; + hstring GetHelpTextCore() const; + Windows::UI::Xaml::Automation::Peers::AutomationLiveSetting GetLiveSettingCore() const; +#pragma endregion #pragma region IUiaEventDispatcher void SignalSelectionChanged() override; @@ -55,8 +61,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation #pragma region ITextProvider Pattern Windows::UI::Xaml::Automation::Provider::ITextRangeProvider RangeFromPoint(Windows::Foundation::Point screenLocation); Windows::UI::Xaml::Automation::Provider::ITextRangeProvider RangeFromChild(Windows::UI::Xaml::Automation::Provider::IRawElementProviderSimple childElement); - winrt::com_array GetVisibleRanges(); - winrt::com_array GetSelection(); + com_array GetVisibleRanges(); + com_array GetSelection(); Windows::UI::Xaml::Automation::SupportedTextSelection SupportedTextSelection(); Windows::UI::Xaml::Automation::Provider::ITextRangeProvider DocumentRange(); #pragma endregion diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters index 3a9cb6eab6..d90f6304ca 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters @@ -18,6 +18,7 @@ + @@ -26,12 +27,12 @@ - + @@ -43,4 +44,9 @@ + + + Resources + + diff --git a/src/cascadia/TerminalSettings/IControlSettings.idl b/src/cascadia/TerminalSettings/IControlSettings.idl index afb6b2e45d..12400ad9d9 100644 --- a/src/cascadia/TerminalSettings/IControlSettings.idl +++ b/src/cascadia/TerminalSettings/IControlSettings.idl @@ -26,6 +26,8 @@ namespace Microsoft.Terminal.Settings // for specifically the control. interface IControlSettings requires Microsoft.Terminal.Settings.ICoreSettings { + String ProfileName; + Boolean UseAcrylic; Double TintOpacity; ScrollbarState ScrollState; diff --git a/src/cascadia/TerminalSettings/TerminalSettings.cpp b/src/cascadia/TerminalSettings/TerminalSettings.cpp index d20eb352de..46fe1b45d3 100644 --- a/src/cascadia/TerminalSettings/TerminalSettings.cpp +++ b/src/cascadia/TerminalSettings/TerminalSettings.cpp @@ -28,6 +28,7 @@ namespace winrt::Microsoft::Terminal::Settings::implementation _cursorHeight{ DEFAULT_CURSOR_HEIGHT }, _wordDelimiters{ DEFAULT_WORD_DELIMITERS }, _copyOnSelect{ false }, + _profileName{}, _useAcrylic{ false }, _tintOpacity{ 0.5 }, _padding{ DEFAULT_PADDING }, @@ -196,6 +197,16 @@ namespace winrt::Microsoft::Terminal::Settings::implementation _copyOnSelect = value; } + void TerminalSettings::ProfileName(hstring const& value) + { + _profileName = value; + } + + hstring TerminalSettings::ProfileName() + { + return _profileName; + } + bool TerminalSettings::UseAcrylic() noexcept { return _useAcrylic; diff --git a/src/cascadia/TerminalSettings/terminalsettings.h b/src/cascadia/TerminalSettings/terminalsettings.h index a4b4098f16..e78fa00901 100644 --- a/src/cascadia/TerminalSettings/terminalsettings.h +++ b/src/cascadia/TerminalSettings/terminalsettings.h @@ -55,6 +55,8 @@ namespace winrt::Microsoft::Terminal::Settings::implementation void CopyOnSelect(bool value) noexcept; // ------------------------ End of Core Settings ----------------------- + hstring ProfileName(); + void ProfileName(hstring const& value); bool UseAcrylic() noexcept; void UseAcrylic(bool value) noexcept; double TintOpacity() noexcept; @@ -120,6 +122,7 @@ namespace winrt::Microsoft::Terminal::Settings::implementation uint32_t _cursorHeight; hstring _wordDelimiters; + hstring _profileName; bool _useAcrylic; double _tintOpacity; hstring _fontFace; From 0e672fac08d6316f3d69c9a1b2d9ab88aa4932ef Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 27 Feb 2020 16:42:26 -0800 Subject: [PATCH 12/13] Move rect expansion to textbuffer; refactor selection code (#4560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - When performing chunk selection, the expansion now occurs at the time of the selection, not the rendering of the selection - `GetSelectionRects()` was moved to the `TextBuffer` and is now shared between ConHost and Windows Terminal - Some of the selection variables were renamed for clarity - Selection COORDs are now in the Text Buffer coordinate space - Fixes an issue with Shift+Click after performing a Multi-Click Selection ## References This also contributes to... - #4509: UIA Box Selection - #2447: UIA Signaling for Selection - #1354: UIA support for Wide Glyphs Now that the expansion occurs at before render-time, the selection anchors are an accurate representation of what is selected. We just need to move `GetText` to the `TextBuffer`. Then we can have those three issues just rely on code from the text buffer. This also means ConHost gets some of this stuff for free 😀 ### TextBuffer - `GetTextRects` is the abstracted form of `GetSelectionRects` - `_ExpandTextRow` is still needed to handle wide glyphs properly ### Terminal - Rename... - `_boxSelection` --> `_blockSelection` for consistency with ConHost - `_selectionAnchor` --> `_selectionStart` for consistency with UIA - `_endSelectionPosition` --> `_selectionEnd` for consistency with UIA - Selection anchors are in Text Buffer coordinates now - Really rely on `SetSelectionEnd` to accomplish appropriate chunk selection and shift+click actions ## Validation Steps Performed - Shift+Click - Multi-Click --> Shift+Click - Chunk Selection at... - top of buffer - bottom of buffer - random region in scrollback Closes #4465 Closes #4547 --- src/buffer/out/textBuffer.cpp | 95 +++++ src/buffer/out/textBuffer.hpp | 4 + .../PublicTerminalCore/HwndTerminal.cpp | 4 +- src/cascadia/TerminalControl/TermControl.cpp | 12 +- src/cascadia/TerminalCore/Terminal.cpp | 8 +- src/cascadia/TerminalCore/Terminal.hpp | 43 +- .../TerminalCore/TerminalSelection.cpp | 401 ++++++------------ .../TerminalCore/terminalrenderdata.cpp | 2 +- .../UnitTests_TerminalCore/SelectionTest.cpp | 62 +-- src/host/renderData.cpp | 2 +- src/host/renderData.hpp | 2 +- src/host/selection.cpp | 184 +------- src/host/selection.hpp | 9 - src/host/ut_host/SelectionTests.cpp | 23 +- src/host/ut_host/TextBufferTests.cpp | 67 +++ .../win32/screenInfoUiaProvider.cpp | 2 +- src/types/IUiaData.h | 2 +- src/types/TermControlUiaProvider.cpp | 2 +- 18 files changed, 395 insertions(+), 529 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 68ff8d0b56..bff427d8fe 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1314,6 +1314,101 @@ TextBuffer::DelimiterClass TextBuffer::_GetDelimiterClass(const std::wstring_vie } } +// Method Description: +// - Determines the line-by-line rectangles based on two COORDs +// - expands the rectangles to support wide glyphs +// - used for selection rects and UIA bounding rects +// Arguments: +// - start: a corner of the text region of interest (inclusive) +// - end: the other corner of the text region of interest (inclusive) +// - blockSelection: when enabled, only get the rectangular text region, +// as opposed to the text extending to the left/right +// buffer margins +// Return Value: +// - the delimiter class for the given char +const std::vector TextBuffer::GetTextRects(COORD start, COORD end, bool blockSelection) const +{ + std::vector textRects; + + const auto bufferSize = GetSize(); + + // (0,0) is the top-left of the screen + // the physically "higher" coordinate is closer to the top-left + // the physically "lower" coordinate is closer to the bottom-right + const auto [higherCoord, lowerCoord] = bufferSize.CompareInBounds(start, end) <= 0 ? + std::make_tuple(start, end) : + std::make_tuple(end, start); + + const auto textRectSize = base::ClampedNumeric(1) + lowerCoord.Y - higherCoord.Y; + textRects.reserve(textRectSize); + for (auto row = higherCoord.Y; row <= lowerCoord.Y; row++) + { + SMALL_RECT textRow; + + textRow.Top = row; + textRow.Bottom = row; + + if (blockSelection || higherCoord.Y == lowerCoord.Y) + { + // set the left and right margin to the left-/right-most respectively + textRow.Left = std::min(higherCoord.X, lowerCoord.X); + textRow.Right = std::max(higherCoord.X, lowerCoord.X); + } + else + { + textRow.Left = (row == higherCoord.Y) ? higherCoord.X : bufferSize.Left(); + textRow.Right = (row == lowerCoord.Y) ? lowerCoord.X : bufferSize.RightInclusive(); + } + + _ExpandTextRow(textRow); + textRects.emplace_back(textRow); + } + + return textRects; +} + +// Method Description: +// - Expand the selection row according to include wide glyphs fully +// - this is particularly useful for box selections (ALT + selection) +// Arguments: +// - selectionRow: the selection row to be expanded +// Return Value: +// - modifies selectionRow's Left and Right values to expand properly +void TextBuffer::_ExpandTextRow(SMALL_RECT& textRow) const +{ + const auto bufferSize = GetSize(); + + // expand left side of rect + COORD targetPoint{ textRow.Left, textRow.Top }; + if (GetCellDataAt(targetPoint)->DbcsAttr().IsTrailing()) + { + if (targetPoint.X == bufferSize.Left()) + { + bufferSize.IncrementInBounds(targetPoint); + } + else + { + bufferSize.DecrementInBounds(targetPoint); + } + textRow.Left = targetPoint.X; + } + + // expand right side of rect + targetPoint = { textRow.Right, textRow.Bottom }; + if (GetCellDataAt(targetPoint)->DbcsAttr().IsLeading()) + { + if (targetPoint.X == bufferSize.RightInclusive()) + { + bufferSize.DecrementInBounds(targetPoint); + } + else + { + bufferSize.IncrementInBounds(targetPoint); + } + textRow.Right = targetPoint.X; + } +} + // Routine Description: // - Retrieves the text data from the selected region and presents it in a clipboard-ready format (given little post-processing). // Arguments: diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 7400efb353..3e870152d6 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -135,6 +135,8 @@ public: bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const; bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const; + const std::vector GetTextRects(COORD start, COORD end, bool blockSelection = false) const; + class TextAndColor { public: @@ -193,6 +195,8 @@ private: ROW& _GetFirstRow(); ROW& _GetPrevRowNoWrap(const ROW& row); + void _ExpandTextRow(SMALL_RECT& selectionRow) const; + enum class DelimiterClass { ControlChar, diff --git a/src/cascadia/PublicTerminalCore/HwndTerminal.cpp b/src/cascadia/PublicTerminalCore/HwndTerminal.cpp index 09d65c13fc..8c57493410 100644 --- a/src/cascadia/PublicTerminalCore/HwndTerminal.cpp +++ b/src/cascadia/PublicTerminalCore/HwndTerminal.cpp @@ -334,7 +334,7 @@ HRESULT _stdcall TerminalStartSelection(void* terminal, COORD cursorPosition, bo terminalPosition.Y /= fontSize.Y; publicTerminal->_terminal->SetSelectionAnchor(terminalPosition); - publicTerminal->_terminal->SetBoxSelection(altPressed); + publicTerminal->_terminal->SetBlockSelection(altPressed); publicTerminal->_renderer->TriggerSelection(); @@ -354,7 +354,7 @@ HRESULT _stdcall TerminalMoveSelection(void* terminal, COORD cursorPosition) terminalPosition.X /= fontSize.X; terminalPosition.Y /= fontSize.Y; - publicTerminal->_terminal->SetEndSelectionPosition(terminalPosition); + publicTerminal->_terminal->SetSelectionEnd(terminalPosition); publicTerminal->_renderer->TriggerSelection(); return S_OK; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 72bb372c79..42eeb355ea 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -142,7 +142,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto lock = _terminal->LockForWriting(); if (search.FindNext()) { - _terminal->SetBoxSelection(false); + _terminal->SetBlockSelection(false); search.Select(); _renderer->TriggerSelection(); } @@ -817,7 +817,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation const auto terminalPosition = _GetTerminalPosition(cursorPosition); // handle ALT key - _terminal->SetBoxSelection(altEnabled); + _terminal->SetBlockSelection(altEnabled); auto clickCount = _NumberOfClicks(cursorPosition, point.Timestamp()); @@ -828,17 +828,17 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (multiClickMapper == 3) { - _terminal->TripleClickSelection(terminalPosition); + _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Line); } else if (multiClickMapper == 2) { - _terminal->DoubleClickSelection(terminalPosition); + _terminal->MultiClickSelection(terminalPosition, ::Terminal::SelectionExpansionMode::Word); } else { if (shiftEnabled && _terminal->IsSelectionActive()) { - _terminal->SetEndSelectionPosition(terminalPosition); + _terminal->SetSelectionEnd(terminalPosition, ::Terminal::SelectionExpansionMode::Cell); } else { @@ -1474,7 +1474,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation terminalPosition.X = std::clamp(terminalPosition.X, 0, lastVisibleCol); // save location (for rendering) + render - _terminal->SetEndSelectionPosition(terminalPosition); + _terminal->SetSelectionEnd(terminalPosition); _renderer->TriggerSelection(); } diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index e4acabbdc9..df55fafdae 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -44,12 +44,10 @@ Terminal::Terminal() : _pfnWriteInput{ nullptr }, _scrollOffset{ 0 }, _snapOnInput{ true }, - _boxSelection{ false }, - _selectionActive{ false }, + _blockSelection{ false }, + _selection{ std::nullopt }, _allowSingleCharSelection{ true }, - _copyOnSelect{ false }, - _selectionAnchor{ 0, 0 }, - _endSelectionPosition{ 0, 0 } + _copyOnSelect{ false } { auto dispatch = std::make_unique(*this); auto engine = std::make_unique(std::move(dispatch)); diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 335e1ee469..254e254e67 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -142,7 +142,7 @@ public: void ClearSelection() override; void SelectNewRegion(const COORD coordStart, const COORD coordEnd) override; const COORD GetSelectionAnchor() const noexcept override; - const COORD GetEndSelectionPosition() const noexcept override; + const COORD GetSelectionEnd() const noexcept override; const std::wstring GetConsoleTitle() const noexcept override; void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute) override; #pragma endregion @@ -157,12 +157,17 @@ public: #pragma region TextSelection // These methods are defined in TerminalSelection.cpp + enum class SelectionExpansionMode + { + Cell, + Word, + Line + }; const bool IsCopyOnSelectActive() const noexcept; - void DoubleClickSelection(const COORD position); - void TripleClickSelection(const COORD position); + void MultiClickSelection(const COORD viewportPos, SelectionExpansionMode expansionMode); void SetSelectionAnchor(const COORD position); - void SetEndSelectionPosition(const COORD position); - void SetBoxSelection(const bool isEnabled) noexcept; + void SetSelectionEnd(const COORD position, std::optional newExpansionMode = std::nullopt); + void SetBlockSelection(const bool isEnabled) noexcept; const TextBuffer::TextAndColor RetrieveSelectedTextFromBuffer(bool trimTrailingWhitespace) const; #pragma endregion @@ -187,19 +192,20 @@ private: bool _suppressApplicationTitle; #pragma region Text Selection - enum class SelectionExpansionMode + // a selection is represented as a range between two COORDs (start and end) + // the pivot is the COORD that remains selected when you extend a selection in any direction + // this is particularly useful when a word selection is extended over its starting point + // see TerminalSelection.cpp for more information + struct SelectionAnchors { - Cell, - Word, - Line + COORD start; + COORD end; + COORD pivot; }; - COORD _selectionAnchor; - COORD _endSelectionPosition; - bool _boxSelection; - bool _selectionActive; + std::optional _selection; + bool _blockSelection; bool _allowSingleCharSelection; bool _copyOnSelect; - SHORT _selectionVerticalOffset; std::wstring _wordDelimiters; SelectionExpansionMode _multiClickSelectionMode; #pragma endregion @@ -248,15 +254,10 @@ private: #pragma region TextSelection // These methods are defined in TerminalSelection.cpp std::vector _GetSelectionRects() const noexcept; - SHORT _ExpandWideGlyphSelectionLeft(const SHORT xPos, const SHORT yPos) const; - SHORT _ExpandWideGlyphSelectionRight(const SHORT xPos, const SHORT yPos) const; - COORD _ExpandDoubleClickSelectionLeft(const COORD position) const; - COORD _ExpandDoubleClickSelectionRight(const COORD position) const; + std::pair _PivotSelection(const COORD targetPos) const; + std::pair _ExpandSelectionAnchors(std::pair anchors) const; COORD _ConvertToBufferCell(const COORD viewportPos) const; const bool _IsSingleCellSelection() const noexcept; - std::tuple _PreprocessSelectionCoords() const; - SMALL_RECT _GetSelectionRow(const SHORT row, const COORD higherCoord, const COORD lowerCoord) const; - void _ExpandSelectionRow(SMALL_RECT& selectionRow) const; #pragma endregion #ifdef UNIT_TESTING diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index cdf229f972..6e0aebaa0a 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -7,6 +7,38 @@ using namespace Microsoft::Terminal::Core; +/* Selection Pivot Description: + * The pivot helps properly update the selection when a user moves a selection over itself + * See SelectionTest::DoubleClickDrag_Left for an example of the functionality mentioned here + * As an example, consider the following scenario... + * 1. Perform a word selection (double-click) on a word + * + * |-position where we double-clicked + * _|_ + * |word| + * |--| + * start & pivot-| |-end + * + * 2. Drag your mouse down a line + * + * + * start & pivot-|__________ + * __|word_______| + * |______| + * | + * |-end & mouse position + * + * 3. Drag your mouse up two lines + * + * |-start & mouse position + * |________ + * ____| ______| + * |___w|ord + * |-end & pivot + * + * The pivot never moves until a new selection is created. It ensures that that cell will always be selected. + */ + // Method Description: // - Helper to determine the selected region of the buffer. Used for rendering. // Return Value: @@ -22,91 +54,12 @@ std::vector Terminal::_GetSelectionRects() const noexcept try { - // NOTE: (0,0) is the top-left of the screen - // the physically "higher" coordinate is closer to the top-left - // the physically "lower" coordinate is closer to the bottom-right - const auto [higherCoord, lowerCoord] = _PreprocessSelectionCoords(); - - SHORT selectionRectSize; - THROW_IF_FAILED(ShortSub(lowerCoord.Y, higherCoord.Y, &selectionRectSize)); - THROW_IF_FAILED(ShortAdd(selectionRectSize, 1, &selectionRectSize)); - - std::vector selectionArea; - selectionArea.reserve(selectionRectSize); - for (auto row = higherCoord.Y; row <= lowerCoord.Y; row++) - { - SMALL_RECT selectionRow = _GetSelectionRow(row, higherCoord, lowerCoord); - _ExpandSelectionRow(selectionRow); - selectionArea.emplace_back(selectionRow); - } - result.swap(selectionArea); + return _buffer->GetTextRects(_selection->start, _selection->end, _blockSelection); } CATCH_LOG(); return result; } -// Method Description: -// - convert selection anchors to proper coordinates for rendering -// NOTE: (0,0) is top-left so vertical comparison is inverted -// Arguments: -// - None -// Return Value: -// - tuple.first: the physically "higher" coordinate (closer to the top-left) -// - tuple.second: the physically "lower" coordinate (closer to the bottom-right) -std::tuple Terminal::_PreprocessSelectionCoords() const -{ - // create these new anchors for comparison and rendering - COORD selectionAnchorWithOffset{ _selectionAnchor }; - COORD endSelectionPositionWithOffset{ _endSelectionPosition }; - - // Add anchor offset here to update properly on new buffer output - THROW_IF_FAILED(ShortAdd(selectionAnchorWithOffset.Y, _selectionVerticalOffset, &selectionAnchorWithOffset.Y)); - THROW_IF_FAILED(ShortAdd(endSelectionPositionWithOffset.Y, _selectionVerticalOffset, &endSelectionPositionWithOffset.Y)); - - // clamp anchors to be within buffer bounds - const auto bufferSize = _buffer->GetSize(); - bufferSize.Clamp(selectionAnchorWithOffset); - bufferSize.Clamp(endSelectionPositionWithOffset); - - // NOTE: (0,0) is top-left so vertical comparison is inverted - // CompareInBounds returns whether A is to the left of (rv<0), equal to (rv==0), or to the right of (rv>0) B. - // Here, we want the "left"most coordinate to be the one "higher" on the screen. The other gets the dubious honor of - // being the "lower." - return bufferSize.CompareInBounds(selectionAnchorWithOffset, endSelectionPositionWithOffset) <= 0 ? - std::make_tuple(selectionAnchorWithOffset, endSelectionPositionWithOffset) : - std::make_tuple(endSelectionPositionWithOffset, selectionAnchorWithOffset); -} - -// Method Description: -// - constructs the selection row at the given row -// NOTE: (0,0) is top-left so vertical comparison is inverted -// Arguments: -// - row: the buffer y-value under observation -// - higherCoord: the physically "higher" coordinate (closer to the top-left) -// - lowerCoord: the physically "lower" coordinate (closer to the bottom-right) -// Return Value: -// - the selection row needed for rendering -SMALL_RECT Terminal::_GetSelectionRow(const SHORT row, const COORD higherCoord, const COORD lowerCoord) const -{ - SMALL_RECT selectionRow; - - selectionRow.Top = row; - selectionRow.Bottom = row; - - if (_boxSelection || higherCoord.Y == lowerCoord.Y) - { - selectionRow.Left = std::min(higherCoord.X, lowerCoord.X); - selectionRow.Right = std::max(higherCoord.X, lowerCoord.X); - } - else - { - selectionRow.Left = (row == higherCoord.Y) ? higherCoord.X : _buffer->GetSize().Left(); - selectionRow.Right = (row == lowerCoord.Y) ? lowerCoord.X : _buffer->GetSize().RightInclusive(); - } - - return selectionRow; -} - // Method Description: // - Get the current anchor position relative to the whole text buffer // Arguments: @@ -115,9 +68,7 @@ SMALL_RECT Terminal::_GetSelectionRow(const SHORT row, const COORD higherCoord, // - None const COORD Terminal::GetSelectionAnchor() const noexcept { - COORD selectionAnchorPos{ _selectionAnchor }; - selectionAnchorPos.Y = base::ClampAdd(selectionAnchorPos.Y, _selectionVerticalOffset); - return selectionAnchorPos; + return _selection->start; } // Method Description: @@ -126,91 +77,9 @@ const COORD Terminal::GetSelectionAnchor() const noexcept // - None // Return Value: // - None -const COORD Terminal::GetEndSelectionPosition() const noexcept +const COORD Terminal::GetSelectionEnd() const noexcept { - COORD endSelectionPos{ _endSelectionPosition }; - endSelectionPos.Y = base::ClampAdd(endSelectionPos.Y, _selectionVerticalOffset); - return endSelectionPos; -} - -// Method Description: -// - Expand the selection row according to selection mode and wide glyphs -// - this is particularly useful for box selections (ALT + selection) -// Arguments: -// - selectionRow: the selection row to be expanded -// Return Value: -// - modifies selectionRow's Left and Right values to expand properly -void Terminal::_ExpandSelectionRow(SMALL_RECT& selectionRow) const -{ - const auto row = selectionRow.Top; - - // expand selection for Double/Triple Click - if (_multiClickSelectionMode == SelectionExpansionMode::Word) - { - selectionRow.Left = _ExpandDoubleClickSelectionLeft({ selectionRow.Left, row }).X; - selectionRow.Right = _ExpandDoubleClickSelectionRight({ selectionRow.Right, row }).X; - } - else if (_multiClickSelectionMode == SelectionExpansionMode::Line) - { - selectionRow.Left = _buffer->GetSize().Left(); - selectionRow.Right = _buffer->GetSize().RightInclusive(); - } - - // expand selection for Wide Glyphs - selectionRow.Left = _ExpandWideGlyphSelectionLeft(selectionRow.Left, row); - selectionRow.Right = _ExpandWideGlyphSelectionRight(selectionRow.Right, row); -} - -// Method Description: -// - Expands the selection left-wards to cover a wide glyph, if necessary -// Arguments: -// - position: the (x,y) coordinate on the visible viewport -// Return Value: -// - updated x position to encapsulate the wide glyph -SHORT Terminal::_ExpandWideGlyphSelectionLeft(const SHORT xPos, const SHORT yPos) const -{ - // don't change the value if at/outside the boundary - const auto bufferSize = _buffer->GetSize(); - if (xPos <= bufferSize.Left() || xPos > bufferSize.RightInclusive()) - { - return xPos; - } - - COORD position{ xPos, yPos }; - const auto attr = _buffer->GetCellDataAt(position)->DbcsAttr(); - if (attr.IsTrailing()) - { - // move off by highlighting the lead half too. - // alters position.X - bufferSize.DecrementInBounds(position); - } - return position.X; -} - -// Method Description: -// - Expands the selection right-wards to cover a wide glyph, if necessary -// Arguments: -// - position: the (x,y) coordinate on the visible viewport -// Return Value: -// - updated x position to encapsulate the wide glyph -SHORT Terminal::_ExpandWideGlyphSelectionRight(const SHORT xPos, const SHORT yPos) const -{ - // don't change the value if at/outside the boundary - const auto bufferSize = _buffer->GetSize(); - if (xPos < bufferSize.Left() || xPos >= bufferSize.RightInclusive()) - { - return xPos; - } - - COORD position{ xPos, yPos }; - const auto attr = _buffer->GetCellDataAt(position)->DbcsAttr(); - if (attr.IsLeading()) - { - // move off by highlighting the trailing half too. - // alters position.X - bufferSize.IncrementInBounds(position); - } - return position.X; + return _selection->end; } // Method Description: @@ -219,7 +88,7 @@ SHORT Terminal::_ExpandWideGlyphSelectionRight(const SHORT xPos, const SHORT yPo // - bool representing if selection is only a single cell. Used for copyOnSelect const bool Terminal::_IsSingleCellSelection() const noexcept { - return (_selectionAnchor == _endSelectionPosition); + return (_selection->start == _selection->end); } // Method Description: @@ -234,7 +103,7 @@ const bool Terminal::IsSelectionActive() const noexcept { return false; } - return _selectionActive; + return _selection.has_value(); } // Method Description: @@ -247,79 +116,60 @@ const bool Terminal::IsCopyOnSelectActive() const noexcept } // Method Description: -// - Select the sequence between delimiters defined in Settings +// - Perform a multi-click selection at viewportPos expanding according to the expansionMode // Arguments: -// - position: the (x,y) coordinate on the visible viewport -void Terminal::DoubleClickSelection(const COORD position) +// - viewportPos: the (x,y) coordinate on the visible viewport +// - expansionMode: the SelectionExpansionMode to dictate the boundaries of the selection anchors +void Terminal::MultiClickSelection(const COORD viewportPos, SelectionExpansionMode expansionMode) { -#pragma warning(suppress : 26496) // cpp core checks wants this const but .Clamp() can write it. - COORD positionWithOffsets = _ConvertToBufferCell(position); + // set the selection pivot to expand the selection using SetSelectionEnd() + _selection = SelectionAnchors{}; + _selection->pivot = _ConvertToBufferCell(viewportPos); - // scan leftwards until delimiter is found and - // set selection anchor to one right of that spot - _selectionAnchor = _ExpandDoubleClickSelectionLeft(positionWithOffsets); - THROW_IF_FAILED(ShortSub(_selectionAnchor.Y, gsl::narrow(ViewStartIndex()), &_selectionAnchor.Y)); - _selectionVerticalOffset = gsl::narrow(ViewStartIndex()); + _multiClickSelectionMode = expansionMode; + SetSelectionEnd(viewportPos); - // scan rightwards until delimiter is found and - // set endSelectionPosition to one left of that spot - _endSelectionPosition = _ExpandDoubleClickSelectionRight(positionWithOffsets); - THROW_IF_FAILED(ShortSub(_endSelectionPosition.Y, gsl::narrow(ViewStartIndex()), &_endSelectionPosition.Y)); - - _selectionActive = true; - _multiClickSelectionMode = SelectionExpansionMode::Word; -} - -// Method Description: -// - Select the entire row of the position clicked -// Arguments: -// - position: the (x,y) coordinate on the visible viewport -void Terminal::TripleClickSelection(const COORD position) -{ - SetSelectionAnchor({ 0, position.Y }); - SetEndSelectionPosition({ _buffer->GetSize().RightInclusive(), position.Y }); - - _multiClickSelectionMode = SelectionExpansionMode::Line; + // we need to set the _selectionPivot again + // for future shift+clicks + _selection->pivot = _selection->start; } // Method Description: // - Record the position of the beginning of a selection // Arguments: // - position: the (x,y) coordinate on the visible viewport -void Terminal::SetSelectionAnchor(const COORD position) +void Terminal::SetSelectionAnchor(const COORD viewportPos) { - _selectionAnchor = position; + _selection = SelectionAnchors{}; + _selection->pivot = _ConvertToBufferCell(viewportPos); - // include _scrollOffset here to ensure this maps to the right spot of the original viewport - THROW_IF_FAILED(ShortSub(_selectionAnchor.Y, gsl::narrow(_scrollOffset), &_selectionAnchor.Y)); - - // copy value of ViewStartIndex to support scrolling - // and update on new buffer output (used in _GetSelectionRects()) - _selectionVerticalOffset = gsl::narrow(ViewStartIndex()); - - _selectionActive = true; _allowSingleCharSelection = (_copyOnSelect) ? false : true; - SetEndSelectionPosition(position); - _multiClickSelectionMode = SelectionExpansionMode::Cell; + SetSelectionEnd(viewportPos); + + _selection->start = _selection->pivot; } // Method Description: -// - Record the position of the end of a selection +// - Update selection anchors when dragging to a position +// - based on the selection expansion mode // Arguments: -// - position: the (x,y) coordinate on the visible viewport -void Terminal::SetEndSelectionPosition(const COORD position) +// - viewportPos: the (x,y) coordinate on the visible viewport +// - newExpansionMode: overwrites the _multiClickSelectionMode for this function call. Used for ShiftClick +void Terminal::SetSelectionEnd(const COORD viewportPos, std::optional newExpansionMode) { - _endSelectionPosition = position; + const auto textBufferPos = _ConvertToBufferCell(viewportPos); - // include _scrollOffset here to ensure this maps to the right spot of the original viewport - THROW_IF_FAILED(ShortSub(_endSelectionPosition.Y, gsl::narrow(_scrollOffset), &_endSelectionPosition.Y)); + // if this is a shiftClick action, we need to overwrite the _multiClickSelectionMode value (even if it's the same) + // Otherwise, we may accidentally expand during other selection-based actions + _multiClickSelectionMode = newExpansionMode.has_value() ? *newExpansionMode : _multiClickSelectionMode; - // copy value of ViewStartIndex to support scrolling - // and update on new buffer output (used in _GetSelectionRects()) - _selectionVerticalOffset = gsl::narrow(ViewStartIndex()); + const auto anchors = _PivotSelection(textBufferPos); + std::tie(_selection->start, _selection->end) = _ExpandSelectionAnchors(anchors); + // moving the endpoint of what used to be a single cell selection + // allows the user to drag back and select just one cell if (_copyOnSelect && !_IsSingleCellSelection()) { _allowSingleCharSelection = true; @@ -327,12 +177,65 @@ void Terminal::SetEndSelectionPosition(const COORD position) } // Method Description: -// - enable/disable box selection (ALT + selection) +// - returns a new pair of selection anchors for selecting around the pivot +// - This ensures start < end when compared // Arguments: -// - isEnabled: new value for _boxSelection -void Terminal::SetBoxSelection(const bool isEnabled) noexcept +// - targetPos: the (x,y) coordinate we are moving to on the text buffer +// Return Value: +// - the new start/end for a selection +std::pair Terminal::_PivotSelection(const COORD targetPos) const { - _boxSelection = isEnabled; + if (_buffer->GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0) + { + // target is before pivot + // treat target as start + return std::make_pair(targetPos, _selection->pivot); + } + else + { + // target is after pivot + // treat pivot as start + return std::make_pair(_selection->pivot, targetPos); + } +} + +// Method Description: +// - Update the selection anchors to expand according to the expansion mode +// Arguments: +// - anchors: a pair of selection anchors representing a desired selection +// Return Value: +// - the new start/end for a selection +std::pair Terminal::_ExpandSelectionAnchors(std::pair anchors) const +{ + COORD start = anchors.first; + COORD end = anchors.second; + + const auto bufferSize = _buffer->GetSize(); + switch (_multiClickSelectionMode) + { + case SelectionExpansionMode::Line: + start = { bufferSize.Left(), start.Y }; + end = { bufferSize.RightInclusive(), end.Y }; + break; + case SelectionExpansionMode::Word: + start = _buffer->GetWordStart(start, _wordDelimiters); + end = _buffer->GetWordEnd(end, _wordDelimiters); + break; + case SelectionExpansionMode::Cell: + default: + // no expansion is necessary + break; + } + return std::make_pair(start, end); +} + +// Method Description: +// - enable/disable block selection (ALT + selection) +// Arguments: +// - isEnabled: new value for _blockSelection +void Terminal::SetBlockSelection(const bool isEnabled) noexcept +{ + _blockSelection = isEnabled; } // Method Description: @@ -340,11 +243,8 @@ void Terminal::SetBoxSelection(const bool isEnabled) noexcept #pragma warning(disable : 26440) // changing this to noexcept would require a change to ConHost's selection model void Terminal::ClearSelection() { - _selectionActive = false; _allowSingleCharSelection = false; - _selectionAnchor = { 0, 0 }; - _endSelectionPosition = { 0, 0 }; - _selectionVerticalOffset = 0; + _selection = std::nullopt; } // Method Description: @@ -359,47 +259,13 @@ const TextBuffer::TextAndColor Terminal::RetrieveSelectedTextFromBuffer(bool tri std::function GetForegroundColor = std::bind(&Terminal::GetForegroundColor, this, std::placeholders::_1); std::function GetBackgroundColor = std::bind(&Terminal::GetBackgroundColor, this, std::placeholders::_1); - return _buffer->GetTextForClipboard(!_boxSelection, + return _buffer->GetTextForClipboard(!_blockSelection, trimTrailingWhitespace, _GetSelectionRects(), GetForegroundColor, GetBackgroundColor); } -// Method Description: -// - expand the double click selection to the left -// - stopped by delimiter if started on delimiter -// Arguments: -// - position: buffer coordinate for selection -// Return Value: -// - updated copy of "position" to new expanded location (with vertical offset) -COORD Terminal::_ExpandDoubleClickSelectionLeft(const COORD position) const -{ - // force position to be within bounds -#pragma warning(suppress : 26496) // cpp core checks wants this const but .Clamp() can write it. - COORD positionWithOffsets = position; - _buffer->GetSize().Clamp(positionWithOffsets); - - return _buffer->GetWordStart(positionWithOffsets, _wordDelimiters); -} - -// Method Description: -// - expand the double click selection to the right -// - stopped by delimiter if started on delimiter -// Arguments: -// - position: buffer coordinate for selection -// Return Value: -// - updated copy of "position" to new expanded location (with vertical offset) -COORD Terminal::_ExpandDoubleClickSelectionRight(const COORD position) const -{ - // force position to be within bounds -#pragma warning(suppress : 26496) // cpp core checks wants this const but .Clamp() can write it. - COORD positionWithOffsets = position; - _buffer->GetSize().Clamp(positionWithOffsets); - - return _buffer->GetWordEnd(positionWithOffsets, _wordDelimiters); -} - // Method Description: // - convert viewport position to the corresponding location on the buffer // Arguments: @@ -408,13 +274,10 @@ COORD Terminal::_ExpandDoubleClickSelectionRight(const COORD position) const // - the corresponding location on the buffer COORD Terminal::_ConvertToBufferCell(const COORD viewportPos) const { - // Force position to be valid - COORD positionWithOffsets = viewportPos; - _buffer->GetSize().Clamp(positionWithOffsets); - - THROW_IF_FAILED(ShortSub(viewportPos.Y, gsl::narrow(_scrollOffset), &positionWithOffsets.Y)); - THROW_IF_FAILED(ShortAdd(positionWithOffsets.Y, gsl::narrow(ViewStartIndex()), &positionWithOffsets.Y)); - return positionWithOffsets; + const auto yPos = base::ClampedNumeric(_VisibleStartIndex()) + viewportPos.Y; + COORD bufferPos = { viewportPos.X, yPos }; + _buffer->GetSize().Clamp(bufferPos); + return bufferPos; } // Method Description: diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index 81794c4fba..f894a54c96 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -173,7 +173,7 @@ void Terminal::SelectNewRegion(const COORD coordStart, const COORD coordEnd) realCoordEnd.Y -= gsl::narrow(_VisibleStartIndex()); SetSelectionAnchor(realCoordStart); - SetEndSelectionPosition(realCoordEnd); + SetSelectionEnd(realCoordEnd, SelectionExpansionMode::Cell); } const std::wstring Terminal::GetConsoleTitle() const noexcept diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 0c2e5df813..447910e47e 100644 --- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp @@ -72,7 +72,7 @@ namespace TerminalCoreUnitTests term.SetSelectionAnchor({ 5, rowValue }); // Simulate move to (x,y) = (15,20) - term.SetEndSelectionPosition({ 15, 20 }); + term.SetSelectionEnd({ 15, 20 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -110,19 +110,19 @@ namespace TerminalCoreUnitTests { const COORD maxCoord = { SHRT_MAX, SHRT_MAX }; - // Test SetSelectionAnchor(COORD) and SetEndSelectionPosition(COORD) + // Test SetSelectionAnchor(COORD) and SetSelectionEnd(COORD) // Behavior: clamp coord to viewport. auto ValidateSingleClickSelection = [&](SHORT scrollback, SMALL_RECT expected) { Terminal term; DummyRenderTarget emptyRT; term.Create({ 10, 10 }, scrollback, emptyRT); - // NOTE: SetEndSelectionPosition(COORD) is called within SetSelectionAnchor(COORD) + // NOTE: SetSelectionEnd(COORD) is called within SetSelectionAnchor(COORD) term.SetSelectionAnchor(maxCoord); ValidateSingleRowSelection(term, expected); }; - // Test DoubleClickSelection(COORD) + // Test a Double Click Selection // Behavior: clamp coord to viewport. // Then, do double click selection. auto ValidateDoubleClickSelection = [&](SHORT scrollback, SMALL_RECT expected) { @@ -130,11 +130,11 @@ namespace TerminalCoreUnitTests DummyRenderTarget emptyRT; term.Create({ 10, 10 }, scrollback, emptyRT); - term.DoubleClickSelection(maxCoord); + term.MultiClickSelection(maxCoord, Terminal::SelectionExpansionMode::Word); ValidateSingleRowSelection(term, expected); }; - // Test TripleClickSelection(COORD) + // Test a Triple Click Selection // Behavior: clamp coord to viewport. // Then, do triple click selection. auto ValidateTripleClickSelection = [&](SHORT scrollback, SMALL_RECT expected) { @@ -142,7 +142,7 @@ namespace TerminalCoreUnitTests DummyRenderTarget emptyRT; term.Create({ 10, 10 }, scrollback, emptyRT); - term.TripleClickSelection(maxCoord); + term.MultiClickSelection(maxCoord, Terminal::SelectionExpansionMode::Line); ValidateSingleRowSelection(term, expected); }; @@ -226,17 +226,17 @@ namespace TerminalCoreUnitTests // Case 1: Move out of right boundary Log::Comment(L"Out of bounds: X-value too large"); - term.SetEndSelectionPosition({ 20, 5 }); + term.SetSelectionEnd({ 20, 5 }); ValidateSingleRowSelection(term, SMALL_RECT({ 5, 5, rightBoundary, 5 })); // Case 2: Move out of left boundary Log::Comment(L"Out of bounds: X-value negative"); - term.SetEndSelectionPosition({ -20, 5 }); + term.SetSelectionEnd({ -20, 5 }); ValidateSingleRowSelection(term, { leftBoundary, 5, 5, 5 }); // Case 3: Move out of top boundary Log::Comment(L"Out of bounds: Y-value negative"); - term.SetEndSelectionPosition({ 5, -20 }); + term.SetSelectionEnd({ 5, -20 }); { auto selectionRects = term.GetSelectionRects(); @@ -267,7 +267,7 @@ namespace TerminalCoreUnitTests // Case 4: Move out of bottom boundary Log::Comment(L"Out of bounds: Y-value too large"); - term.SetEndSelectionPosition({ 5, 20 }); + term.SetSelectionEnd({ 5, 20 }); { auto selectionRects = term.GetSelectionRects(); @@ -310,10 +310,10 @@ namespace TerminalCoreUnitTests // Simulate ALT + click at (x,y) = (5,10) term.SetSelectionAnchor({ 5, rowValue }); - term.SetBoxSelection(true); + term.SetBlockSelection(true); // Simulate move to (x,y) = (15,20) - term.SetEndSelectionPosition({ 15, 20 }); + term.SetSelectionEnd({ 15, 20 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -349,7 +349,7 @@ namespace TerminalCoreUnitTests term.SetSelectionAnchor({ 5, rowValue }); // Simulate move to (x,y) = (15,20) - term.SetEndSelectionPosition({ 15, 20 }); + term.SetSelectionEnd({ 15, 20 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -449,10 +449,10 @@ namespace TerminalCoreUnitTests // Simulate ALT + click at (x,y) = (5,8) term.SetSelectionAnchor({ 5, 8 }); - term.SetBoxSelection(true); + term.SetBlockSelection(true); // Simulate move to (x,y) = (7,12) - term.SetEndSelectionPosition({ 7, 12 }); + term.SetSelectionEnd({ 7, 12 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -501,7 +501,7 @@ namespace TerminalCoreUnitTests // Simulate double click at (x,y) = (5,10) auto clickPos = COORD{ 5, 10 }; - term.DoubleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Word); // Validate selection area ValidateSingleRowSelection(term, SMALL_RECT({ 4, 10, (4 + gsl::narrow(text.size()) - 1), 10 })); @@ -519,7 +519,7 @@ namespace TerminalCoreUnitTests // Simulate click at (x,y) = (5,10) auto clickPos = COORD{ 5, 10 }; - term.DoubleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Word); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -546,7 +546,7 @@ namespace TerminalCoreUnitTests // Simulate click at (x,y) = (15,10) // this is over the '>' char auto clickPos = COORD{ 15, 10 }; - term.DoubleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Word); // ---Validate selection area--- // "Terminal" is in class 2 @@ -572,14 +572,14 @@ namespace TerminalCoreUnitTests term.Write(text); // Simulate double click at (x,y) = (5,10) - term.DoubleClickSelection({ 5, 10 }); + term.MultiClickSelection({ 5, 10 }, Terminal::SelectionExpansionMode::Word); // Simulate move to (x,y) = (21,10) // // buffer: doubleClickMe dragThroughHere // ^ ^ // start finish - term.SetEndSelectionPosition({ 21, 10 }); + term.SetSelectionEnd({ 21, 10 }); // Validate selection area ValidateSingleRowSelection(term, SMALL_RECT({ 4, 10, 32, 10 })); @@ -601,14 +601,14 @@ namespace TerminalCoreUnitTests term.Write(text); // Simulate double click at (x,y) = (21,10) - term.DoubleClickSelection({ 21, 10 }); + term.MultiClickSelection({ 21, 10 }, Terminal::SelectionExpansionMode::Word); // Simulate move to (x,y) = (5,10) // // buffer: doubleClickMe dragThroughHere // ^ ^ // finish start - term.SetEndSelectionPosition({ 5, 10 }); + term.SetSelectionEnd({ 5, 10 }); // Validate selection area ValidateSingleRowSelection(term, SMALL_RECT({ 4, 10, 32, 10 })); @@ -622,7 +622,7 @@ namespace TerminalCoreUnitTests // Simulate click at (x,y) = (5,10) auto clickPos = COORD{ 5, 10 }; - term.TripleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Line); // Validate selection area ValidateSingleRowSelection(term, SMALL_RECT({ 0, 10, 99, 10 })); @@ -636,10 +636,10 @@ namespace TerminalCoreUnitTests // Simulate click at (x,y) = (5,10) auto clickPos = COORD{ 5, 10 }; - term.TripleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Line); // Simulate move to (x,y) = (7,10) - term.SetEndSelectionPosition({ 7, 10 }); + term.SetSelectionEnd({ 7, 10 }); // Validate selection area ValidateSingleRowSelection(term, SMALL_RECT({ 0, 10, 99, 10 })); @@ -653,10 +653,10 @@ namespace TerminalCoreUnitTests // Simulate click at (x,y) = (5,10) auto clickPos = COORD{ 5, 10 }; - term.TripleClickSelection(clickPos); + term.MultiClickSelection(clickPos, Terminal::SelectionExpansionMode::Line); // Simulate move to (x,y) = (5,11) - term.SetEndSelectionPosition({ 5, 11 }); + term.SetSelectionEnd({ 5, 11 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionRects = term.GetSelectionRects(); @@ -689,7 +689,7 @@ namespace TerminalCoreUnitTests // Simulate move to (x,y) = (5,10) // (So, no movement) - term.SetEndSelectionPosition({ 5, 10 }); + term.SetSelectionEnd({ 5, 10 }); // Case 1: single cell selection not allowed { @@ -705,12 +705,12 @@ namespace TerminalCoreUnitTests } // Case 2: move off of single cell - term.SetEndSelectionPosition({ 6, 10 }); + term.SetSelectionEnd({ 6, 10 }); ValidateSingleRowSelection(term, { 5, 10, 6, 10 }); VERIFY_IS_TRUE(term.IsSelectionActive()); // Case 3: move back onto single cell (now allowed) - term.SetEndSelectionPosition({ 5, 10 }); + term.SetSelectionEnd({ 5, 10 }); ValidateSingleRowSelection(term, { 5, 10, 5, 10 }); // single cell selection should now be allowed diff --git a/src/host/renderData.cpp b/src/host/renderData.cpp index f6e1451938..5cd99ce185 100644 --- a/src/host/renderData.cpp +++ b/src/host/renderData.cpp @@ -400,7 +400,7 @@ const COORD RenderData::GetSelectionAnchor() const noexcept // - none // Return Value: // - current selection anchor -const COORD RenderData::GetEndSelectionPosition() const noexcept +const COORD RenderData::GetSelectionEnd() const noexcept { // The selection area in ConHost is encoded as two things... // - SelectionAnchor: the initial position where the selection was started diff --git a/src/host/renderData.hpp b/src/host/renderData.hpp index 3a548e0a81..6005412ddf 100644 --- a/src/host/renderData.hpp +++ b/src/host/renderData.hpp @@ -61,7 +61,7 @@ public: void ClearSelection() override; void SelectNewRegion(const COORD coordStart, const COORD coordEnd) override; const COORD GetSelectionAnchor() const noexcept; - const COORD GetEndSelectionPosition() const noexcept; + const COORD GetSelectionEnd() const noexcept; void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr); #pragma endregion }; diff --git a/src/host/selection.cpp b/src/host/selection.cpp index 1103cfd80b..7da4ee0cf2 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -34,111 +34,6 @@ Selection& Selection::Instance() return *_instance; } -// Routine Description: -// - Determines the line-by-line selection rectangles based on global selection state. -// Arguments: -// - selectionRect - The selection rectangle outlining the region to be selected -// - selectionAnchor - The corner of the selection rectangle that selection started from -// - lineSelection - True to process in line mode. False to process in block mode. -// Return Value: -// - Returns a vector where each SMALL_RECT is one Row worth of the area to be selected. -// - Returns empty vector if no rows are selected. -// - Throws exceptions for out of memory issues -std::vector Selection::s_GetSelectionRects(const SMALL_RECT& selectionRect, - const COORD selectionAnchor, - const bool lineSelection) -{ - std::vector selectionAreas; - - const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - const auto& screenInfo = gci.GetActiveOutputBuffer(); - - // if the anchor (start of select) was in the top right or bottom left of the box, - // we need to remove rectangular overlap in the middle. - // e.g. - // For selections with the anchor in the top left (A) or bottom right (B), - // it is valid to maintain the inner rectangle (+) as part of the selection - // A+++++++================ - // ==============++++++++B - // + and = are valid highlights in this scenario. - // For selections with the anchor in in the top right (A) or bottom left (B), - // we must remove a portion of the first/last line that lies within the rectangle (+) - // +++++++A================= - // ==============B+++++++ - // Only = is valid for highlight in this scenario. - // This is only needed for line selection. Box selection doesn't need to account for this. - - bool removeRectPortion = false; - - if (lineSelection) - { - const auto selectionStart = selectionAnchor; - - // only if top and bottom aren't the same line... we need the whole rectangle if we're on the same line. - // e.g. A++++++++++++++B - // All the + are valid select points. - if (selectionRect.Top != selectionRect.Bottom) - { - if ((selectionStart.X == selectionRect.Right && selectionStart.Y == selectionRect.Top) || - (selectionStart.X == selectionRect.Left && selectionStart.Y == selectionRect.Bottom)) - { - removeRectPortion = true; - } - } - } - - // for each row within the selection rectangle - for (short i = selectionRect.Top; i <= selectionRect.Bottom; i++) - { - // create a rectangle representing the highlight on one row - SMALL_RECT highlightRow; - highlightRow.Top = i; - highlightRow.Bottom = i; - highlightRow.Left = selectionRect.Left; - highlightRow.Right = selectionRect.Right; - - // compensate for line selection by extending one or both ends of the rectangle to the edge - if (lineSelection) - { - // if not the first row, pad the left selection to the buffer edge - if (i != selectionRect.Top) - { - highlightRow.Left = 0; - } - - // if not the last row, pad the right selection to the buffer edge - if (i != selectionRect.Bottom) - { - highlightRow.Right = screenInfo.GetBufferSize().RightInclusive(); - } - - // if we've determined we're in a scenario where we must remove the inner rectangle from the lines... - if (removeRectPortion) - { - if (i == selectionRect.Top) - { - // from the top row, move the left edge of the highlight line to the right edge of the rectangle - highlightRow.Left = selectionRect.Right; - } - else if (i == selectionRect.Bottom) - { - // from the bottom row, move the right edge of the highlight line to the left edge of the rectangle - highlightRow.Right = selectionRect.Left; - } - } - } - - // compensate for double width characters by calling double-width measuring/limiting function - const COORD targetPoint{ highlightRow.Left, highlightRow.Top }; - const SHORT stringLength = highlightRow.Right - highlightRow.Left + 1; - highlightRow = s_BisectSelection(stringLength, targetPoint, screenInfo, highlightRow); - - selectionAreas.emplace_back(highlightRow); - } - - return selectionAreas; -} - // Routine Description: // - Determines the line-by-line selection rectangles based on global selection state. // Arguments: @@ -154,65 +49,17 @@ std::vector Selection::GetSelectionRects() const return std::vector(); } - return s_GetSelectionRects(_srSelectionRect, _coordSelectionAnchor, IsLineSelection()); -} + const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + const auto& screenInfo = gci.GetActiveOutputBuffer(); -// Routine Description: -// - This routine checks to ensure that clipboard selection isn't trying to cut a double byte character in half. -// It will adjust the SmallRect rectangle size to ensure this. -// Arguments: -// - sStringLength - The length of the string we're attempting to clip. -// - coordTargetPoint - The row/column position within the text buffer that we're about to try to clip. -// - screenInfo - Screen information structure containing relevant text and dimension information. -// - rect - The region of the text that we want to clip, and then adjusted to the region that should be -// clipped without splicing double-width characters. -// Return Value: -// - the clipped region -SMALL_RECT Selection::s_BisectSelection(const short sStringLength, - const COORD coordTargetPoint, - const SCREEN_INFORMATION& screenInfo, - const SMALL_RECT rect) -{ - SMALL_RECT outRect = rect; - try - { - auto iter = screenInfo.GetCellDataAt(coordTargetPoint); - if (iter->DbcsAttr().IsTrailing()) - { - if (coordTargetPoint.X == 0) - { - outRect.Left++; - } - else - { - outRect.Left--; - } - } + // _coordSelectionAnchor is at one of the corners of _srSelectionRects + // endSelectionAnchor is at the exact opposite corner + COORD endSelectionAnchor; + endSelectionAnchor.X = (_coordSelectionAnchor.X == _srSelectionRect.Left) ? _srSelectionRect.Right : _srSelectionRect.Left; + endSelectionAnchor.Y = (_coordSelectionAnchor.Y == _srSelectionRect.Top) ? _srSelectionRect.Bottom : _srSelectionRect.Top; - // Check end position of strings - if (coordTargetPoint.X + sStringLength < screenInfo.GetBufferSize().Width()) - { - iter += sStringLength; - if (iter->DbcsAttr().IsTrailing()) - { - outRect.Right++; - } - } - else - { - if (coordTargetPoint.Y + 1 < screenInfo.GetBufferSize().Height()) - { - const auto nextLineIter = screenInfo.GetCellDataAt({ 0, coordTargetPoint.Y + 1 }); - if (nextLineIter->DbcsAttr().IsTrailing()) - { - outRect.Right--; - } - } - } - } - CATCH_LOG(); - - return outRect; + const auto blockSelection = !IsLineSelection(); + return screenInfo.GetTextBuffer().GetTextRects(_coordSelectionAnchor, endSelectionAnchor, blockSelection); } // Routine Description: @@ -564,18 +411,13 @@ void Selection::ColorSelection(const SMALL_RECT& srRect, const TextAttribute att // - attr - Color to apply to region. void Selection::ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr) { - // Make a rectangle for the region as if it were selected by a mouse. - // We will use the first one as the "anchor" to represent where the mouse went down. - SMALL_RECT srSelection; - srSelection.Top = std::min(coordSelectionStart.Y, coordSelectionEnd.Y); - srSelection.Bottom = std::max(coordSelectionStart.Y, coordSelectionEnd.Y); - srSelection.Left = std::min(coordSelectionStart.X, coordSelectionEnd.X); - srSelection.Right = std::max(coordSelectionStart.X, coordSelectionEnd.X); - // Extract row-by-row selection rectangles for the selection area. try { - const auto rectangles = s_GetSelectionRects(srSelection, coordSelectionStart, true); + const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + const auto& screenInfo = gci.GetActiveOutputBuffer(); + + const auto rectangles = screenInfo.GetTextBuffer().GetTextRects(coordSelectionStart, coordSelectionEnd); for (const auto& rect : rectangles) { ColorSelection(rect, attr); diff --git a/src/host/selection.hpp b/src/host/selection.hpp index 76a241950d..8b51db5278 100644 --- a/src/host/selection.hpp +++ b/src/host/selection.hpp @@ -72,15 +72,6 @@ private: void _PaintSelection() const; - static SMALL_RECT s_BisectSelection(const short sStringLength, - const COORD coordTargetPoint, - const SCREEN_INFORMATION& screenInfo, - const SMALL_RECT rect); - - static std::vector s_GetSelectionRects(const SMALL_RECT& selectionRect, - const COORD selectionAnchor, - const bool lineSelection); - void _CancelMarkSelection(); void _CancelMouseSelection(); diff --git a/src/host/ut_host/SelectionTests.cpp b/src/host/ut_host/SelectionTests.cpp index 7a6aea220d..e332f3ad0c 100644 --- a/src/host/ut_host/SelectionTests.cpp +++ b/src/host/ut_host/SelectionTests.cpp @@ -323,7 +323,7 @@ class SelectionTests // selection rectangle starts from the target and goes for the length requested srSelection.Left = coordTargetPoint.X; - srSelection.Right = coordTargetPoint.X + sStringLength - 1; + srSelection.Right = coordTargetPoint.X + sStringLength; // save original for comparison srOriginal.Top = srSelection.Top; @@ -331,7 +331,12 @@ class SelectionTests srOriginal.Left = srSelection.Left; srOriginal.Right = srSelection.Right; - srSelection = Selection::s_BisectSelection(sStringLength, coordTargetPoint, screenInfo, srSelection); + COORD startPos{ sTargetX, sTargetY }; + COORD endPos{ base::ClampAdd(sTargetX, sLength), sTargetY }; + const auto selectionRects = screenInfo.GetTextBuffer().GetTextRects(startPos, endPos); + + VERIFY_ARE_EQUAL(static_cast(1), selectionRects.size()); + srSelection = selectionRects.at(0); VERIFY_ARE_EQUAL(srOriginal.Top, srSelection.Top); VERIFY_ARE_EQUAL(srOriginal.Bottom, srSelection.Bottom); @@ -378,10 +383,10 @@ class SelectionTests // start from position 10 before end of row (80 length row) // row is 2 - // selection is 10 characters long + // selection is 9 characters long // the left edge shouldn't move // the right edge should move one to the left (-1) to not select the leading byte - TestBisectSelectionDelta(70, 2, 10, 0, -1); + TestBisectSelectionDelta(70, 2, 9, 0, -1); // 2b. End position is leading half and is elsewhere in the row @@ -389,16 +394,16 @@ class SelectionTests // row is 2 // selection is 10 characters long // the left edge shouldn't move - // the right edge should move one to the right (+1) to add the trailing byte to the selection - TestBisectSelectionDelta(58, 2, 10, 0, 1); + // the right edge should not move, because it is already on the trailing byte + TestBisectSelectionDelta(58, 2, 10, 0, 0); // 2c. End position is leading half and is at end of buffer // start from position 10 before end of row (80 length row) // row is 300 (or 299 for the index) - // selection is 10 characters long + // selection is 9 characters long // the left edge shouldn't move - // the right edge shouldn't move - TestBisectSelectionDelta(70, 299, 10, 0, 0); + // the right edge should move one to the left (-1) to not select the leading byte + TestBisectSelectionDelta(70, 299, 9, 0, -1); } }; diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 1722813292..3343b71608 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -148,6 +148,8 @@ class TextBufferTests void WriteLinesToBuffer(const std::vector& text, TextBuffer& buffer); TEST_METHOD(GetWordBoundaries); + + TEST_METHOD(GetTextRects); }; void TextBufferTests::TestBufferCreate() @@ -2136,3 +2138,68 @@ void TextBufferTests::GetWordBoundaries() VERIFY_ARE_EQUAL(expected, result); } } + +void TextBufferTests::GetTextRects() +{ + // GetTextRects() is used to... + // - Represent selection rects + // - Represent UiaTextRanges for accessibility + + // This is the burrito emoji: 🌯 + // It's encoded in UTF-16, as needed by the buffer. + const auto burrito = std::wstring(L"\xD83C\xDF2F"); + + COORD bufferSize{ 20, 50 }; + UINT cursorSize = 12; + TextAttribute attr{ 0x7f }; + auto _buffer = std::make_unique(bufferSize, attr, cursorSize, _renderTarget); + + // Setup: Write lines of text to the buffer + const std::vector text = { L"0123456789", + L" " + burrito + L"3456" + burrito, + L" " + burrito + L"45" + burrito, + burrito + L"234567" + burrito, + L"0123456789" }; + WriteLinesToBuffer(text, *_buffer); + // - - - Text Buffer Contents - - - + // |0123456789 + // | 🌯3456🌯 + // | 🌯45🌯 + // |🌯234567🌯 + // |0123456789 + // - - - - - - - - - - - - - - - - + + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:blockSelection", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool blockSelection; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"blockSelection", blockSelection), L"Get 'blockSelection' variant"); + + std::vector expected{}; + if (blockSelection) + { + expected.push_back({ 1, 0, 7, 0 }); + expected.push_back({ 1, 1, 8, 1 }); // expand right + expected.push_back({ 1, 2, 7, 2 }); + expected.push_back({ 0, 3, 7, 3 }); // expand left + expected.push_back({ 1, 4, 7, 4 }); + } + else + { + expected.push_back({ 1, 0, 19, 0 }); + expected.push_back({ 0, 1, 19, 1 }); + expected.push_back({ 0, 2, 19, 2 }); + expected.push_back({ 0, 3, 19, 3 }); + expected.push_back({ 0, 4, 7, 4 }); + } + + COORD start{ 1, 0 }; + COORD end{ 7, 4 }; + const auto result = _buffer->GetTextRects(start, end, blockSelection); + VERIFY_ARE_EQUAL(expected.size(), result.size()); + for (size_t i = 0; i < expected.size(); ++i) + { + VERIFY_ARE_EQUAL(expected.at(i), result.at(i)); + } +} diff --git a/src/interactivity/win32/screenInfoUiaProvider.cpp b/src/interactivity/win32/screenInfoUiaProvider.cpp index 6aa03a32e6..873060c041 100644 --- a/src/interactivity/win32/screenInfoUiaProvider.cpp +++ b/src/interactivity/win32/screenInfoUiaProvider.cpp @@ -105,7 +105,7 @@ HRESULT ScreenInfoUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple* const auto start = _pData->GetSelectionAnchor(); // we need to make end exclusive - auto end = _pData->GetEndSelectionPosition(); + auto end = _pData->GetSelectionEnd(); _pData->GetTextBuffer().GetSize().IncrementInBounds(end, true); // TODO GH #4509: Box Selection is misrepresented here as a line selection. diff --git a/src/types/IUiaData.h b/src/types/IUiaData.h index 3106cce532..dda7045976 100644 --- a/src/types/IUiaData.h +++ b/src/types/IUiaData.h @@ -37,7 +37,7 @@ namespace Microsoft::Console::Types virtual void ClearSelection() = 0; virtual void SelectNewRegion(const COORD coordStart, const COORD coordEnd) = 0; virtual const COORD GetSelectionAnchor() const noexcept = 0; - virtual const COORD GetEndSelectionPosition() const noexcept = 0; + virtual const COORD GetSelectionEnd() const noexcept = 0; virtual void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute attr) = 0; }; diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp index 752029f78d..ae973a4fd7 100644 --- a/src/types/TermControlUiaProvider.cpp +++ b/src/types/TermControlUiaProvider.cpp @@ -125,7 +125,7 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple const auto start = _pData->GetSelectionAnchor(); // we need to make end exclusive - auto end = _pData->GetEndSelectionPosition(); + auto end = _pData->GetSelectionEnd(); _pData->GetTextBuffer().GetSize().IncrementInBounds(end, true); // TODO GH #4509: Box Selection is misrepresented here as a line selection. From 161fe60171cc0caa4514dc96354714f2f99d181d Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Fri, 28 Feb 2020 11:32:19 -0800 Subject: [PATCH 13/13] Replace TSFInputControl with a xaml file (#4743) This pull request is like #4729 but for TSFInputControl. --- .../TerminalControl/TSFInputControl.cpp | 48 ++++--------------- .../TerminalControl/TSFInputControl.h | 5 -- .../TerminalControl/TSFInputControl.xaml | 16 +++++++ .../TerminalControl/TerminalControl.vcxproj | 11 +++-- 4 files changed, 34 insertions(+), 46 deletions(-) create mode 100644 src/cascadia/TerminalControl/TSFInputControl.xaml diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index 2e020f1c93..a51969f2d1 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -20,33 +20,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _editContext{ nullptr }, _inComposition{ false } { - _Create(); - } - - // Method Description: - // - Creates XAML controls for displaying user input and hooks up CoreTextEditContext handlers - // for handling text input from the Text Services Framework. - // Arguments: - // - - // Return Value: - // - - void TSFInputControl::_Create() - { - // TextBlock for user input form TSF - _textBlock = Controls::TextBlock(); - _textBlock.Visibility(Visibility::Collapsed); - _textBlock.IsTextSelectionEnabled(false); - _textBlock.TextDecorations(TextDecorations::Underline); - - // Canvas for controlling exact position of the TextBlock - _canvas = Windows::UI::Xaml::Controls::Canvas(); - _canvas.Visibility(Visibility::Collapsed); - - // add the Textblock to the Canvas - _canvas.Children().Append(_textBlock); - - // set the content of this control to be the Canvas - this->Content(_canvas); + InitializeComponent(); // Create a CoreTextEditingContext for since we are acting like a custom edit control auto manager = Core::CoreTextServicesManager::GetForCurrentView(); @@ -177,15 +151,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation request.LayoutBounds().ControlBounds(ScaleRect(controlRect, scaleFactor)); // position textblock to cursor position - _canvas.SetLeft(_textBlock, clientCursorPos.X); - _canvas.SetTop(_textBlock, ::base::ClampedNumeric(clientCursorPos.Y)); + Canvas().SetLeft(TextBlock(), clientCursorPos.X); + Canvas().SetTop(TextBlock(), ::base::ClampedNumeric(clientCursorPos.Y)); - _textBlock.Height(fontHeight); + TextBlock().Height(fontHeight); // calculate FontSize in pixels from DIPs const double fontSizePx = (fontHeight * 72) / USER_DEFAULT_SCREEN_DPI; - _textBlock.FontSize(fontSizePx); + TextBlock().FontSize(fontSizePx); - _textBlock.FontFamily(Media::FontFamily(fontArgs->FontFace())); + TextBlock().FontFamily(Media::FontFamily(fontArgs->FontFace())); } // Method Description: @@ -302,8 +276,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation try { - _canvas.Visibility(Visibility::Visible); - _textBlock.Visibility(Visibility::Visible); + Canvas().Visibility(Visibility::Visible); const auto length = ::base::ClampSub(range.EndCaretPosition, range.StartCaretPosition); _inputBuffer = _inputBuffer.replace( @@ -311,7 +284,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation length, text); - _textBlock.Text(_inputBuffer); + TextBlock().Text(_inputBuffer); // If we receive tabbed IME input like emoji, kaomojis, and symbols, send it to the terminal immediately. // They aren't composition, so we don't want to wait for the user to start and finish a composition to send the text. @@ -348,7 +321,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // clear the buffer for next round const auto bufferLength = ::base::ClampedNumeric(_inputBuffer.length()); _inputBuffer.clear(); - _textBlock.Text(L""); + TextBlock().Text(L""); // Leaving focus before NotifyTextChanged seems to guarantee that the next // composition will send us a CompositionStarted event. @@ -357,8 +330,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _editContext.NotifyFocusEnter(); // hide the controls until text input starts again - _canvas.Visibility(Visibility::Collapsed); - _textBlock.Visibility(Visibility::Collapsed); + Canvas().Visibility(Visibility::Collapsed); } // Method Description: diff --git a/src/cascadia/TerminalControl/TSFInputControl.h b/src/cascadia/TerminalControl/TSFInputControl.h index 8438d37bbb..f463444c0e 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.h +++ b/src/cascadia/TerminalControl/TSFInputControl.h @@ -67,15 +67,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionStarted_revoker _compositionStartedRevoker; winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionCompleted_revoker _compositionCompletedRevoker; - Windows::UI::Xaml::Controls::Canvas _canvas; - Windows::UI::Xaml::Controls::TextBlock _textBlock; - Windows::UI::Text::Core::CoreTextEditContext _editContext; std::wstring _inputBuffer; - void _Create(); - bool _inComposition; void _SendAndClearText(); }; diff --git a/src/cascadia/TerminalControl/TSFInputControl.xaml b/src/cascadia/TerminalControl/TSFInputControl.xaml new file mode 100644 index 0000000000..50c87a6e53 --- /dev/null +++ b/src/cascadia/TerminalControl/TSFInputControl.xaml @@ -0,0 +1,16 @@ + + + + + + diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj b/src/cascadia/TerminalControl/TerminalControl.vcxproj index 2ae8ea7e4c..e057d90bd2 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj @@ -43,7 +43,7 @@ TermControlAutomationPeer.idl - TSFInputControl.idl + TSFInputControl.xaml @@ -59,7 +59,7 @@ TermControl.xaml - TSFInputControl.idl + TSFInputControl.xaml @@ -75,7 +75,9 @@ TermControl.xaml - + + TSFInputControl.xaml + @@ -114,6 +116,9 @@ Designer + + Designer +