From 23f2f6551aab71d8dc931db4716bd4aafdf6e76e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 30 Jun 2022 11:17:27 -0700 Subject: [PATCH 1/7] Add keyboard navigation to hyperlinks --- src/cascadia/TerminalControl/ControlCore.cpp | 27 ++- src/cascadia/TerminalControl/ControlCore.h | 1 + src/cascadia/TerminalControl/ControlCore.idl | 2 +- src/cascadia/TerminalControl/TermControl.cpp | 1 + src/cascadia/TerminalCore/Terminal.cpp | 1 + src/cascadia/TerminalCore/Terminal.hpp | 4 + .../TerminalCore/TerminalSelection.cpp | 167 ++++++++++++++++++ .../TerminalSettingsModel/defaults.json | 1 - 8 files changed, 197 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index d60f90bd380..072e426e2c5 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -418,12 +418,29 @@ namespace winrt::Microsoft::Terminal::Control::implementation keyDown) { const auto isInMarkMode = _terminal->SelectionMode() == ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Mark; - if (isInMarkMode && modifiers.IsCtrlPressed() && vkey == 'A') + if (isInMarkMode) { - auto lock = _terminal->LockForWriting(); - _terminal->SelectAll(); - _updateSelectionUI(); - return true; + if (modifiers.IsCtrlPressed() && vkey == 'A') + { + auto lock = _terminal->LockForWriting(); + _terminal->SelectAll(); + _updateSelectionUI(); + return true; + } + else if (_settings->DetectURLs() && vkey == VK_TAB) + { + auto lock = _terminal->LockForWriting(); + _terminal->SelectHyperlink(!modifiers.IsShiftPressed()); + _updateSelectionUI(); + return true; + } + else if (_terminal->IsTargetingUrl() && vkey == VK_RETURN) + { + auto lock = _terminal->LockForReading(); + const auto uri = _terminal->GetHyperlinkAtPosition(_terminal->GetSelectionAnchor()); + _OpenHyperlinkHandlers(*this, winrt::make(winrt::hstring{ uri })); + return true; + } } // try to update the selection diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 4208798e10d..e4209318e7f 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -218,6 +218,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation TYPED_EVENT(FoundMatch, IInspectable, Control::FoundResultsArgs); TYPED_EVENT(ShowWindowChanged, IInspectable, Control::ShowWindowArgs); TYPED_EVENT(UpdateSelectionMarkers, IInspectable, Control::UpdateSelectionMarkersEventArgs); + TYPED_EVENT(OpenHyperlink, IInspectable, Control::OpenHyperlinkEventArgs); // clang-format on private: diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl index 1d7801bdad4..ec8230fa180 100644 --- a/src/cascadia/TerminalControl/ControlCore.idl +++ b/src/cascadia/TerminalControl/ControlCore.idl @@ -153,6 +153,6 @@ namespace Microsoft.Terminal.Control event Windows.Foundation.TypedEventHandler FoundMatch; event Windows.Foundation.TypedEventHandler ShowWindowChanged; event Windows.Foundation.TypedEventHandler UpdateSelectionMarkers; - + event Windows.Foundation.TypedEventHandler OpenHyperlink; }; } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 7a1b57df3a6..731ff14e68a 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -84,6 +84,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _core.HoveredHyperlinkChanged({ this, &TermControl::_hoveredHyperlinkChanged }); _core.FoundMatch({ this, &TermControl::_coreFoundMatch }); _core.UpdateSelectionMarkers({ this, &TermControl::_updateSelectionMarkers }); + _core.OpenHyperlink({ this, &TermControl::_HyperlinkHandler }); _interactivity.OpenHyperlink({ this, &TermControl::_HyperlinkHandler }); _interactivity.ScrollPositionChanged({ this, &TermControl::_ScrollPositionChanged }); diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index d41e4f53b65..9aa946cc706 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -46,6 +46,7 @@ Terminal::Terminal() : _altGrAliasing{ true }, _blockSelection{ false }, _selectionMode{ SelectionInteractionMode::None }, + _isTargetingUrl{ false }, _selection{ std::nullopt }, _selectionEndpoint{ static_cast(0) }, _anchorInactiveSelectionEndpoint{ false }, diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index c63eb887b65..4897310f74a 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -273,6 +273,8 @@ class Microsoft::Terminal::Core::Terminal final : SelectionInteractionMode SelectionMode() const noexcept; void SwitchSelectionEndpoint(); void ToggleMarkMode(); + void SelectHyperlink(const bool movingForward); + bool IsTargetingUrl() const noexcept; using UpdateSelectionParams = std::optional>; UpdateSelectionParams ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey) const; @@ -349,6 +351,7 @@ class Microsoft::Terminal::Core::Terminal final : std::wstring _wordDelimiters; SelectionExpansion _multiClickSelectionMode; SelectionInteractionMode _selectionMode; + bool _isTargetingUrl; SelectionEndpoint _selectionEndpoint; bool _anchorInactiveSelectionEndpoint; #pragma endregion @@ -424,6 +427,7 @@ class Microsoft::Terminal::Core::Terminal final : std::pair _PivotSelection(const til::point targetPos, bool& targetStart) const; std::pair _ExpandSelectionAnchors(std::pair anchors) const; til::point _ConvertToBufferCell(const til::point viewportPos) const; + til::point _ConvertToViewportCell(const til::point viewportPos) const; void _MoveByChar(SelectionDirection direction, til::point& pos); void _MoveByWord(SelectionDirection direction, til::point& pos); void _MoveByViewport(SelectionDirection direction, til::point& pos); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 007a26b0c3f..1c1d0f06bcf 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -305,6 +305,7 @@ void Terminal::ToggleMarkMode() _selection->pivot = cursorPos; _selectionMode = SelectionInteractionMode::Mark; _blockSelection = false; + _isTargetingUrl = false; WI_SetAllFlags(_selectionEndpoint, SelectionEndpoint::Start | SelectionEndpoint::End); } } @@ -336,6 +337,157 @@ void Terminal::SwitchSelectionEndpoint() } } +// Method Description: +// - selects the next/previous hyperlink, if one is available +// Arguments: +// - searchForward: if true, we're scanning forward towards the end of the buffer. Otherwise, we're scanning backwards towards the top. +// Return Value: +// - true if we found a hyperlink to select (and selected it). False otherwise. +void Terminal::SelectHyperlink(const bool searchForward) +{ + if (!_markMode) + { + // This feature only works in mark mode + _isTargetingUrl = false; + return; + } + + // 0. Useful tools/vars + const auto bufferSize = _activeBuffer().GetSize(); + const auto viewportHeight = _GetMutableViewport().Height(); + + // extracts the next/previous hyperlink from the list of hyperlink ranges provided + auto extractResultFromList = [&, onUrl = _isTargetingUrl, selectionStart = _selection->start](const std::vector>& list, bool forward, bool useViewportCoords) { + // checks if we're already on the hyperlink and if we're trying to set it to the same one + auto isAlreadySelected = [onUrl, selectionStartForValidation = useViewportCoords ? _ConvertToViewportCell(selectionStart) : selectionStart](const interval_tree::Interval range) { + return onUrl && selectionStartForValidation == range.start; + }; + + std::optional> resultFromList; + if (list.size() == 1) + { + if (!isAlreadySelected(list.front())) + { + resultFromList = std::make_pair(list.front().start, list.front().stop); + } + } + else if (list.size() > 1) + { + // multiple results, find the first one in the direction we're going + if (forward) + { + for (const auto& range : list) + { + if (isAlreadySelected(range)) + { + continue; + } + else if (!resultFromList || range.start < resultFromList->first) + { + resultFromList = std::make_pair(range.start, range.stop); + } + } + } + else + { + for (const auto& range : list) + { + if (isAlreadySelected(range)) + { + continue; + } + else if (!resultFromList || range.start > resultFromList->first) + { + resultFromList = std::make_pair(range.start, range.stop); + } + } + } + } + + // useViewportCoords --> pattern tree stores everything as viewport coords, + // so we need to convert them on the way out + if (useViewportCoords && resultFromList) + { + resultFromList->first = _ConvertToBufferCell(resultFromList->first); + resultFromList->second = _ConvertToBufferCell(resultFromList->second); + } + return resultFromList; + }; + + // 1. Look for the hyperlink + til::point searchStart = searchForward ? _selection->start : til::point{ bufferSize.Left(), _VisibleStartIndex() }; + til::point searchEnd = searchForward ? til::point{ bufferSize.RightInclusive(), _VisibleEndIndex() } : _selection->start; + + // 1.A) Try searching the current viewport (no scrolling required) + auto patterns = _patternIntervalTree; + auto resultList = patterns.findContained(_ConvertToViewportCell(searchStart), _ConvertToViewportCell(searchEnd)); + std::optional> result = extractResultFromList(resultList, searchForward, true); + if (!result) + { + // 1.B) Incrementally search through more of the space + if (searchForward) + { + searchStart = { bufferSize.Left(), searchEnd.y + 1 }; + searchEnd = { bufferSize.RightInclusive(), std::min(searchStart.y + viewportHeight, ViewEndIndex()) }; + } + else + { + searchEnd = { bufferSize.RightInclusive(), searchStart.y - 1 }; + searchStart = { bufferSize.Left(), std::max(searchStart.y - viewportHeight, bufferSize.Top()) }; + } + const til::point bufferStart{ bufferSize.Origin() }; + const til::point bufferEnd{ bufferSize.RightInclusive(), ViewEndIndex() }; + while (!result && bufferSize.IsInBounds(searchStart) && bufferSize.IsInBounds(searchEnd) && searchStart <= searchEnd && bufferStart <= searchStart && searchEnd <= bufferEnd) + { + patterns = _activeBuffer().GetPatterns(searchStart.y, searchEnd.y); + resultList = patterns.findContained(searchStart, searchEnd); + result = extractResultFromList(resultList, searchForward, false); + if (!result) + { + searchStart.y += 1; + searchEnd.y = std::min(searchStart.y + viewportHeight, ViewEndIndex()); + } + } + + // 1.C) Nothing was found. Bail! + if (!result.has_value()) + { + return; + } + } + + // 2. Select the hyperlink + _selection->start = result->first; + _selection->pivot = result->first; + _selection->end = result->second; + bufferSize.DecrementInBounds(_selection->end); + _isTargetingUrl = true; + + // 3. Scroll to the selected area (if necessary) + // TODO CARLOS: I stole this from UpdateSelection(). I should probably de-duplicate it. + if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInBounds(_selection->end)) + { + if (const auto amtAboveView = visibleViewport.Top() - _selection->end.Y; amtAboveView > 0) + { + // anchor is above visible viewport, scroll by that amount + _scrollOffset += amtAboveView; + } + else + { + // anchor is below visible viewport, scroll by that amount + const auto amtBelowView = _selection->end.Y - visibleViewport.BottomInclusive(); + _scrollOffset -= amtBelowView; + } + _NotifyScrollEvent(); + _activeBuffer().TriggerScroll(); + } +} + +bool Terminal::IsTargetingUrl() const noexcept +{ + return _isTargetingUrl; +} + Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey) const { if ((_selectionMode == SelectionInteractionMode::Mark || mods.IsShiftPressed()) && !mods.IsAltPressed()) @@ -437,6 +589,7 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion } // 3. Actually modify the selection state + _isTargetingUrl = false; _selectionMode = std::max(_selectionMode, SelectionInteractionMode::Keyboard); if (shouldMoveBothEndpoints) { @@ -630,6 +783,7 @@ void Terminal::ClearSelection() { _selection = std::nullopt; _selectionMode = SelectionInteractionMode::None; + _isTargetingUrl = false; _selectionEndpoint = static_cast(0); _anchorInactiveSelectionEndpoint = false; } @@ -674,6 +828,19 @@ til::point Terminal::_ConvertToBufferCell(const til::point viewportPos) const return bufferPos; } +// Method Description: +// - convert buffer position to the corresponding location on the viewport +// Arguments: +// - bufferPos: a coordinate on the buffer +// Return Value: +// - the corresponding location on the viewport +til::point Terminal::_ConvertToViewportCell(const til::point bufferPos) const +{ + const auto yPos = bufferPos.Y - _VisibleStartIndex(); + til::point viewportPos = { bufferPos.X, yPos }; + return viewportPos; +} + // Method Description: // - This method won't be used. We just throw and do nothing. For now we // need this method to implement UiaData interface diff --git a/src/cascadia/TerminalSettingsModel/defaults.json b/src/cascadia/TerminalSettingsModel/defaults.json index 3527bf125a7..706f21d9ea2 100644 --- a/src/cascadia/TerminalSettingsModel/defaults.json +++ b/src/cascadia/TerminalSettingsModel/defaults.json @@ -382,7 +382,6 @@ // Clipboard Integration { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" }, { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" }, - { "command": { "action": "copy", "singleLine": false }, "keys": "enter" }, { "command": "paste", "keys": "ctrl+shift+v" }, { "command": "paste", "keys": "shift+insert" }, { "command": "selectAll", "keys": "ctrl+shift+a" }, From d183cacd39c6de8c37a22541d3be79302a17e1f2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 7 Jul 2022 13:07:38 -0700 Subject: [PATCH 2/7] apply feedback; fix scroll bug; use ctrl+enter --- src/cascadia/TerminalControl/ControlCore.cpp | 18 ++- src/cascadia/TerminalCore/Terminal.hpp | 8 +- .../TerminalCore/TerminalSelection.cpp | 105 +++++++++--------- 3 files changed, 74 insertions(+), 57 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 072e426e2c5..d0b7f12d30d 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -417,30 +417,40 @@ namespace winrt::Microsoft::Terminal::Control::implementation vkey != VK_SNAPSHOT && keyDown) { - const auto isInMarkMode = _terminal->SelectionMode() == ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Mark; + const auto isInMarkMode = _terminal->SelectionMode() == ::Terminal::SelectionInteractionMode::Mark; if (isInMarkMode) { if (modifiers.IsCtrlPressed() && vkey == 'A') { + // Ctrl + A --> Select all auto lock = _terminal->LockForWriting(); _terminal->SelectAll(); _updateSelectionUI(); return true; } - else if (_settings->DetectURLs() && vkey == VK_TAB) + else if (vkey == VK_TAB && _settings->DetectURLs()) { + // [Shift +] Tab --> next/previous hyperlink auto lock = _terminal->LockForWriting(); - _terminal->SelectHyperlink(!modifiers.IsShiftPressed()); + const auto direction = modifiers.IsShiftPressed() ? ::Terminal::SearchDirection::Backward : ::Terminal::SearchDirection::Forward; + _terminal->SelectHyperlink(direction); _updateSelectionUI(); return true; } - else if (_terminal->IsTargetingUrl() && vkey == VK_RETURN) + else if (vkey == VK_RETURN && modifiers.IsCtrlPressed() && _terminal->IsTargetingUrl()) { + // Ctrl + Enter --> Open URL auto lock = _terminal->LockForReading(); const auto uri = _terminal->GetHyperlinkAtPosition(_terminal->GetSelectionAnchor()); _OpenHyperlinkHandlers(*this, winrt::make(winrt::hstring{ uri })); return true; } + else if (vkey == VK_RETURN) + { + // [Shift +] Enter --> copy text + auto lock = _terminal->LockForReading(); + CopySelectionToClipboard(modifiers.IsShiftPressed(), nullptr); + } } // try to update the selection diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 4897310f74a..e4b314c82ac 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -249,6 +249,12 @@ class Microsoft::Terminal::Core::Terminal final : Down }; + enum class SearchDirection + { + Forward, + Backward + }; + enum class SelectionExpansion { Char, @@ -273,7 +279,7 @@ class Microsoft::Terminal::Core::Terminal final : SelectionInteractionMode SelectionMode() const noexcept; void SwitchSelectionEndpoint(); void ToggleMarkMode(); - void SelectHyperlink(const bool movingForward); + void SelectHyperlink(const SearchDirection dir); bool IsTargetingUrl() const noexcept; using UpdateSelectionParams = std::optional>; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 1c1d0f06bcf..1ff18346e81 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -6,6 +6,7 @@ #include "unicode.hpp" using namespace Microsoft::Terminal::Core; +using namespace Microsoft::Console::Types; DEFINE_ENUM_FLAG_OPERATORS(Terminal::SelectionEndpoint); @@ -340,12 +341,12 @@ void Terminal::SwitchSelectionEndpoint() // Method Description: // - selects the next/previous hyperlink, if one is available // Arguments: -// - searchForward: if true, we're scanning forward towards the end of the buffer. Otherwise, we're scanning backwards towards the top. +// - dir: the direction we're scanning the buffer in to find the hyperlink of interest // Return Value: // - true if we found a hyperlink to select (and selected it). False otherwise. -void Terminal::SelectHyperlink(const bool searchForward) +void Terminal::SelectHyperlink(const SearchDirection dir) { - if (!_markMode) + if (_selectionMode != SelectionInteractionMode::Mark) { // This feature only works in mark mode _isTargetingUrl = false; @@ -356,76 +357,64 @@ void Terminal::SelectHyperlink(const bool searchForward) const auto bufferSize = _activeBuffer().GetSize(); const auto viewportHeight = _GetMutableViewport().Height(); - // extracts the next/previous hyperlink from the list of hyperlink ranges provided - auto extractResultFromList = [&, onUrl = _isTargetingUrl, selectionStart = _selection->start](const std::vector>& list, bool forward, bool useViewportCoords) { - // checks if we're already on the hyperlink and if we're trying to set it to the same one - auto isAlreadySelected = [onUrl, selectionStartForValidation = useViewportCoords ? _ConvertToViewportCell(selectionStart) : selectionStart](const interval_tree::Interval range) { - return onUrl && selectionStartForValidation == range.start; - }; + // The patterns are stored relative to the "search area". Initially, this search area will be the viewport, + // but as we progressively search through more of the buffer, this will change. + // Keep track of the search area here, and use the lambda below to convert points to the search area coordinate space. + auto searchArea = _GetVisibleViewport(); + auto convertToSearchArea = [&searchArea](const til::point pt) { + auto copy = pt; + searchArea.ConvertToOrigin(©); + return copy; + }; + // extracts the next/previous hyperlink from the list of hyperlink ranges provided + auto extractResultFromList = [&](std::vector>& list) { + const auto selectionStartInSearchArea = convertToSearchArea(_selection->start); std::optional> resultFromList; - if (list.size() == 1) + for (const auto& range : list) { - if (!isAlreadySelected(list.front())) + if (_isTargetingUrl && selectionStartInSearchArea == range.start) { - resultFromList = std::make_pair(list.front().start, list.front().stop); + // if we're already on the hyperlink and we're trying to set it to the same one, + // skip it + continue; } - } - else if (list.size() > 1) - { - // multiple results, find the first one in the direction we're going - if (forward) + else if (!resultFromList) { - for (const auto& range : list) - { - if (isAlreadySelected(range)) - { - continue; - } - else if (!resultFromList || range.start < resultFromList->first) - { - resultFromList = std::make_pair(range.start, range.stop); - } - } + resultFromList = std::make_pair(range.start, range.stop); } - else + else if (dir == SearchDirection::Forward && range.start < resultFromList->first) { - for (const auto& range : list) - { - if (isAlreadySelected(range)) - { - continue; - } - else if (!resultFromList || range.start > resultFromList->first) - { - resultFromList = std::make_pair(range.start, range.stop); - } - } + resultFromList = std::make_pair(range.start, range.stop); + } + else if (dir == SearchDirection::Backward && range.start > resultFromList->first) + { + resultFromList = std::make_pair(range.start, range.stop); } } - // useViewportCoords --> pattern tree stores everything as viewport coords, + // pattern tree stores everything as viewport coords, // so we need to convert them on the way out - if (useViewportCoords && resultFromList) + if (resultFromList) { - resultFromList->first = _ConvertToBufferCell(resultFromList->first); - resultFromList->second = _ConvertToBufferCell(resultFromList->second); + searchArea.ConvertFromOrigin(&resultFromList->first); + searchArea.ConvertFromOrigin(&resultFromList->second); } return resultFromList; }; // 1. Look for the hyperlink - til::point searchStart = searchForward ? _selection->start : til::point{ bufferSize.Left(), _VisibleStartIndex() }; - til::point searchEnd = searchForward ? til::point{ bufferSize.RightInclusive(), _VisibleEndIndex() } : _selection->start; + til::point searchStart = dir == SearchDirection::Forward ? _selection->start : til::point{ bufferSize.Left(), _VisibleStartIndex() }; + til::point searchEnd = dir == SearchDirection::Forward ? til::point{ bufferSize.RightInclusive(), _VisibleEndIndex() } : _selection->start; // 1.A) Try searching the current viewport (no scrolling required) auto patterns = _patternIntervalTree; auto resultList = patterns.findContained(_ConvertToViewportCell(searchStart), _ConvertToViewportCell(searchEnd)); - std::optional> result = extractResultFromList(resultList, searchForward, true); + std::optional> result = extractResultFromList(resultList); if (!result) { // 1.B) Incrementally search through more of the space - if (searchForward) + if (dir == SearchDirection::Forward) { searchStart = { bufferSize.Left(), searchEnd.y + 1 }; searchEnd = { bufferSize.RightInclusive(), std::min(searchStart.y + viewportHeight, ViewEndIndex()) }; @@ -435,17 +424,28 @@ void Terminal::SelectHyperlink(const bool searchForward) searchEnd = { bufferSize.RightInclusive(), searchStart.y - 1 }; searchStart = { bufferSize.Left(), std::max(searchStart.y - viewportHeight, bufferSize.Top()) }; } + searchArea = Viewport::FromDimensions(searchStart, searchEnd.x + 1, searchEnd.y + 1); + const til::point bufferStart{ bufferSize.Origin() }; const til::point bufferEnd{ bufferSize.RightInclusive(), ViewEndIndex() }; while (!result && bufferSize.IsInBounds(searchStart) && bufferSize.IsInBounds(searchEnd) && searchStart <= searchEnd && bufferStart <= searchStart && searchEnd <= bufferEnd) { patterns = _activeBuffer().GetPatterns(searchStart.y, searchEnd.y); - resultList = patterns.findContained(searchStart, searchEnd); - result = extractResultFromList(resultList, searchForward, false); + resultList = patterns.findContained(convertToSearchArea(searchStart), convertToSearchArea(searchEnd)); + result = extractResultFromList(resultList); if (!result) { - searchStart.y += 1; - searchEnd.y = std::min(searchStart.y + viewportHeight, ViewEndIndex()); + if (dir == SearchDirection::Forward) + { + searchStart.y += 1; + searchEnd.y = std::min(searchStart.y + viewportHeight, ViewEndIndex()); + } + else + { + searchEnd.y -= 1; + searchStart.y = std::max(searchEnd.y - viewportHeight, bufferSize.Top()); + } + searchArea = Viewport::FromDimensions(searchStart, searchEnd.x + 1, searchEnd.y + 1); } } @@ -462,6 +462,7 @@ void Terminal::SelectHyperlink(const bool searchForward) _selection->end = result->second; bufferSize.DecrementInBounds(_selection->end); _isTargetingUrl = true; + _selectionEndpoint = SelectionEndpoint::End; // 3. Scroll to the selected area (if necessary) // TODO CARLOS: I stole this from UpdateSelection(). I should probably de-duplicate it. From 0ba6f678cf0a20d278d370c1e292aaeabd17c86c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 7 Jul 2022 13:27:31 -0700 Subject: [PATCH 3/7] add _ScrollToPoint() for deduplication --- src/cascadia/TerminalCore/Terminal.hpp | 2 +- .../TerminalCore/TerminalSelection.cpp | 65 +++++++------------ 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index e4b314c82ac..31003e631cc 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -433,7 +433,7 @@ class Microsoft::Terminal::Core::Terminal final : std::pair _PivotSelection(const til::point targetPos, bool& targetStart) const; std::pair _ExpandSelectionAnchors(std::pair anchors) const; til::point _ConvertToBufferCell(const til::point viewportPos) const; - til::point _ConvertToViewportCell(const til::point viewportPos) const; + void _ScrollToPoint(const til::point pos); void _MoveByChar(SelectionDirection direction, til::point& pos); void _MoveByWord(SelectionDirection direction, til::point& pos); void _MoveByViewport(SelectionDirection direction, til::point& pos); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 1ff18346e81..fbcd1960347 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -304,6 +304,7 @@ void Terminal::ToggleMarkMode() _selection->start = cursorPos; _selection->end = cursorPos; _selection->pivot = cursorPos; + _ScrollToPoint(cursorPos); _selectionMode = SelectionInteractionMode::Mark; _blockSelection = false; _isTargetingUrl = false; @@ -409,7 +410,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir) // 1.A) Try searching the current viewport (no scrolling required) auto patterns = _patternIntervalTree; - auto resultList = patterns.findContained(_ConvertToViewportCell(searchStart), _ConvertToViewportCell(searchEnd)); + auto resultList = patterns.findContained(convertToSearchArea(searchStart), convertToSearchArea(searchEnd)); std::optional> result = extractResultFromList(resultList); if (!result) { @@ -465,23 +466,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir) _selectionEndpoint = SelectionEndpoint::End; // 3. Scroll to the selected area (if necessary) - // TODO CARLOS: I stole this from UpdateSelection(). I should probably de-duplicate it. - if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInBounds(_selection->end)) - { - if (const auto amtAboveView = visibleViewport.Top() - _selection->end.Y; amtAboveView > 0) - { - // anchor is above visible viewport, scroll by that amount - _scrollOffset += amtAboveView; - } - else - { - // anchor is below visible viewport, scroll by that amount - const auto amtBelowView = _selection->end.Y - visibleViewport.BottomInclusive(); - _scrollOffset -= amtBelowView; - } - _NotifyScrollEvent(); - _activeBuffer().TriggerScroll(); - } + _ScrollToPoint(_selection->end); } bool Terminal::IsTargetingUrl() const noexcept @@ -613,22 +598,7 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion } // 4. Scroll (if necessary) - if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInBounds(targetPos)) - { - if (const auto amtAboveView = visibleViewport.Top() - targetPos.Y; amtAboveView > 0) - { - // anchor is above visible viewport, scroll by that amount - _scrollOffset += amtAboveView; - } - else - { - // anchor is below visible viewport, scroll by that amount - const auto amtBelowView = targetPos.Y - visibleViewport.BottomInclusive(); - _scrollOffset -= amtBelowView; - } - _NotifyScrollEvent(); - _activeBuffer().TriggerScroll(); - } + _ScrollToPoint(targetPos); } void Terminal::SelectAll() @@ -830,16 +800,27 @@ til::point Terminal::_ConvertToBufferCell(const til::point viewportPos) const } // Method Description: -// - convert buffer position to the corresponding location on the viewport +// - if necessary, scroll the viewport such that the given point is visible // Arguments: -// - bufferPos: a coordinate on the buffer -// Return Value: -// - the corresponding location on the viewport -til::point Terminal::_ConvertToViewportCell(const til::point bufferPos) const +// - pos: a coordinate relative to the buffer (not viewport) +void Terminal::_ScrollToPoint(const til::point pos) { - const auto yPos = bufferPos.Y - _VisibleStartIndex(); - til::point viewportPos = { bufferPos.X, yPos }; - return viewportPos; + if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInBounds(pos)) + { + if (const auto amtAboveView = visibleViewport.Top() - pos.Y; amtAboveView > 0) + { + // anchor is above visible viewport, scroll by that amount + _scrollOffset += amtAboveView; + } + else + { + // anchor is below visible viewport, scroll by that amount + const auto amtBelowView = pos.Y - visibleViewport.BottomInclusive(); + _scrollOffset -= amtBelowView; + } + _NotifyScrollEvent(); + _activeBuffer().TriggerScroll(); + } } // Method Description: From df2e0ca1ee459f70a1b37c240482fabf9611f1e6 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 11 Jul 2022 11:26:46 -0700 Subject: [PATCH 4/7] address some more of Leonard's feedback --- .../TerminalCore/TerminalSelection.cpp | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index fbcd1960347..dd156b463c4 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -371,26 +371,43 @@ void Terminal::SelectHyperlink(const SearchDirection dir) // extracts the next/previous hyperlink from the list of hyperlink ranges provided auto extractResultFromList = [&](std::vector>& list) { const auto selectionStartInSearchArea = convertToSearchArea(_selection->start); + std::optional> resultFromList; - for (const auto& range : list) + if (!list.empty()) { - if (_isTargetingUrl && selectionStartInSearchArea == range.start) - { - // if we're already on the hyperlink and we're trying to set it to the same one, - // skip it - continue; - } - else if (!resultFromList) - { - resultFromList = std::make_pair(range.start, range.stop); - } - else if (dir == SearchDirection::Forward && range.start < resultFromList->first) + if (dir == SearchDirection::Forward) { - resultFromList = std::make_pair(range.start, range.stop); + // pattern tree includes the currently selected range when going forward, + // so we need to check if we're pointing to that one before returning it. + auto range = list.front(); + if (_isTargetingUrl && range.start == selectionStartInSearchArea) + { + if (list.size() > 1) + { + // if we're pointing to the currently selected URL, + // pick the next one. + range = til::at(list, 1); + resultFromList = { range.start, range.stop }; + } + else + { + // LOAD-BEARING: the only range here is the one that's currently selected. + // Make sure this is set to nullopt so that we keep searching through the buffer. + resultFromList = std::nullopt; + } + } + else + { + // not on currently selected range, return the first one + resultFromList = { range.start, range.stop }; + } } - else if (dir == SearchDirection::Backward && range.start > resultFromList->first) + else if (dir == SearchDirection::Backward) { - resultFromList = std::make_pair(range.start, range.stop); + // moving backwards excludes the currently selected range, + // simply return the last one in the list as it's ordered + const auto range = list.back(); + resultFromList = { range.start, range.stop }; } } @@ -409,8 +426,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir) til::point searchEnd = dir == SearchDirection::Forward ? til::point{ bufferSize.RightInclusive(), _VisibleEndIndex() } : _selection->start; // 1.A) Try searching the current viewport (no scrolling required) - auto patterns = _patternIntervalTree; - auto resultList = patterns.findContained(convertToSearchArea(searchStart), convertToSearchArea(searchEnd)); + auto resultList = _patternIntervalTree.findContained(convertToSearchArea(searchStart), convertToSearchArea(searchEnd)); std::optional> result = extractResultFromList(resultList); if (!result) { @@ -431,7 +447,7 @@ void Terminal::SelectHyperlink(const SearchDirection dir) const til::point bufferEnd{ bufferSize.RightInclusive(), ViewEndIndex() }; while (!result && bufferSize.IsInBounds(searchStart) && bufferSize.IsInBounds(searchEnd) && searchStart <= searchEnd && bufferStart <= searchStart && searchEnd <= bufferEnd) { - patterns = _activeBuffer().GetPatterns(searchStart.y, searchEnd.y); + auto patterns = _activeBuffer().GetPatterns(searchStart.y, searchEnd.y); resultList = patterns.findContained(convertToSearchArea(searchStart), convertToSearchArea(searchEnd)); result = extractResultFromList(resultList); if (!result) From 8fdf294c9dfa63e3dd95e9338262f261c09d00a2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 11 Jul 2022 12:38:22 -0700 Subject: [PATCH 5/7] bugfix: open hyperlink when in scrolled position --- src/cascadia/TerminalControl/ControlCore.cpp | 8 +++--- src/cascadia/TerminalCore/Terminal.cpp | 27 ++++++++++++-------- src/cascadia/TerminalCore/Terminal.hpp | 7 ++--- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index d0b7f12d30d..bf12c324648 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -441,7 +441,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // Ctrl + Enter --> Open URL auto lock = _terminal->LockForReading(); - const auto uri = _terminal->GetHyperlinkAtPosition(_terminal->GetSelectionAnchor()); + const auto uri = _terminal->GetHyperlinkAtBufferPosition(_terminal->GetSelectionAnchor()); _OpenHyperlinkHandlers(*this, winrt::make(winrt::hstring{ uri })); return true; } @@ -622,7 +622,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (terminalPosition.has_value()) { auto lock = _terminal->LockForReading(); // Lock for the duration of our reads. - newId = _terminal->GetHyperlinkIdAtPosition(*terminalPosition); + newId = _terminal->GetHyperlinkIdAtViewportPosition(*terminalPosition); newInterval = _terminal->GetHyperlinkIntervalFromPosition(*terminalPosition); } @@ -653,7 +653,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // Lock for the duration of our reads. auto lock = _terminal->LockForReading(); - return winrt::hstring{ _terminal->GetHyperlinkAtPosition(til::point{ pos }) }; + return winrt::hstring{ _terminal->GetHyperlinkAtViewportPosition(til::point{ pos }) }; } winrt::hstring ControlCore::HoveredUriText() const @@ -661,7 +661,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto lock = _terminal->LockForReading(); // Lock for the duration of our reads. if (_lastHoveredCell.has_value()) { - return winrt::hstring{ _terminal->GetHyperlinkAtPosition(*_lastHoveredCell) }; + return winrt::hstring{ _terminal->GetHyperlinkAtViewportPosition(*_lastHoveredCell) }; } return {}; } diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 9aa946cc706..7608518157f 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -564,17 +564,24 @@ bool Terminal::ShouldSendAlternateScroll(const unsigned int uiButton, // Method Description: // - Given a coord, get the URI at that location // Arguments: -// - The position -std::wstring Terminal::GetHyperlinkAtPosition(const til::point position) +// - The position relative to the viewport +std::wstring Terminal::GetHyperlinkAtViewportPosition(const til::point viewportPos) { - auto attr = _activeBuffer().GetCellDataAt(_ConvertToBufferCell(position))->TextAttr(); + return GetHyperlinkAtBufferPosition(_ConvertToBufferCell(viewportPos)); +} + +std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) +{ + auto attr = _activeBuffer().GetCellDataAt(bufferPos)->TextAttr(); if (attr.IsHyperlink()) { auto uri = _activeBuffer().GetHyperlinkUriFromId(attr.GetHyperlinkId()); return uri; } // also look through our known pattern locations in our pattern interval tree - const auto result = GetHyperlinkIntervalFromPosition(position); + auto viewportPos = bufferPos; + _GetVisibleViewport().ConvertToOrigin(&viewportPos); + const auto result = GetHyperlinkIntervalFromPosition(viewportPos); if (result.has_value() && result->value == _hyperlinkPatternId) { const auto start = result->start; @@ -595,23 +602,23 @@ std::wstring Terminal::GetHyperlinkAtPosition(const til::point position) // Method Description: // - Gets the hyperlink ID of the text at the given terminal position // Arguments: -// - The position of the text +// - The position of the text relative to the viewport // Return value: // - The hyperlink ID -uint16_t Terminal::GetHyperlinkIdAtPosition(const til::point position) +uint16_t Terminal::GetHyperlinkIdAtViewportPosition(const til::point viewportPos) { - return _activeBuffer().GetCellDataAt(_ConvertToBufferCell(position))->TextAttr().GetHyperlinkId(); + return _activeBuffer().GetCellDataAt(_ConvertToBufferCell(viewportPos))->TextAttr().GetHyperlinkId(); } // Method description: // - Given a position in a URI pattern, gets the start and end coordinates of the URI // Arguments: -// - The position +// - The position relative to the viewport // Return value: // - The interval representing the start and end coordinates -std::optional Terminal::GetHyperlinkIntervalFromPosition(const til::point position) +std::optional Terminal::GetHyperlinkIntervalFromPosition(const til::point viewportPos) { - const auto results = _patternIntervalTree.findOverlapping({ position.X + 1, position.Y }, position); + const auto results = _patternIntervalTree.findOverlapping({ viewportPos.X + 1, viewportPos.Y }, viewportPos); if (results.size() > 0) { for (const auto& result : results) diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 31003e631cc..383c44181a5 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -162,9 +162,10 @@ class Microsoft::Terminal::Core::Terminal final : void FocusChanged(const bool focused) noexcept override; - std::wstring GetHyperlinkAtPosition(const til::point position); - uint16_t GetHyperlinkIdAtPosition(const til::point position); - std::optional::interval> GetHyperlinkIntervalFromPosition(const til::point position); + std::wstring GetHyperlinkAtViewportPosition(const til::point viewportPos); + std::wstring GetHyperlinkAtBufferPosition(const til::point bufferPos); + uint16_t GetHyperlinkIdAtViewportPosition(const til::point viewportPos); + std::optional::interval> GetHyperlinkIntervalFromPosition(const til::point viewportPos); #pragma endregion #pragma region IBaseData(base to IRenderData and IUiaData) From 5ae78c3a14e4b40c992209d6b97687e42cd4ca23 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 11 Jul 2022 17:37:12 -0700 Subject: [PATCH 6/7] fix enter --> copy --- src/cascadia/TerminalControl/ControlCore.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index bf12c324648..70bc534cda0 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -448,8 +448,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation else if (vkey == VK_RETURN) { // [Shift +] Enter --> copy text - auto lock = _terminal->LockForReading(); + // Don't lock here! CopySelectionToClipboard already locks for you! CopySelectionToClipboard(modifiers.IsShiftPressed(), nullptr); + _terminal->ClearSelection(); + _updateSelectionUI(); + return true; } } From 7456e15962d654f13df87cbc7ef42a3710ce9f83 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 11 Jul 2022 17:38:44 -0700 Subject: [PATCH 7/7] rename GetHyperlinkIntervalFromPosition --- src/cascadia/TerminalControl/ControlCore.cpp | 4 ++-- src/cascadia/TerminalCore/Terminal.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 70bc534cda0..01669c7d929 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -621,12 +621,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation _lastHoveredCell = terminalPosition; uint16_t newId{ 0u }; // we can't use auto here because we're pre-declaring newInterval. - decltype(_terminal->GetHyperlinkIntervalFromPosition({})) newInterval{ std::nullopt }; + decltype(_terminal->GetHyperlinkIntervalFromViewportPosition({})) newInterval{ std::nullopt }; if (terminalPosition.has_value()) { auto lock = _terminal->LockForReading(); // Lock for the duration of our reads. newId = _terminal->GetHyperlinkIdAtViewportPosition(*terminalPosition); - newInterval = _terminal->GetHyperlinkIntervalFromPosition(*terminalPosition); + newInterval = _terminal->GetHyperlinkIntervalFromViewportPosition(*terminalPosition); } // If the hyperlink ID changed or the interval changed, trigger a redraw all diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 7608518157f..f8dce231a3a 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -581,7 +581,7 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) // also look through our known pattern locations in our pattern interval tree auto viewportPos = bufferPos; _GetVisibleViewport().ConvertToOrigin(&viewportPos); - const auto result = GetHyperlinkIntervalFromPosition(viewportPos); + const auto result = GetHyperlinkIntervalFromViewportPosition(viewportPos); if (result.has_value() && result->value == _hyperlinkPatternId) { const auto start = result->start; @@ -616,7 +616,7 @@ uint16_t Terminal::GetHyperlinkIdAtViewportPosition(const til::point viewportPos // - The position relative to the viewport // Return value: // - The interval representing the start and end coordinates -std::optional Terminal::GetHyperlinkIntervalFromPosition(const til::point viewportPos) +std::optional Terminal::GetHyperlinkIntervalFromViewportPosition(const til::point viewportPos) { const auto results = _patternIntervalTree.findOverlapping({ viewportPos.X + 1, viewportPos.Y }, viewportPos); if (results.size() > 0)