From 76e77deeccd93401999bd40f09bed91548f026ce Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 30 Jul 2021 17:46:42 -0700 Subject: [PATCH 01/10] Add selection marker overlays for keyboard selection --- src/cascadia/TerminalControl/ControlCore.cpp | 26 ++++++ src/cascadia/TerminalControl/ControlCore.h | 4 + src/cascadia/TerminalControl/ControlCore.idl | 4 + src/cascadia/TerminalControl/EventArgs.cpp | 1 + src/cascadia/TerminalControl/EventArgs.h | 13 +++ src/cascadia/TerminalControl/EventArgs.idl | 6 +- src/cascadia/TerminalControl/TermControl.cpp | 92 +++++++++++++++++-- src/cascadia/TerminalControl/TermControl.h | 3 + src/cascadia/TerminalControl/TermControl.xaml | 17 ++++ src/cascadia/TerminalCore/Terminal.hpp | 3 + .../TerminalCore/TerminalSelection.cpp | 45 +++++++-- 11 files changed, 197 insertions(+), 17 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index a224ca22c4d..5b538ce4755 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -409,6 +409,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto lock = _terminal->LockForWriting(); _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); return true; } @@ -417,6 +418,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->ClearSelection(); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // When there is a selection active, escape should clear it and NOT flow through @@ -912,6 +914,26 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->SetSelectionAnchor(position.to_win32_coord()); } + Core::Point ControlCore::SelectionAnchor() const + { + auto lock = _terminal->LockForReading(); + const auto start = _terminal->SelectionStartForRendering(); + return { start.X, start.Y }; + } + + Core::Point ControlCore::SelectionEnd() const + { + auto lock = _terminal->LockForReading(); + const auto end = _terminal->SelectionEndForRendering(); + return { end.X, end.Y }; + } + + bool ControlCore::MovingStart() const + { + auto lock = _terminal->LockForReading(); + return _terminal->MovingStart(); + } + // Method Description: // - Sets selection's end position to match supplied cursor position, e.g. while mouse dragging. // Arguments: @@ -935,6 +957,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // save location (for rendering) + render _terminal->SetSelectionEnd(terminalPosition.to_win32_coord()); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // Called when the Terminal wants to set something to the clipboard, i.e. @@ -997,6 +1020,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->ClearSelection(); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // send data up for clipboard @@ -1351,6 +1375,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->SetBlockSelection(false); search.Select(); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // Raise a FoundMatch event, which the control will use to notify @@ -1553,6 +1578,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } void ControlCore::AttachUiaEngine(::Microsoft::Console::Render::IRenderEngine* const pEngine) diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 9cf4355a6ce..59733bcb2f9 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -151,6 +151,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool HasSelection() const; bool CopyOnSelect() const; Windows::Foundation::Collections::IVector SelectedText(bool trimTrailingWhitespace) const; + Core::Point SelectionAnchor() const; + Core::Point SelectionEnd() const; + bool MovingStart() const; void SetSelectionAnchor(const til::point& position); void SetEndSelectionPoint(const til::point& position); @@ -208,6 +211,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation TYPED_EVENT(ReceivedOutput, IInspectable, IInspectable); TYPED_EVENT(FoundMatch, IInspectable, Control::FoundResultsArgs); TYPED_EVENT(ShowWindowChanged, IInspectable, Control::ShowWindowArgs); + TYPED_EVENT(UpdateSelectionMarkers, IInspectable, Control::UpdateSelectionMarkersEventArgs); // clang-format on private: diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index edcb9fb69b1..e80ffdbe3da 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -90,6 +90,9 @@ namespace Microsoft.Terminal.Control Boolean HasSelection { get; }; IVector SelectedText(Boolean trimTrailingWhitespace); + Microsoft.Terminal.Core.Point SelectionAnchor { get; }; + Microsoft.Terminal.Core.Point SelectionEnd { get; }; + Boolean MovingStart { get; }; String HoveredUriText { get; }; Windows.Foundation.IReference HoveredCell { get; }; @@ -125,6 +128,7 @@ namespace Microsoft.Terminal.Control event Windows.Foundation.TypedEventHandler ReceivedOutput; event Windows.Foundation.TypedEventHandler FoundMatch; event Windows.Foundation.TypedEventHandler ShowWindowChanged; + event Windows.Foundation.TypedEventHandler UpdateSelectionMarkers; }; } diff --git a/src/cascadia/TerminalControl/EventArgs.cpp b/src/cascadia/TerminalControl/EventArgs.cpp index 665ea17f448..9f9b41ac1f7 100644 --- a/src/cascadia/TerminalControl/EventArgs.cpp +++ b/src/cascadia/TerminalControl/EventArgs.cpp @@ -13,3 +13,4 @@ #include "TransparencyChangedEventArgs.g.cpp" #include "FoundResultsArgs.g.cpp" #include "ShowWindowArgs.g.cpp" +#include "UpdateSelectionMarkersEventArgs.g.cpp" diff --git a/src/cascadia/TerminalControl/EventArgs.h b/src/cascadia/TerminalControl/EventArgs.h index 0d7c6d4a005..edefb2b0045 100644 --- a/src/cascadia/TerminalControl/EventArgs.h +++ b/src/cascadia/TerminalControl/EventArgs.h @@ -13,6 +13,8 @@ #include "TransparencyChangedEventArgs.g.h" #include "FoundResultsArgs.g.h" #include "ShowWindowArgs.g.h" +#include "UpdateSelectionMarkersEventArgs.g.h" +#include "cppwinrt_utils.h" // TODO CARLOS: maybe we can remove this include? namespace winrt::Microsoft::Terminal::Control::implementation { @@ -157,4 +159,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation WINRT_PROPERTY(bool, ShowOrHide); }; + + struct UpdateSelectionMarkersEventArgs : public UpdateSelectionMarkersEventArgsT + { + public: + UpdateSelectionMarkersEventArgs(const bool clearMarkers) : + _ClearMarkers(clearMarkers) + { + } + + WINRT_PROPERTY(bool, ClearMarkers, false); + }; } diff --git a/src/cascadia/TerminalControl/EventArgs.idl b/src/cascadia/TerminalControl/EventArgs.idl index 62ed095c8f4..941384c5b05 100644 --- a/src/cascadia/TerminalControl/EventArgs.idl +++ b/src/cascadia/TerminalControl/EventArgs.idl @@ -69,7 +69,6 @@ namespace Microsoft.Terminal.Control Double Opacity { get; }; } - runtimeclass FoundResultsArgs { Boolean FoundMatch { get; }; @@ -79,4 +78,9 @@ namespace Microsoft.Terminal.Control { Boolean ShowOrHide { get; }; } + + runtimeclass UpdateSelectionMarkersEventArgs + { + Boolean ClearMarkers { get; }; + } } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 37e4ad8096e..da156b3c6ad 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -83,6 +83,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _core.RaiseNotice({ this, &TermControl::_coreRaisedNotice }); _core.HoveredHyperlinkChanged({ this, &TermControl::_hoveredHyperlinkChanged }); _core.FoundMatch({ this, &TermControl::_coreFoundMatch }); + _core.UpdateSelectionMarkers({ this, &TermControl::_updateSelectionMarkers }); _interactivity.OpenHyperlink({ this, &TermControl::_HyperlinkHandler }); _interactivity.ScrollPositionChanged({ this, &TermControl::_ScrollPositionChanged }); @@ -320,6 +321,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation // switch from a solid color brush to an acrylic one. _changeBackgroundColor(bg); + // Update selection markers + Windows::UI::Xaml::Media::SolidColorBrush selectionBackgroundBrush{ til::color{ newAppearance.SelectionBackground() } }; + SelectionStartIcon().Foreground(selectionBackgroundBrush); + SelectionEndIcon().Foreground(selectionBackgroundBrush); + // Set TSF Foreground Media::SolidColorBrush foregroundBrush{}; if (_core.Settings().UseBackgroundImageForWindow()) @@ -1786,6 +1792,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation update.newValue = args.ViewTop(); _updateScrollBar->Run(update); + _updateSelectionMarkers(nullptr, winrt::make(false)); } // Method Description: @@ -2685,8 +2692,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation _core.ClearHoveredCell(); } - winrt::fire_and_forget TermControl::_hoveredHyperlinkChanged(IInspectable sender, - IInspectable args) + winrt::fire_and_forget TermControl::_hoveredHyperlinkChanged(IInspectable /*sender*/, + IInspectable /*args*/) { auto weakThis{ get_weak() }; co_await wil::resume_foreground(Dispatcher()); @@ -2713,12 +2720,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation HyperlinkTooltipBorder().BorderThickness(newThickness); // Compute the location of the top left corner of the cell in DIPS - const til::size marginsInDips{ til::math::rounding, GetPadding().Left, GetPadding().Top }; - const til::point startPos{ lastHoveredCell.Value() }; - const til::size fontSize{ til::math::rounding, _core.FontSize() }; - const auto posInPixels{ startPos * fontSize }; - const til::point posInDIPs{ til::math::flooring, posInPixels.x / scale, posInPixels.y / scale }; - const auto locationInDIPs{ posInDIPs + marginsInDips }; + const til::point locationInDIPs{ _toPosInDips(lastHoveredCell.Value()) }; // Move the border to the top left corner of the cell OverlayCanvas().SetLeft(HyperlinkTooltipBorder(), locationInDIPs.x - offset.x); @@ -2728,10 +2730,84 @@ namespace winrt::Microsoft::Terminal::Control::implementation } } + winrt::fire_and_forget TermControl::_updateSelectionMarkers(IInspectable /*sender*/, Control::UpdateSelectionMarkersEventArgs args) + { + auto weakThis{ get_weak() }; + co_await resume_foreground(Dispatcher()); + if (weakThis.get() && args) + { + if (_core.HasSelection() && !args.ClearMarkers()) + { + // show/update selection markers + // figure out which endpoint to move, get it and the relevant icon (hide the other icon) + const auto movingStart{ _core.MovingStart() }; + const auto selectionAnchor{ movingStart ? _core.SelectionAnchor() : _core.SelectionEnd() }; + const auto& icon{ movingStart ? SelectionStartIcon() : SelectionEndIcon() }; + const auto& otherIcon{ movingStart ? SelectionEndIcon() : SelectionStartIcon() }; + if (selectionAnchor.Y < 0 || selectionAnchor.Y >= _core.ViewHeight()) + { + // if the endpoint is outside of the viewport, + // just hide the markers + icon.Opacity(0); + otherIcon.Opacity(0); + co_return; + } + else + { + icon.Opacity(1); + otherIcon.Opacity(0); + } + + // Compute the location of the top left corner of the cell in DIPS + const til::point locationInDIPs{ _toPosInDips(selectionAnchor) }; + + // Move the icon to the top left corner of the cell + SelectionCanvas().SetLeft(icon, + (locationInDIPs.x - SwapChainPanel().ActualOffset().x)); + SelectionCanvas().SetTop(icon, + (locationInDIPs.y - SwapChainPanel().ActualOffset().y)); + } + else + { + // hide selection markers + SelectionStartIcon().Opacity(0); + SelectionEndIcon().Opacity(0); + } + } + } + + til::point TermControl::_toPosInDips(const Core::Point terminalCellPos) + { + const til::point terminalPos{ terminalCellPos }; + const til::size marginsInDips{ til::math::rounding, GetPadding().Left, GetPadding().Top }; + const til::size fontSize{ til::math::rounding, _core.FontSize() }; + const til::point posInPixels{ terminalPos * fontSize }; + const auto scale{ SwapChainPanel().CompositionScaleX() }; + const til::point posInDIPs{ til::math::flooring, posInPixels.x / scale, posInPixels.y / scale }; + return posInDIPs + marginsInDips; + } + void TermControl::_coreFontSizeChanged(const int fontWidth, const int fontHeight, const bool isInitialChange) { + // scale the selection markers to be the size of a cell + auto scaleIconMarker = [fontWidth, fontHeight](Windows::UI::Xaml::Controls::FontIcon icon) { + const auto size{ icon.DesiredSize() }; + const auto scaleX = fontWidth / size.Width; + const auto scaleY = fontHeight / size.Height; + + Windows::UI::Xaml::Media::ScaleTransform transform{}; + transform.ScaleX(transform.ScaleX() * scaleX); + transform.ScaleY(transform.ScaleY() * scaleY); + icon.RenderTransform(transform); + + // now hide the icon + icon.Opacity(0); + }; + scaleIconMarker(SelectionStartIcon()); + scaleIconMarker(SelectionEndIcon()); + // Don't try to inspect the core here. The Core is raising this while // it's holding its write lock. If the handlers calls back to some // method on the TermControl on the same thread, and that _method_ calls diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index b8d70e7741f..a3c881fb62e 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -278,6 +278,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _FontInfoHandler(const IInspectable& sender, const FontInfoEventArgs& eventArgs); winrt::fire_and_forget _hoveredHyperlinkChanged(IInspectable sender, IInspectable args); + winrt::fire_and_forget _updateSelectionMarkers(IInspectable sender, Control::UpdateSelectionMarkersEventArgs args); void _coreFontSizeChanged(const int fontWidth, const int fontHeight, @@ -286,6 +287,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _coreRaisedNotice(const IInspectable& s, const Control::NoticeEventArgs& args); void _coreWarningBell(const IInspectable& sender, const IInspectable& args); void _coreFoundMatch(const IInspectable& sender, const Control::FoundResultsArgs& args); + + til::point _toPosInDips(const Core::Point terminalCellPos); }; } diff --git a/src/cascadia/TerminalControl/TermControl.xaml b/src/cascadia/TerminalControl/TermControl.xaml index 94afdb9c438..71f97be53b3 100644 --- a/src/cascadia/TerminalControl/TermControl.xaml +++ b/src/cascadia/TerminalControl/TermControl.xaml @@ -1213,6 +1213,23 @@ + + + + + + we're moving start endpoint ("higher") + // false --> we're moving end endpoint ("lower") + return _selection->start == _selection->pivot ? false : true; +} + // Method Description: // - updates the selection endpoints based on a direction and expansion mode. Primarily used for keyboard selection. // Arguments: @@ -318,10 +343,9 @@ Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion mode, ControlKeyStates mods) { // 1. Figure out which endpoint to update - // If we're in mark mode, shift dictates whether you are moving the end or not. - // Otherwise, we're updating an existing selection, so one of the endpoints is the pivot, - // signifying that the other endpoint is the one we want to move. - const auto movingEnd{ _markMode ? mods.IsShiftPressed() : _selection->start == _selection->pivot }; + // One of the endpoints is the pivot, + // signifying that the other endpoint is the one we want to move. + const auto movingEnd{ _selection->start == _selection->pivot }; auto targetPos{ movingEnd ? _selection->end : _selection->start }; // 2. Perform the movement @@ -346,12 +370,17 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion if (_markMode) { // [Mark Mode] - // - moveSelectionEnd --> just move end (i.e. shift + arrow keys) - // - !moveSelectionEnd --> move all three (i.e. just use arrow keys) - _selection->end = targetPos; - if (!movingEnd) + // - shift --> updating a standard selection + // - !shift --> move all three (i.e. just use arrow keys) + if (mods.IsShiftPressed()) + { + auto targetStart = false; + std::tie(_selection->start, _selection->end) = _PivotSelection(targetPos, targetStart); + } + else { _selection->start = targetPos; + _selection->end = targetPos; _selection->pivot = targetPos; } } From 214d3af27ff86cf9b4c3210a40e2cfc40cdecc94 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 2 Jun 2022 17:11:13 -0700 Subject: [PATCH 02/10] simplify logic a bit --- .../TerminalCore/TerminalSelection.cpp | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index cddf90f17fe..68b4a29b1e4 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -366,26 +366,17 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion } // 3. Actually modify the selection - // NOTE: targetStart doesn't matter here - if (_markMode) + if (_markMode && !mods.IsShiftPressed()) { - // [Mark Mode] - // - shift --> updating a standard selection - // - !shift --> move all three (i.e. just use arrow keys) - if (mods.IsShiftPressed()) - { - auto targetStart = false; - std::tie(_selection->start, _selection->end) = _PivotSelection(targetPos, targetStart); - } - else - { - _selection->start = targetPos; - _selection->end = targetPos; - _selection->pivot = targetPos; - } + // [Mark Mode] + shift unpressed --> move all three (i.e. just use arrow keys) + _selection->start = targetPos; + _selection->end = targetPos; + _selection->pivot = targetPos; } else { + // [Mark Mode] + shift --> updating a standard selection + // NOTE: targetStart doesn't matter here auto targetStart = false; std::tie(_selection->start, _selection->end) = _PivotSelection(targetPos, targetStart); } From f9baadc5514f18dccbfe32889ad06941020e90da Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 6 Jun 2022 14:25:34 -0700 Subject: [PATCH 03/10] add markers as paths; fix selectAll/toggleMarkMode --- src/cascadia/TerminalControl/ControlCore.cpp | 2 + src/cascadia/TerminalControl/TermControl.cpp | 45 ++++++++++--------- src/cascadia/TerminalControl/TermControl.xaml | 19 +++----- src/cascadia/TerminalControl/pch.h | 1 + 4 files changed, 32 insertions(+), 35 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 5b538ce4755..4e501711a96 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1037,6 +1037,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); } void ControlCore::ToggleMarkMode() @@ -1044,6 +1045,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto lock = _terminal->LockForWriting(); _terminal->ToggleMarkMode(); _renderer->TriggerSelection(); + _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); } bool ControlCore::IsInMarkMode() const diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index da156b3c6ad..fbfad36c7cb 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -323,8 +323,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Update selection markers Windows::UI::Xaml::Media::SolidColorBrush selectionBackgroundBrush{ til::color{ newAppearance.SelectionBackground() } }; - SelectionStartIcon().Foreground(selectionBackgroundBrush); - SelectionEndIcon().Foreground(selectionBackgroundBrush); + SelectionStartMarker().Fill(selectionBackgroundBrush); + SelectionEndMarker().Fill(selectionBackgroundBrush); // Set TSF Foreground Media::SolidColorBrush foregroundBrush{}; @@ -2742,36 +2742,36 @@ namespace winrt::Microsoft::Terminal::Control::implementation // figure out which endpoint to move, get it and the relevant icon (hide the other icon) const auto movingStart{ _core.MovingStart() }; const auto selectionAnchor{ movingStart ? _core.SelectionAnchor() : _core.SelectionEnd() }; - const auto& icon{ movingStart ? SelectionStartIcon() : SelectionEndIcon() }; - const auto& otherIcon{ movingStart ? SelectionEndIcon() : SelectionStartIcon() }; + const auto& marker{ movingStart ? SelectionStartMarker() : SelectionEndMarker() }; + const auto& otherMarker{ movingStart ? SelectionEndMarker() : SelectionStartMarker() }; if (selectionAnchor.Y < 0 || selectionAnchor.Y >= _core.ViewHeight()) { // if the endpoint is outside of the viewport, // just hide the markers - icon.Opacity(0); - otherIcon.Opacity(0); + marker.Visibility(Visibility::Collapsed); + otherMarker.Visibility(Visibility::Collapsed); co_return; } else { - icon.Opacity(1); - otherIcon.Opacity(0); + marker.Visibility(Visibility::Visible); + otherMarker.Visibility(Visibility::Collapsed); } // Compute the location of the top left corner of the cell in DIPS const til::point locationInDIPs{ _toPosInDips(selectionAnchor) }; - // Move the icon to the top left corner of the cell - SelectionCanvas().SetLeft(icon, + // Move the marker to the top left corner of the cell + SelectionCanvas().SetLeft(marker, (locationInDIPs.x - SwapChainPanel().ActualOffset().x)); - SelectionCanvas().SetTop(icon, + SelectionCanvas().SetTop(marker, (locationInDIPs.y - SwapChainPanel().ActualOffset().y)); } else { // hide selection markers - SelectionStartIcon().Opacity(0); - SelectionEndIcon().Opacity(0); + SelectionStartMarker().Visibility(Visibility::Collapsed); + SelectionEndMarker().Visibility(Visibility::Collapsed); } } } @@ -2792,21 +2792,22 @@ namespace winrt::Microsoft::Terminal::Control::implementation const bool isInitialChange) { // scale the selection markers to be the size of a cell - auto scaleIconMarker = [fontWidth, fontHeight](Windows::UI::Xaml::Controls::FontIcon icon) { - const auto size{ icon.DesiredSize() }; - const auto scaleX = fontWidth / size.Width; - const auto scaleY = fontHeight / size.Height; + auto scaleMarker = [fontWidth, fontHeight](Windows::UI::Xaml::Shapes::Path shape) { + // The selection markers were designed to be 5x14 in size, + // so use those dimensions below for the scaling + const auto scaleX = fontWidth / 5.0; + const auto scaleY = fontHeight / 14.0; Windows::UI::Xaml::Media::ScaleTransform transform{}; transform.ScaleX(transform.ScaleX() * scaleX); transform.ScaleY(transform.ScaleY() * scaleY); - icon.RenderTransform(transform); + shape.RenderTransform(transform); - // now hide the icon - icon.Opacity(0); + // now hide the shape + shape.Visibility(Visibility::Collapsed); }; - scaleIconMarker(SelectionStartIcon()); - scaleIconMarker(SelectionEndIcon()); + scaleMarker(SelectionStartMarker()); + scaleMarker(SelectionEndMarker()); // Don't try to inspect the core here. The Core is raising this while // it's holding its write lock. If the handlers calls back to some diff --git a/src/cascadia/TerminalControl/TermControl.xaml b/src/cascadia/TerminalControl/TermControl.xaml index 71f97be53b3..e5254b4bc72 100644 --- a/src/cascadia/TerminalControl/TermControl.xaml +++ b/src/cascadia/TerminalControl/TermControl.xaml @@ -1216,19 +1216,12 @@ - - - + + diff --git a/src/cascadia/TerminalControl/pch.h b/src/cascadia/TerminalControl/pch.h index 7d710c5b5c7..34e10473509 100644 --- a/src/cascadia/TerminalControl/pch.h +++ b/src/cascadia/TerminalControl/pch.h @@ -47,6 +47,7 @@ #include #include #include +#include #include #include From aa2ff6b658f5f84d87d59b4dfce26b1412a71093 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 7 Jun 2022 09:59:08 -0700 Subject: [PATCH 04/10] MovingStart-->MovingEnd --- src/cascadia/TerminalControl/ControlCore.cpp | 10 ++++- src/cascadia/TerminalControl/ControlCore.h | 3 +- src/cascadia/TerminalControl/ControlCore.idl | 3 +- src/cascadia/TerminalControl/TermControl.cpp | 43 +++++++++++++------ src/cascadia/TerminalCore/Terminal.hpp | 3 +- .../TerminalCore/TerminalSelection.cpp | 18 +++++--- 6 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 4e501711a96..8073c38e94a 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -928,10 +928,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation return { end.X, end.Y }; } - bool ControlCore::MovingStart() const + bool ControlCore::MovingEnd() const { auto lock = _terminal->LockForReading(); - return _terminal->MovingStart(); + return _terminal->MovingEnd(); + } + + bool ControlCore::MovingCursor() const + { + auto lock = _terminal->LockForReading(); + return _terminal->MovingCursor(); } // Method Description: diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 59733bcb2f9..e79d4dbdd5d 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -153,7 +153,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation Windows::Foundation::Collections::IVector SelectedText(bool trimTrailingWhitespace) const; Core::Point SelectionAnchor() const; Core::Point SelectionEnd() const; - bool MovingStart() const; + bool MovingEnd() const; + bool MovingCursor() const; void SetSelectionAnchor(const til::point& position); void SetEndSelectionPoint(const til::point& position); diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index e80ffdbe3da..77eca8873c5 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -92,7 +92,8 @@ namespace Microsoft.Terminal.Control IVector SelectedText(Boolean trimTrailingWhitespace); Microsoft.Terminal.Core.Point SelectionAnchor { get; }; Microsoft.Terminal.Core.Point SelectionEnd { get; }; - Boolean MovingStart { get; }; + Boolean MovingEnd { get; }; + Boolean MovingCursor { get; }; String HoveredUriText { get; }; Windows.Foundation.IReference HoveredCell { get; }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index fbfad36c7cb..a89f104a1bb 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2738,12 +2738,28 @@ namespace winrt::Microsoft::Terminal::Control::implementation { if (_core.HasSelection() && !args.ClearMarkers()) { + // lambda helper function that can be used to display a selection marker + // - targetEnd: if true, target the "end" selection marker. Otherwise, target "start". + auto displayMarker = [&](bool targetEnd) { + // Compute the location of the top left corner of the cell in DIPS + const auto terminalPos{ targetEnd ? _core.SelectionEnd() : _core.SelectionAnchor() }; + const til::point locationInDIPs{ _toPosInDips(terminalPos) }; + + // Move the marker to the top left corner of the cell + const auto& marker{ targetEnd ? SelectionEndMarker() : SelectionStartMarker() }; + SelectionCanvas().SetLeft(marker, + (locationInDIPs.x - SwapChainPanel().ActualOffset().x)); + SelectionCanvas().SetTop(marker, + (locationInDIPs.y - SwapChainPanel().ActualOffset().y)); + marker.Visibility(Visibility::Visible); + }; + // show/update selection markers // figure out which endpoint to move, get it and the relevant icon (hide the other icon) - const auto movingStart{ _core.MovingStart() }; - const auto selectionAnchor{ movingStart ? _core.SelectionAnchor() : _core.SelectionEnd() }; - const auto& marker{ movingStart ? SelectionStartMarker() : SelectionEndMarker() }; - const auto& otherMarker{ movingStart ? SelectionEndMarker() : SelectionStartMarker() }; + const auto movingEnd{ _core.MovingEnd() }; + const auto selectionAnchor{ movingEnd ? _core.SelectionEnd() : _core.SelectionAnchor() }; + const auto& marker{ movingEnd ? SelectionEndMarker() : SelectionStartMarker() }; + const auto& otherMarker{ movingEnd ? SelectionStartMarker() : SelectionEndMarker() }; if (selectionAnchor.Y < 0 || selectionAnchor.Y >= _core.ViewHeight()) { // if the endpoint is outside of the viewport, @@ -2752,20 +2768,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation otherMarker.Visibility(Visibility::Collapsed); co_return; } + else if (_core.MovingCursor()) + { + // display both markers + displayMarker(true); + displayMarker(false); + } else { - marker.Visibility(Visibility::Visible); + // display one marker, + // but hide the other + displayMarker(movingEnd); otherMarker.Visibility(Visibility::Collapsed); } - - // Compute the location of the top left corner of the cell in DIPS - const til::point locationInDIPs{ _toPosInDips(selectionAnchor) }; - - // Move the marker to the top left corner of the cell - SelectionCanvas().SetLeft(marker, - (locationInDIPs.x - SwapChainPanel().ActualOffset().x)); - SelectionCanvas().SetTop(marker, - (locationInDIPs.y - SwapChainPanel().ActualOffset().y)); } else { diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 6cbd3747453..2ce3e9064a5 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -248,7 +248,8 @@ class Microsoft::Terminal::Core::Terminal final : using UpdateSelectionParams = std::optional>; UpdateSelectionParams ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey) const; - bool MovingStart() const noexcept; + bool MovingEnd() const noexcept; + bool MovingCursor() const noexcept; til::point SelectionStartForRendering() const; til::point SelectionEndForRendering() const; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 68b4a29b1e4..6748997d28d 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -327,11 +327,18 @@ Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams return std::nullopt; } -bool Terminal::MovingStart() const noexcept +bool Terminal::MovingEnd() const noexcept { - // true --> we're moving start endpoint ("higher") - // false --> we're moving end endpoint ("lower") - return _selection->start == _selection->pivot ? false : true; + // true --> we're moving end endpoint ("lower") + // false --> we're moving start endpoint ("higher") + return _selection->start == _selection->pivot; +} + +bool Terminal::MovingCursor() const noexcept +{ + // true --> we're moving end endpoint ("lower") + // false --> we're moving start endpoint ("higher") + return _selection->start == _selection->pivot && _selection->pivot == _selection->end; } // Method Description: @@ -345,8 +352,7 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion // 1. Figure out which endpoint to update // One of the endpoints is the pivot, // signifying that the other endpoint is the one we want to move. - const auto movingEnd{ _selection->start == _selection->pivot }; - auto targetPos{ movingEnd ? _selection->end : _selection->start }; + auto targetPos{ MovingEnd() ? _selection->end : _selection->start }; // 2. Perform the movement switch (mode) From b6a11540fd67fbe87d8869b3ee7db997d674fc21 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 9 Jun 2022 16:12:16 -0700 Subject: [PATCH 05/10] address zadji's feedback --- src/cascadia/TerminalControl/ControlCore.cpp | 33 +++++++++----------- src/cascadia/TerminalControl/ControlCore.h | 1 + src/cascadia/TerminalControl/EventArgs.h | 1 - src/cascadia/TerminalControl/TermControl.cpp | 6 ++-- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index eedecc89785..241c10844da 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -399,7 +399,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); - _renderer->TriggerSelection(); + _updateSelection(false); return true; } @@ -408,8 +408,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); + _updateSelection(false); return true; } @@ -417,8 +416,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (!modifiers.IsWinPressed()) { _terminal->ClearSelection(); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); + _updateSelection(true); } // When there is a selection active, escape should clear it and NOT flow through @@ -962,8 +960,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // save location (for rendering) + render _terminal->SetSelectionEnd(terminalPosition); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); + _updateSelection(true); } // Called when the Terminal wants to set something to the clipboard, i.e. @@ -1025,8 +1022,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (!_settings->CopyOnSelect()) { _terminal->ClearSelection(); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); + _updateSelection(true); } // send data up for clipboard @@ -1042,8 +1038,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); + _updateSelection(false); } bool ControlCore::ToggleBlockSelection() @@ -1052,7 +1047,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (_terminal->IsSelectionActive()) { _terminal->SetBlockSelection(!_terminal->IsBlockSelection()); - _renderer->TriggerSelection(); + _updateSelection(false); return true; } return false; @@ -1062,8 +1057,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->ToggleMarkMode(); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(false)); + _updateSelection(false); } bool ControlCore::IsInMarkMode() const @@ -1078,7 +1072,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->WritePastedText(hstr); _terminal->ClearSelection(); - _renderer->TriggerSelection(); + _updateSelection(true); _terminal->TrySnapOnInput(); } @@ -1394,8 +1388,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->SetBlockSelection(false); search.Select(); - _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); + _updateSelection(true); } // Raise a FoundMatch event, which the control will use to notify @@ -1596,9 +1589,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->MultiClickSelection(terminalPosition, mode); selectionNeedsToBeCopied = true; } + _updateSelection(true); + } + void ControlCore::_updateSelection(const bool clearMarkers) + { _renderer->TriggerSelection(); - _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); + _UpdateSelectionMarkersHandlers(*this, winrt::make(clearMarkers)); } void ControlCore::AttachUiaEngine(::Microsoft::Console::Render::IRenderEngine* const pEngine) diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index e896c33b56f..d741d74148f 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -267,6 +267,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _setFontSizeUnderLock(int fontSize); void _updateFont(const bool initialUpdate = false); void _refreshSizeUnderLock(); + void _updateSelection(const bool clearMarkers); void _sendInputToConnection(std::wstring_view wstr); diff --git a/src/cascadia/TerminalControl/EventArgs.h b/src/cascadia/TerminalControl/EventArgs.h index edefb2b0045..a542e391f63 100644 --- a/src/cascadia/TerminalControl/EventArgs.h +++ b/src/cascadia/TerminalControl/EventArgs.h @@ -14,7 +14,6 @@ #include "FoundResultsArgs.g.h" #include "ShowWindowArgs.g.h" #include "UpdateSelectionMarkersEventArgs.g.h" -#include "cppwinrt_utils.h" // TODO CARLOS: maybe we can remove this include? namespace winrt::Microsoft::Terminal::Control::implementation { diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 81642a7dc92..dae4397c20f 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2818,9 +2818,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto scaleX = fontWidth / 5.0; const auto scaleY = fontHeight / 14.0; - Windows::UI::Xaml::Media::ScaleTransform transform{}; - transform.ScaleX(transform.ScaleX() * scaleX); - transform.ScaleY(transform.ScaleY() * scaleY); + Windows::UI::Xaml::Media::ScaleTransform transform; + transform.ScaleX(scaleX); + transform.ScaleY(scaleY); shape.RenderTransform(transform); // now hide the shape From 659230a34a2707ecd34c0be1950f9776acc20356 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 14 Jun 2022 17:06:13 -0700 Subject: [PATCH 06/10] address Dustin's feedback --- src/cascadia/TerminalControl/ControlCore.cpp | 73 +++++++++++-------- src/cascadia/TerminalControl/ControlCore.h | 7 +- src/cascadia/TerminalControl/ControlCore.idl | 15 +++- src/cascadia/TerminalControl/TermControl.cpp | 37 +++++++--- .../TerminalCore/TerminalSelection.cpp | 11 ++- 5 files changed, 90 insertions(+), 53 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 0ca8383af4e..629702142b2 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -421,7 +421,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); - _updateSelection(false); + _updateSelection(); return true; } @@ -430,7 +430,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers); - _updateSelection(false); + _updateSelection(); return true; } @@ -438,7 +438,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (!modifiers.IsWinPressed()) { _terminal->ClearSelection(); - _updateSelection(true); + _updateSelection(); } // When there is a selection active, escape should clear it and NOT flow through @@ -934,30 +934,29 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->SetSelectionAnchor(position); } - Core::Point ControlCore::SelectionAnchor() const + // Method Description: + // - Retrieves selection metadata from Terminal necessary to draw the + // selection markers. + // - Since all of this needs to be done under lock, it is more performant + // to throw it all in a struct and pass it along. + Control::SelectionMarkerMetadata ControlCore::SelectionMarkerInfo() const { auto lock = _terminal->LockForReading(); - const auto start = _terminal->SelectionStartForRendering(); - return { start.X, start.Y }; - } + Control::SelectionMarkerMetadata info; - Core::Point ControlCore::SelectionEnd() const - { - auto lock = _terminal->LockForReading(); - const auto end = _terminal->SelectionEndForRendering(); - return { end.X, end.Y }; - } + const auto start{ _terminal->SelectionStartForRendering() }; + info.StartPos = { start.X, start.Y }; - bool ControlCore::MovingEnd() const - { - auto lock = _terminal->LockForReading(); - return _terminal->MovingEnd(); - } + const auto end{ _terminal->SelectionEndForRendering() }; + info.EndPos = { end.X, end.Y }; - bool ControlCore::MovingCursor() const - { - auto lock = _terminal->LockForReading(); - return _terminal->MovingCursor(); + info.MovingEnd = _terminal->MovingEnd(); + info.MovingCursor = _terminal->MovingCursor(); + + const auto bufferSize{ _terminal->GetTextBuffer().GetSize() }; + info.FlipStartMarker = _terminal->GetSelectionAnchor() == bufferSize.Origin(); + info.FlipEndMarker = _terminal->GetSelectionEnd() == bufferSize.BottomRightInclusive(); + return info; } // Method Description: @@ -982,7 +981,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation // save location (for rendering) + render _terminal->SetSelectionEnd(terminalPosition); - _updateSelection(true); + _renderer->TriggerSelection(); + + // this is used for mouse dragging, + // so hide the markers + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // Called when the Terminal wants to set something to the clipboard, i.e. @@ -1044,7 +1047,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (!_settings->CopyOnSelect()) { _terminal->ClearSelection(); - _updateSelection(true); + _updateSelection(); } // send data up for clipboard @@ -1060,7 +1063,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); - _updateSelection(false); + _updateSelection(); } bool ControlCore::ToggleBlockSelection() @@ -1069,7 +1072,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (_terminal->IsSelectionActive()) { _terminal->SetBlockSelection(!_terminal->IsBlockSelection()); - _updateSelection(false); + _renderer->TriggerSelection(); + // do not update the selection markers! + // if we were showing them, keep it that way. + // otherwise, continue to not show them return true; } return false; @@ -1079,7 +1085,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { auto lock = _terminal->LockForWriting(); _terminal->ToggleMarkMode(); - _updateSelection(false); + _updateSelection(); } bool ControlCore::IsInMarkMode() const @@ -1094,7 +1100,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->WritePastedText(hstr); _terminal->ClearSelection(); - _updateSelection(true); + _updateSelection(); _terminal->TrySnapOnInput(); } @@ -1414,7 +1420,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _terminal->SetBlockSelection(false); search.Select(); - _updateSelection(true); + _renderer->TriggerSelection(); + + // this is used for search, + // so hide the markers + _UpdateSelectionMarkersHandlers(*this, winrt::make(true)); } // Raise a FoundMatch event, which the control will use to notify @@ -1615,12 +1625,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation _terminal->MultiClickSelection(terminalPosition, mode); selectionNeedsToBeCopied = true; } - _updateSelection(true); + _updateSelection(); } - void ControlCore::_updateSelection(const bool clearMarkers) + void ControlCore::_updateSelection() { _renderer->TriggerSelection(); + const bool clearMarkers{ !_terminal->IsSelectionActive() }; _UpdateSelectionMarkersHandlers(*this, winrt::make(clearMarkers)); } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 26e0daf2bea..b4017b08806 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -159,10 +159,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool HasSelection() const; bool CopyOnSelect() const; Windows::Foundation::Collections::IVector SelectedText(bool trimTrailingWhitespace) const; - Core::Point SelectionAnchor() const; - Core::Point SelectionEnd() const; - bool MovingEnd() const; - bool MovingCursor() const; + Control::SelectionMarkerMetadata SelectionMarkerInfo() const; void SetSelectionAnchor(const til::point position); void SetEndSelectionPoint(const til::point position); @@ -274,7 +271,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _setFontSizeUnderLock(int fontSize); void _updateFont(const bool initialUpdate = false); void _refreshSizeUnderLock(); - void _updateSelection(const bool clearMarkers); + void _updateSelection(); void _sendInputToConnection(std::wstring_view wstr); diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index 2015b099eb3..82bd902c52c 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -29,6 +29,16 @@ namespace Microsoft.Terminal.Control All }; + struct SelectionMarkerMetadata + { + Microsoft.Terminal.Core.Point StartPos; + Microsoft.Terminal.Core.Point EndPos; + Boolean MovingEnd; + Boolean MovingCursor; + Boolean FlipStartMarker; + Boolean FlipEndMarker; + }; + [default_interface] runtimeclass ControlCore : ICoreState { ControlCore(IControlSettings settings, @@ -90,10 +100,7 @@ namespace Microsoft.Terminal.Control Boolean HasSelection { get; }; IVector SelectedText(Boolean trimTrailingWhitespace); - Microsoft.Terminal.Core.Point SelectionAnchor { get; }; - Microsoft.Terminal.Core.Point SelectionEnd { get; }; - Boolean MovingEnd { get; }; - Boolean MovingCursor { get; }; + SelectionMarkerMetadata SelectionMarkerInfo { get; }; String HoveredUriText { get; }; Windows.Foundation.IReference HoveredCell { get; }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 5eb9e8824e8..2e0bfe41542 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2788,15 +2788,31 @@ namespace winrt::Microsoft::Terminal::Control::implementation { if (_core.HasSelection() && !args.ClearMarkers()) { + // retrieve all of the necessary selection marker data + // from the TerminalCore layer under one lock to improve performance + const auto markerData{ _core.SelectionMarkerInfo() }; + // lambda helper function that can be used to display a selection marker // - targetEnd: if true, target the "end" selection marker. Otherwise, target "start". auto displayMarker = [&](bool targetEnd) { + const auto flipMarker{ targetEnd ? markerData.FlipEndMarker : markerData.FlipStartMarker }; + const auto& marker{ targetEnd ? SelectionEndMarker() : SelectionStartMarker() }; + + // Ensure the marker is oriented properly + // (i.e. if start is at the beginning of the buffer, it should be flipped) + auto transform{ marker.RenderTransform().as() }; + transform.ScaleX(std::abs(transform.ScaleX()) * (flipMarker ? -1.0 : 1.0)); + marker.RenderTransform(transform); + // Compute the location of the top left corner of the cell in DIPS - const auto terminalPos{ targetEnd ? _core.SelectionEnd() : _core.SelectionAnchor() }; + auto terminalPos{ targetEnd ? markerData.EndPos : markerData.StartPos }; + if (flipMarker) + { + terminalPos.X += targetEnd ? -1 : 1; + } const til::point locationInDIPs{ _toPosInDips(terminalPos) }; // Move the marker to the top left corner of the cell - const auto& marker{ targetEnd ? SelectionEndMarker() : SelectionStartMarker() }; SelectionCanvas().SetLeft(marker, (locationInDIPs.x - SwapChainPanel().ActualOffset().x)); SelectionCanvas().SetTop(marker, @@ -2806,10 +2822,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation // show/update selection markers // figure out which endpoint to move, get it and the relevant icon (hide the other icon) - const auto movingEnd{ _core.MovingEnd() }; - const auto selectionAnchor{ movingEnd ? _core.SelectionEnd() : _core.SelectionAnchor() }; - const auto& marker{ movingEnd ? SelectionEndMarker() : SelectionStartMarker() }; - const auto& otherMarker{ movingEnd ? SelectionStartMarker() : SelectionEndMarker() }; + const auto selectionAnchor{ markerData.MovingEnd ? markerData.EndPos : markerData.StartPos }; + const auto& marker{ markerData.MovingEnd ? SelectionEndMarker() : SelectionStartMarker() }; + const auto& otherMarker{ markerData.MovingEnd ? SelectionStartMarker() : SelectionEndMarker() }; if (selectionAnchor.Y < 0 || selectionAnchor.Y >= _core.ViewHeight()) { // if the endpoint is outside of the viewport, @@ -2818,7 +2833,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation otherMarker.Visibility(Visibility::Collapsed); co_return; } - else if (_core.MovingCursor()) + else if (markerData.MovingCursor) { // display both markers displayMarker(true); @@ -2828,7 +2843,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // display one marker, // but hide the other - displayMarker(movingEnd); + displayMarker(markerData.MovingEnd); otherMarker.Visibility(Visibility::Collapsed); } } @@ -2857,11 +2872,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation const bool isInitialChange) { // scale the selection markers to be the size of a cell - auto scaleMarker = [fontWidth, fontHeight](Windows::UI::Xaml::Shapes::Path shape) { + auto scaleMarker = [fontWidth, fontHeight, dpiScale{ SwapChainPanel().CompositionScaleX() }](const Windows::UI::Xaml::Shapes::Path& shape) { // The selection markers were designed to be 5x14 in size, // so use those dimensions below for the scaling - const auto scaleX = fontWidth / 5.0; - const auto scaleY = fontHeight / 14.0; + const auto scaleX = fontWidth / 5.0 / dpiScale; + const auto scaleY = fontHeight / 14.0 / dpiScale; Windows::UI::Xaml::Media::ScaleTransform transform; transform.ScaleX(scaleX); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index c77f3f7b8f1..22ff09ee564 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -82,6 +82,9 @@ const til::point Terminal::GetSelectionEnd() const noexcept return _selection->end; } +// Method Description: +// - Gets the viewport-relative position of where to draw the marker +// for the selection's start endpoint til::point Terminal::SelectionStartForRendering() const { auto pos{ _selection->start }; @@ -91,6 +94,9 @@ til::point Terminal::SelectionStartForRendering() const return til::point{ pos }; } +// Method Description: +// - Gets the viewport-relative position of where to draw the marker +// for the selection's end endpoint til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; @@ -337,8 +343,9 @@ bool Terminal::MovingEnd() const noexcept bool Terminal::MovingCursor() const noexcept { - // true --> we're moving end endpoint ("lower") - // false --> we're moving start endpoint ("higher") + // Relevant for keyboard selection: + // true --> the selection is just a "cursor"; we're moving everything together with arrow keys + // false --> otherwise return _selection->start == _selection->pivot && _selection->pivot == _selection->end; } From 2007e180fdea24d220758258a095b18e3ed07c19 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 15 Jun 2022 14:24:33 -0700 Subject: [PATCH 07/10] flip markers at side boundaries --- src/cascadia/TerminalControl/ControlCore.cpp | 4 ++-- src/cascadia/TerminalControl/TermControl.cpp | 4 +++- .../TerminalCore/TerminalSelection.cpp | 18 ++++++++++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 629702142b2..b81800592fc 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -954,8 +954,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation info.MovingCursor = _terminal->MovingCursor(); const auto bufferSize{ _terminal->GetTextBuffer().GetSize() }; - info.FlipStartMarker = _terminal->GetSelectionAnchor() == bufferSize.Origin(); - info.FlipEndMarker = _terminal->GetSelectionEnd() == bufferSize.BottomRightInclusive(); + info.FlipStartMarker = _terminal->GetSelectionAnchor().x == bufferSize.Left(); + info.FlipEndMarker = _terminal->GetSelectionEnd().x == bufferSize.RightInclusive(); return info; } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 2e0bfe41542..01209c683b9 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2808,7 +2808,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto terminalPos{ targetEnd ? markerData.EndPos : markerData.StartPos }; if (flipMarker) { - terminalPos.X += targetEnd ? -1 : 1; + // When we flip the marker, a negative scaling makes us be one cell-width to the left. + // Add one to the viewport pos' x-coord to fix that. + terminalPos.X += 1; } const til::point locationInDIPs{ _toPosInDips(terminalPos) }; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 22ff09ee564..98729c789b1 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -89,7 +89,14 @@ til::point Terminal::SelectionStartForRendering() const { auto pos{ _selection->start }; const auto bufferSize{ GetTextBuffer().GetSize() }; - bufferSize.DecrementInBounds(pos); + if (pos.x != bufferSize.Left()) + { + // In general, we need to draw the marker one before the + // beginning of the selection. + // When we're at the left boundary, we want to + // flip the marker, so we skip this step. + bufferSize.DecrementInBounds(pos); + } pos.Y = base::ClampSub(pos.Y, _VisibleStartIndex()); return til::point{ pos }; } @@ -101,7 +108,14 @@ til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; const auto bufferSize{ GetTextBuffer().GetSize() }; - bufferSize.IncrementInBounds(pos); + if (pos.x != bufferSize.RightInclusive()) + { + // In general, we need to draw the marker one after the + // end of the selection. + // When we're at the right boundary, we want to + // flip the marker, so we skip this step. + bufferSize.IncrementInBounds(pos); + } pos.Y = base::ClampSub(pos.Y, _VisibleStartIndex()); return til::point{ pos }; } From 4623371334591a89826628cd41212ba3a91906c2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 16 Jun 2022 13:40:09 -0700 Subject: [PATCH 08/10] fix unfocused appearance bug --- src/cascadia/TerminalControl/TermControl.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 01209c683b9..2ca6a972781 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1837,7 +1837,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation update.newValue = args.ViewTop(); _updateScrollBar->Run(update); - _updateSelectionMarkers(nullptr, winrt::make(false)); + + // if a selection marker is already visible, + // update the position of those markers + if (SelectionStartMarker().Visibility() == Visibility::Visible || SelectionEndMarker().Visibility() == Visibility::Visible) + { + _updateSelectionMarkers(nullptr, winrt::make(false)); + } } // Method Description: From 5eb17173c26551937a12573e22ef2846912e8409 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 17 Jun 2022 11:28:36 -0700 Subject: [PATCH 09/10] update names for marker data --- src/cascadia/TerminalControl/ControlCore.cpp | 8 ++++---- src/cascadia/TerminalControl/ControlCore.h | 2 +- src/cascadia/TerminalControl/ControlCore.idl | 8 ++++---- src/cascadia/TerminalControl/TermControl.cpp | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index b81800592fc..1e60af76d3b 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -939,10 +939,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation // selection markers. // - Since all of this needs to be done under lock, it is more performant // to throw it all in a struct and pass it along. - Control::SelectionMarkerMetadata ControlCore::SelectionMarkerInfo() const + Control::SelectionData ControlCore::SelectionInfo() const { auto lock = _terminal->LockForReading(); - Control::SelectionMarkerMetadata info; + Control::SelectionData info; const auto start{ _terminal->SelectionStartForRendering() }; info.StartPos = { start.X, start.Y }; @@ -954,8 +954,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation info.MovingCursor = _terminal->MovingCursor(); const auto bufferSize{ _terminal->GetTextBuffer().GetSize() }; - info.FlipStartMarker = _terminal->GetSelectionAnchor().x == bufferSize.Left(); - info.FlipEndMarker = _terminal->GetSelectionEnd().x == bufferSize.RightInclusive(); + info.StartAtLeftBoundary = _terminal->GetSelectionAnchor().x == bufferSize.Left(); + info.EndAtRightBoundary = _terminal->GetSelectionEnd().x == bufferSize.RightInclusive(); return info; } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index b4017b08806..e11bd19772a 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -159,7 +159,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool HasSelection() const; bool CopyOnSelect() const; Windows::Foundation::Collections::IVector SelectedText(bool trimTrailingWhitespace) const; - Control::SelectionMarkerMetadata SelectionMarkerInfo() const; + Control::SelectionData SelectionInfo() const; void SetSelectionAnchor(const til::point position); void SetEndSelectionPoint(const til::point position); diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index 82bd902c52c..1dc39842f97 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -29,14 +29,14 @@ namespace Microsoft.Terminal.Control All }; - struct SelectionMarkerMetadata + struct SelectionData { Microsoft.Terminal.Core.Point StartPos; Microsoft.Terminal.Core.Point EndPos; Boolean MovingEnd; Boolean MovingCursor; - Boolean FlipStartMarker; - Boolean FlipEndMarker; + Boolean StartAtLeftBoundary; + Boolean EndAtRightBoundary; }; [default_interface] runtimeclass ControlCore : ICoreState @@ -100,7 +100,7 @@ namespace Microsoft.Terminal.Control Boolean HasSelection { get; }; IVector SelectedText(Boolean trimTrailingWhitespace); - SelectionMarkerMetadata SelectionMarkerInfo { get; }; + SelectionData SelectionInfo { get; }; String HoveredUriText { get; }; Windows.Foundation.IReference HoveredCell { get; }; diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 2ca6a972781..be3c8ad0cab 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -2796,12 +2796,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // retrieve all of the necessary selection marker data // from the TerminalCore layer under one lock to improve performance - const auto markerData{ _core.SelectionMarkerInfo() }; + const auto markerData{ _core.SelectionInfo() }; // lambda helper function that can be used to display a selection marker // - targetEnd: if true, target the "end" selection marker. Otherwise, target "start". auto displayMarker = [&](bool targetEnd) { - const auto flipMarker{ targetEnd ? markerData.FlipEndMarker : markerData.FlipStartMarker }; + const auto flipMarker{ targetEnd ? markerData.EndAtRightBoundary : markerData.StartAtLeftBoundary }; const auto& marker{ targetEnd ? SelectionEndMarker() : SelectionStartMarker() }; // Ensure the marker is oriented properly From 326dadf2b8f6e1290b4e8e09482952fd5a875798 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 17 Jun 2022 12:24:06 -0700 Subject: [PATCH 10/10] change marker color to cursor color --- src/cascadia/TerminalControl/TermControl.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index be3c8ad0cab..fb44871fdd6 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -360,9 +360,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation _changeBackgroundColor(bg); // Update selection markers - Windows::UI::Xaml::Media::SolidColorBrush selectionBackgroundBrush{ til::color{ newAppearance.SelectionBackground() } }; - SelectionStartMarker().Fill(selectionBackgroundBrush); - SelectionEndMarker().Fill(selectionBackgroundBrush); + Windows::UI::Xaml::Media::SolidColorBrush cursorColorBrush{ til::color{ newAppearance.CursorColor() } }; + SelectionStartMarker().Fill(cursorColorBrush); + SelectionEndMarker().Fill(cursorColorBrush); // Set TSF Foreground Media::SolidColorBrush foregroundBrush{};