From ca6b54e6526396c2367ccf5022429499b45c90b8 Mon Sep 17 00:00:00 2001 From: Leon Liang <57155886+leonMSFT@users.noreply.github.com> Date: Mon, 30 Mar 2020 16:21:47 -0700 Subject: [PATCH] Redraw TSFInputControl when Terminal cursor updates (#5135) ## Summary of the Pull Request This PR will allow TSFInputControl to redraw its Canvas and TextBlock in response to when the Terminal cursor position updates. This will fix the issue where during Korean composition, the first symbol of the next composition will appear on top of the previous composed character. Since the Terminal Cursor updates a lot, I've added some checks to see if the TSFInputControl really needs to redraw. This will also decrease the number of actual redraws since we receive a bunch of `LayoutRequested` events when there's no difference between them. ## PR Checklist * [x] Closes #4963 * [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 Startup, teardown, CJK IME gibberish testing, making sure the IME block shows up in the right place. --- .../TerminalControl/TSFInputControl.cpp | 128 +++++++++++++----- .../TerminalControl/TSFInputControl.h | 9 ++ .../TerminalControl/TSFInputControl.idl | 1 + src/cascadia/TerminalControl/TermControl.cpp | 32 ++++- src/cascadia/TerminalControl/TermControl.h | 1 + src/cascadia/TerminalCore/Terminal.cpp | 15 ++ src/cascadia/TerminalCore/Terminal.hpp | 4 + 7 files changed, 152 insertions(+), 38 deletions(-) diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index a82a4c0a4..477a56573 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -19,7 +19,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation TSFInputControl::TSFInputControl() : _editContext{ nullptr }, _inComposition{ false }, - _activeTextStart{ 0 } + _activeTextStart{ 0 }, + _focused{ false }, + _currentTerminalCursorPos{ 0, 0 }, + _currentCanvasWidth{ 0.0 }, + _currentTextBlockHeight{ 0.0 }, + _currentTextBounds{ 0, 0, 0, 0 }, + _currentControlBounds{ 0, 0, 0, 0 } { InitializeComponent(); @@ -58,7 +64,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // Explicitly disconnect the LayoutRequested handler -- it can cause problems during application teardown. // See GH#4159 for more info. + // Also disconnect compositionCompleted and textUpdating explicitly. It seems to occasionally cause problems if + // a composition is active during application teardown. _layoutRequestedRevoker.revoke(); + _compositionCompletedRevoker.revoke(); + _textUpdatingRevoker.revoke(); } // Method Description: @@ -73,6 +83,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (_editContext != nullptr) { _editContext.NotifyFocusEnter(); + _focused = true; } } @@ -88,6 +99,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation if (_editContext != nullptr) { _editContext.NotifyFocusLeave(); + _focused = false; } } @@ -114,28 +126,51 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } // Method Description: - // - Handler for LayoutRequested event by CoreEditContext responsible - // for returning the current position the IME should be placed - // in screen coordinates on the screen. TSFInputControls internal - // XAML controls (TextBlock/Canvas) are also positioned and updated. - // NOTE: documentation says application should handle this event + // - Redraw the canvas if certain dimensions have changed since the last + // redraw. This includes the Terminal cursor position, the Canvas width, and the TextBlock height. // Arguments: - // - sender: CoreTextEditContext sending the request. - // - args: CoreTextLayoutRequestedEventArgs to be updated with position information. + // - // Return Value: // - - void TSFInputControl::_layoutRequestedHandler(CoreTextEditContext sender, CoreTextLayoutRequestedEventArgs const& args) + void TSFInputControl::TryRedrawCanvas() { - auto request = args.Request(); - - // Get window in screen coordinates, this is the entire window including tabs - const auto windowBounds = CoreWindow::GetForCurrentThread().Bounds(); + if (!_focused) + { + return; + } // Get the cursor position in text buffer position auto cursorArgs = winrt::make_self(); _CurrentCursorPositionHandlers(*this, *cursorArgs); - const COORD cursorPos = { ::base::ClampedNumeric(cursorArgs->CurrentPosition().X), ::base::ClampedNumeric(cursorArgs->CurrentPosition().Y) }; + const til::point cursorPos{ gsl::narrow_cast(cursorArgs->CurrentPosition().X), gsl::narrow_cast(cursorArgs->CurrentPosition().Y) }; + const double actualCanvasWidth = Canvas().ActualWidth(); + + const double actualTextBlockHeight = TextBlock().ActualHeight(); + + if (_currentTerminalCursorPos == cursorPos && + _currentCanvasWidth == actualCanvasWidth && + _currentTextBlockHeight == actualTextBlockHeight) + { + return; + } + + _currentTerminalCursorPos = cursorPos; + _currentCanvasWidth = actualCanvasWidth; + _currentTextBlockHeight = actualTextBlockHeight; + + _RedrawCanvas(); + } + + // Method Description: + // - Redraw the Canvas and update the current Text Bounds and Control Bounds for + // the CoreTextEditContext. + // Arguments: + // - + // Return Value: + // - + void TSFInputControl::_RedrawCanvas() + { // Get Font Info as we use this is the pixel size for characters in the display auto fontArgs = winrt::make_self(); _CurrentFontInfoHandlers(*this, *fontArgs); @@ -145,8 +180,27 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Convert text buffer cursor position to client coordinate position within the window COORD clientCursorPos; - clientCursorPos.X = ::base::ClampMul(cursorPos.X, ::base::ClampedNumeric(fontWidth)); - clientCursorPos.Y = ::base::ClampMul(cursorPos.Y, ::base::ClampedNumeric(fontHeight)); + clientCursorPos.X = ::base::ClampMul(_currentTerminalCursorPos.x(), ::base::ClampedNumeric(fontWidth)); + clientCursorPos.Y = ::base::ClampMul(_currentTerminalCursorPos.y(), ::base::ClampedNumeric(fontHeight)); + + // position textblock to cursor position + Canvas().SetLeft(TextBlock(), clientCursorPos.X); + Canvas().SetTop(TextBlock(), clientCursorPos.Y); + + // calculate FontSize in pixels from DPIs + const double fontSizePx = (fontHeight * 72) / USER_DEFAULT_SCREEN_DPI; + TextBlock().FontSize(fontSizePx); + TextBlock().FontFamily(Media::FontFamily(fontArgs->FontFace())); + + const auto widthToTerminalEnd = _currentCanvasWidth - ::base::ClampedNumeric(clientCursorPos.X); + // Make sure that we're setting the MaxWidth to a positive number - a + // negative number here will crash us in mysterious ways with a useless + // stack trace + const auto newMaxWidth = std::max(0.0, widthToTerminalEnd); + TextBlock().MaxWidth(newMaxWidth); + + // Get window in screen coordinates, this is the entire window including tabs + const auto windowBounds = CoreWindow::GetForCurrentThread().Bounds(); // Convert from client coordinate to screen coordinate by adding window position COORD screenCursorPos; @@ -162,33 +216,35 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // Get scale factor for view const double scaleFactor = DisplayInformation::GetForCurrentView().RawPixelsPerViewPixel(); + const auto yOffset = ::base::ClampedNumeric(_currentTextBlockHeight) - fontHeight; + const auto textBottom = ::base::ClampedNumeric(screenCursorPos.Y) + yOffset; - // position textblock to cursor position - Canvas().SetLeft(TextBlock(), clientCursorPos.X); - Canvas().SetTop(TextBlock(), ::base::ClampedNumeric(clientCursorPos.Y)); + _currentTextBounds = ScaleRect(Rect(screenCursorPos.X, textBottom, 0, fontHeight), scaleFactor); + _currentControlBounds = ScaleRect(Rect(screenCursorPos.X, screenCursorPos.Y, 0, fontHeight), scaleFactor); + } - // calculate FontSize in pixels from DIPs - const double fontSizePx = (fontHeight * 72) / USER_DEFAULT_SCREEN_DPI; - TextBlock().FontSize(fontSizePx); - TextBlock().FontFamily(Media::FontFamily(fontArgs->FontFace())); + // Method Description: + // - Handler for LayoutRequested event by CoreEditContext responsible + // for returning the current position the IME should be placed + // in screen coordinates on the screen. TSFInputControls internal + // XAML controls (TextBlock/Canvas) are also positioned and updated. + // NOTE: documentation says application should handle this event + // Arguments: + // - sender: CoreTextEditContext sending the request. + // - args: CoreTextLayoutRequestedEventArgs to be updated with position information. + // Return Value: + // - + void TSFInputControl::_layoutRequestedHandler(CoreTextEditContext sender, CoreTextLayoutRequestedEventArgs const& args) + { + auto request = args.Request(); - const auto canvasActualWidth = Canvas().ActualWidth(); - const auto widthToTerminalEnd = canvasActualWidth - ::base::ClampedNumeric(clientCursorPos.X); - // Make sure that we're setting the MaxWidth to a positive number - a - // negative number here will crash us in mysterious ways with a useless - // stack trace - const auto newMaxWidth = std::max(0.0, widthToTerminalEnd); - TextBlock().MaxWidth(newMaxWidth); + TryRedrawCanvas(); // Set the text block bounds - const auto yOffset = ::base::ClampedNumeric(TextBlock().ActualHeight()) - fontHeight; - const auto textBottom = ::base::ClampedNumeric(screenCursorPos.Y) + yOffset; - Rect selectionRect = Rect(screenCursorPos.X, textBottom, 0, fontHeight); - request.LayoutBounds().TextBounds(ScaleRect(selectionRect, scaleFactor)); + request.LayoutBounds().TextBounds(_currentTextBounds); // Set the control bounds of the whole control - Rect controlRect = Rect(screenCursorPos.X, screenCursorPos.Y, 0, fontHeight); - request.LayoutBounds().ControlBounds(ScaleRect(controlRect, scaleFactor)); + request.LayoutBounds().ControlBounds(_currentControlBounds); } // Method Description: diff --git a/src/cascadia/TerminalControl/TSFInputControl.h b/src/cascadia/TerminalControl/TSFInputControl.h index f57925b2c..536bed5df 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.h +++ b/src/cascadia/TerminalControl/TSFInputControl.h @@ -37,6 +37,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void NotifyFocusEnter(); void NotifyFocusLeave(); void ClearBuffer(); + void TryRedrawCanvas(); void Close(); @@ -73,6 +74,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation bool _inComposition; size_t _activeTextStart; void _SendAndClearText(); + void _RedrawCanvas(); + bool _focused; + + til::point _currentTerminalCursorPos; + double _currentCanvasWidth; + double _currentTextBlockHeight; + winrt::Windows::Foundation::Rect _currentControlBounds; + winrt::Windows::Foundation::Rect _currentTextBounds; }; } namespace winrt::Microsoft::Terminal::TerminalControl::factory_implementation diff --git a/src/cascadia/TerminalControl/TSFInputControl.idl b/src/cascadia/TerminalControl/TSFInputControl.idl index 0722f6d5a..04ee78bb9 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.idl +++ b/src/cascadia/TerminalControl/TSFInputControl.idl @@ -28,6 +28,7 @@ namespace Microsoft.Terminal.TerminalControl void NotifyFocusEnter(); void NotifyFocusLeave(); void ClearBuffer(); + void TryRedrawCanvas(); void Close(); } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 51937ef82..7d5d0d76e 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -86,6 +86,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation auto pfnScrollPositionChanged = std::bind(&TermControl::_TerminalScrollPositionChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3); _terminal->SetScrollPositionChangedCallback(pfnScrollPositionChanged); + auto pfnTerminalCursorPositionChanged = std::bind(&TermControl::_TerminalCursorPositionChanged, this); + _terminal->SetCursorPositionChangedCallback(pfnTerminalCursorPositionChanged); + // This event is explicitly revoked in the destructor: does not need weak_ref auto onReceiveOutputFn = [this](const hstring str) { _terminal->Write(str); @@ -1793,6 +1796,31 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } } + // Method Description: + // - Tells TSFInputControl to redraw the Canvas/TextBlock so it'll update + // to be where the current cursor position is. + // Arguments: + // - N/A + winrt::fire_and_forget TermControl::_TerminalCursorPositionChanged() + { + if (_closing.load()) + { + return; + } + + auto weakThis{ get_weak() }; + + co_await winrt::resume_foreground(Dispatcher()); + + if (auto control{ weakThis.get() }) + { + if (!_closing.load()) + { + TSFInputControl().TryRedrawCanvas(); + } + } + } + hstring TermControl::Title() { hstring hstr{ _terminal->GetConsoleTitle() }; @@ -2243,8 +2271,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation return; } - const COORD cursorPos = _terminal->GetCursorPosition(); - Windows::Foundation::Point p = { gsl::narrow_cast(cursorPos.X), gsl::narrow_cast(cursorPos.Y) }; + const til::point cursorPos = _terminal->GetCursorPosition(); + Windows::Foundation::Point p = { ::base::ClampedNumeric(cursorPos.x()), ::base::ClampedNumeric(cursorPos.y()) }; eventArgs.CurrentPosition(p); } diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 9d365996c..c0cf91863 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -196,6 +196,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _DoResize(const double newWidth, const double newHeight); void _TerminalTitleChanged(const std::wstring_view& wstr); winrt::fire_and_forget _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize); + winrt::fire_and_forget _TerminalCursorPositionChanged(); void _MouseScrollHandler(const double delta, Windows::UI::Input::PointerPoint const& pointerPoint); void _MouseZoomHandler(const double delta); diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index b9d8ccf40..b3471ee68 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -704,6 +704,8 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) _buffer->GetRenderTarget().TriggerRedrawAll(); _NotifyScrollEvent(); } + + _NotifyTerminalCursorPositionChanged(); } void Terminal::UserScrollViewport(const int viewTop) @@ -736,6 +738,14 @@ try } CATCH_LOG() +void Terminal::_NotifyTerminalCursorPositionChanged() noexcept +{ + if (_pfnCursorPositionChanged) + { + _pfnCursorPositionChanged(); + } +} + void Terminal::SetWriteInputCallback(std::function pfn) noexcept { _pfnWriteInput.swap(pfn); @@ -751,6 +761,11 @@ void Terminal::SetScrollPositionChangedCallback(std::function pfn) noexcept +{ + _pfnCursorPositionChanged.swap(pfn); +} + // Method Description: // - Allows setting a callback for when the background color is changed // Arguments: diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 2317cee41..f5ca256e3 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -168,6 +168,7 @@ public: void SetWriteInputCallback(std::function pfn) noexcept; void SetTitleChangedCallback(std::function pfn) noexcept; void SetScrollPositionChangedCallback(std::function pfn) noexcept; + void SetCursorPositionChangedCallback(std::function pfn) noexcept; void SetBackgroundCallback(std::function pfn) noexcept; void SetCursorOn(const bool isOn) noexcept; @@ -194,6 +195,7 @@ private: std::function _pfnTitleChanged; std::function _pfnScrollPositionChanged; std::function _pfnBackgroundColorChanged; + std::function _pfnCursorPositionChanged; std::unique_ptr<::Microsoft::Console::VirtualTerminal::StateMachine> _stateMachine; std::unique_ptr<::Microsoft::Console::VirtualTerminal::TerminalInput> _terminalInput; @@ -266,6 +268,8 @@ private: void _NotifyScrollEvent() noexcept; + void _NotifyTerminalCursorPositionChanged() noexcept; + #pragma region TextSelection // These methods are defined in TerminalSelection.cpp std::vector _GetSelectionRects() const noexcept;