[Follow-up] Polish keyboard navigation to hyperlinks by carlos-zamora · Pull Request #13494 · microsoft/terminal

carlos-zamora · 2022-07-13T00:05:34Z

Summary of the Pull Request

Polishes #13405 with some additional feedback found in testing and post-mortem reviews. Such feedback includes:

ControlInteractivity vs ControlCore split (link)
clearing the selection should be under lock when copying text via "Enter"
move mark mode keybindings into a helper function
decide if "Enter" should be configurable or non-configurable (link)
rename _isTargetingUrl
bugfix: ctrl+enter when the link is outside of the viewport

References

Original PR: #13405
Relevant issue: #6649
Epic: #4993

zadjii-msft · 2022-07-22T11:26:13Z

Marking for discussion, but drill into the links above folks. The discussion is happening on the original PR.

zadjii-msft · 2022-08-15T21:45:47Z

ControlInteractivity vs ControlCore split (link)

Meh it's fine

decide if "Enter" should be configurable or non-configurable (link)

Leave it as always a feature of Mark Mode, and add it back as a possible setting, so enter on a mouse selection will work, but you can unbound it and it'll stop working for a mouse selection, but still work in mark mode.

carlos-zamora · 2022-08-15T23:22:03Z

FYI: this will have a merge conflict with #13659

lhecker · 2022-08-16T19:07:03Z

src/cascadia/TerminalCore/Terminal.cpp

+    else
+    {
+        // Hyperlink is outside of the current view.
+        // We need to find if there's a pattern at that location.
+        const auto patterns = _activeBuffer().GetPatterns(bufferPos.y, bufferPos.y);
+
+        // NOTE: patterns is stored with top y-position being 0,
+        //       so we need to cleverly set the y-pos to 0.
+        const til::point viewportPos{ bufferPos.x, 0 };
+        const auto results = patterns.findOverlapping(viewportPos, viewportPos);
+        if (!results.empty())
+        {
+            result = results.front();
+            result->start.y += bufferPos.y;
+            result->stop.y += bufferPos.y;
+        }
+    }


Why do we have to handle hyperlinks outside of the viewport?

Keyboard-based navigation through the buffer looking for links would not be very effective if it was constrained to only working in the visible region :)

Oh I guess the scrolling is happening in the caller then?

I guess I didn't think about how this function was used. It's GOOD that it is robust now, but I suppose now that it was never called with coordinates outside the viewport, eh?

I think given due to #13854 (comment), long-term, we should probably have everything be in the buffer coordinate system. It'll simplify a lot of this extra code. We also need to find hyperlinks outside of the existing viewport. I assume we didn't do that the first time due to performance concerns. :/

carlos-zamora · 2022-08-24T18:07:22Z

@msftbot merge this in 10 minutes

ghost · 2022-08-24T18:07:27Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 24 Aug 2022 18:17:26 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

Blocking til after my meeting!

DHowett · 2022-08-25T18:52:22Z

src/cascadia/TerminalCore/Terminal.cpp

+    {
+        // Hyperlink is outside of the current view.
+        // We need to find if there's a pattern at that location.
+        const auto patterns = _activeBuffer().GetPatterns(bufferPos.y, bufferPos.y);


what happens when a hyperlink starts above the screen but wraps onto another line? Will we accidentally capture only half of it?

Welp. Found this out during a bug bash and now it's a line item in #13854. See #13854 (comment) for details on what's going on.

DHowett · 2022-08-25T18:53:22Z

src/cascadia/TerminalCore/Terminal.cpp

+    else
+    {
+        // Hyperlink is outside of the current view.
+        // We need to find if there's a pattern at that location.
+        const auto patterns = _activeBuffer().GetPatterns(bufferPos.y, bufferPos.y);
+
+        // NOTE: patterns is stored with top y-position being 0,
+        //       so we need to cleverly set the y-pos to 0.
+        const til::point viewportPos{ bufferPos.x, 0 };
+        const auto results = patterns.findOverlapping(viewportPos, viewportPos);
+        if (!results.empty())
+        {
+            result = results.front();
+            result->start.y += bufferPos.y;
+            result->stop.y += bufferPos.y;
+        }
+    }


I guess I didn't think about how this function was used. It's GOOD that it is robust now, but I suppose now that it was never called with coordinates outside the viewport, eh?

DHowett · 2022-08-25T18:54:09Z

And hey, does "selectionPointingAtHyperlink" really mean, "selectionPointingAtHyperlinkAfterHavingBeenHavigatedToInMarkMode"? Or, if you manually select a URL with your mouse, will it update to be true???

carlos-zamora · 2022-09-01T17:13:04Z

And hey, does "selectionPointingAtHyperlink" really mean, "selectionPointingAtHyperlinkAfterHavingBeenHavigatedToInMarkMode"?

yeah, it really means "selectionPointingAtHyperlinkAfterHavingBeenHavigatedToInMarkMode". In fact, that's really the only scenario we need to keep track of so that we skip over the hyperlink we're currently on.

Or, if you manually select a URL with your mouse, will it update to be true???

This creates a weird corner case that may require further thinking. Right now, ctrl+enter is a mark mode thing. So if you create a selection using your mouse (and even if you modify it using shift+left/right), ctrl+enter won't do anything. You have to enter mark mode to be able to do try and open it as a URL.

Perhaps the correct solution here is...

if (IsSelectionActive() && /*ctrl+enter pressed*/)
{
	if (IsInMarkMode())
	{
		TryOpenURL();
	}
	else if (IsBoundToAction(/*ctrl+enter*/))
	{
		DoAction(/*ctrl+enter*/);
	}
	else
	{
		TryOpenURL();
	}
}

Let me know if you agree and I'll move this into #13854

EDIT: eh, I'll just move it there. It's a real problem haha

ghost · 2022-09-13T17:06:02Z

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

[Follow-up] Polish keyboard navigation to hyperlinks

e0806be

carlos-zamora requested a review from DHowett July 13, 2022 00:05

This comment was marked as resolved.

Sign in to view

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 22, 2022

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 15, 2022

zadjii-msft assigned carlos-zamora Aug 15, 2022

zadjii-msft added this to the Terminal v1.16 milestone Aug 15, 2022

carlos-zamora added 2 commits August 15, 2022 16:19

rebind 'enter' as kbd

26d1deb

Merge branch 'main' into dev/cazamor/polish-tab-to-link

a493944

carlos-zamora marked this pull request as ready for review August 15, 2022 23:21

code format

1acf0d4

lhecker approved these changes Aug 16, 2022

View reviewed changes

lhecker reviewed Aug 16, 2022

View reviewed changes

carlos-zamora removed their assignment Aug 16, 2022

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 16, 2022

ghost requested review from PankajBhojwani and zadjii-msft August 16, 2022 22:43

Merge branch 'main' into dev/cazamor/polish-tab-to-link

57b1dab

zadjii-msft approved these changes Aug 24, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 24, 2022

DHowett requested changes Aug 24, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 24, 2022

DHowett approved these changes Aug 25, 2022

View reviewed changes

ghost merged commit 4488a25 into main Aug 25, 2022

ghost deleted the dev/cazamor/polish-tab-to-link branch August 25, 2022 18:53

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 1, 2022

carlos-zamora mentioned this pull request Sep 1, 2022

Mark mode hyperlink navigation sometimes skips over entries #13854

Open

7 tasks

This pull request was closed.

Conversation

carlos-zamora commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

Uh oh!

This comment was marked as resolved.

zadjii-msft commented Jul 22, 2022

Uh oh!

zadjii-msft commented Aug 15, 2022

Uh oh!

carlos-zamora commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Aug 24, 2022

Uh oh!

ghost commented Aug 24, 2022

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Aug 25, 2022

Uh oh!

carlos-zamora commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Jul 13, 2022 •

edited

Loading

carlos-zamora commented Sep 1, 2022 •

edited

Loading