Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display URI tooltip, render dashed/solid underline for links #7420

Merged
merged 49 commits into from
Sep 10, 2020

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Aug 26, 2020

Summary of the Pull Request

  • Render hyperlinks with a dashed underline
  • Render hovered hyperlinks with a solid underline
  • Show URI tooltip on hover

References

#5001

PR Checklist

  • Closes #xxx
  • I work here
  • Tests added/passed
  • I've discussed this with core contributors already.

Detailed Description of the Pull Request / Additional comments

TermControl now has a canvas that contains a tiny border to which a tooltip is attached. When we hover over hyperlinked text, we move the border to the mouse location and update the tooltip content with the URI.

Introduced a new underline type (HyperlinkUnderline), supports rendering for it, and uses it to render hyperlinks. HyperlinkUnderline is usually a dashed underline, but when a link is hovered, all text with the same hyperlink ID is rendered with a solid underline.

Validation Steps Performed

tooltip

src/renderer/dx/DxRenderer.cpp Show resolved Hide resolved
src/renderer/dx/DxRenderer.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.xaml Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2020
Comment on lines +1290 to +1295
if (newId != _lastHoveredId)
{
_renderEngine->UpdateHyperlinkHoveredId(newId);
_renderer->TriggerRedrawAll();
_lastHoveredId = newId;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this logic (check equality, store last hovered ID, trigger redraw) all belongs inside UpdateHyperlinkHoveredId. Is that bad for the render engine contract @miniksa?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(right now we're tracking the last ID in two places)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh its frustrating. The problem is that the specific engine has no way of signaling the renderer base to do the redraw all operation. When I originally laid it out, it was supposed to be getting commands inbound only.

This isn't that far off from us storing the invalid regions or cursor positions in multiple places. For instance, some of the renderers remember the last cursor pos they drew so they can erase the cursor "turds". Yes, it's two places... but there's precedent and I'm not sure what else is to be done beyond rearchitecting, which is out of scope.

Comment on lines +1290 to +1295
if (newId != _lastHoveredId)
{
_renderEngine->UpdateHyperlinkHoveredId(newId);
_renderer->TriggerRedrawAll();
_lastHoveredId = newId;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(right now we're tracking the last ID in two places)

src/cascadia/TerminalControl/TermControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.xaml Outdated Show resolved Hide resolved
src/renderer/dx/DxRenderer.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 3, 2020
@DHowett DHowett changed the title Display URI tooltip and changes to hyperlink rendering Display URI tooltip, render dashed/solid underline for links Sep 10, 2020
@DHowett DHowett merged commit be50e56 into master Sep 10, 2020
@DHowett DHowett deleted the dev/pabhoj/tooltip branch September 10, 2020 22:00
@ghost
Copy link

ghost commented Sep 22, 2020

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants