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.

Sign up for GitHub

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

Jump to bottom

Allow interaction with hyperlinks in mouse mode #9396

Merged
7 commits merged into from
Mar 10, 2021
Merged

Allow interaction with hyperlinks in mouse mode #9396

7 commits merged into from
Mar 10, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Mar 5, 2021 •
edited
Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

In mouse mode:

  • Underline hyperlinks
  • Activate hyperlink on ctrl+click rather than sending input to VT

@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Mar 5, 2021
carlos-zamora
carlos-zamora approved these changes Mar 5, 2021
View reviewed changes
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Mar 6, 2021
@zadjii-msft
Copy link
Member

wait is this going to immediately conflict with #9403?

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 8, 2021

wait is this going to immediately conflict with #9403?

Code-wise yes :) Logically not :)

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 8, 2021

@zadjii-msft - but before reviewing it further - probably it worth considering the discussion in the ticket (#9117). Probably we want to switch the links to ctrl+shift+click to be consistent between mouse mode and non-mouse mode without doing the hacks I did (of both passing the click and handling the link).

@zadjii-msft
Copy link
Member

Hmm. I'm not sure how I feel about that. I like the idea of consistency, but I dislike the idea of adding shift to the default modifiers you need to press.

If I'm looking to other terminal emulators for precedent, it seems like hyperlinks in gnome-terminal are actually a higher priority than mouse mode. So you can click on links with just ctrl+click even in mouse mode. Maybe we should follow that precedent? I'll ask the team

@zadjii-msft
Copy link
Member

Okay yea team consensus was that the gnome-terminal behavior was sensible and we should just do that. So no need for shift while clicking links with mouse mode enabled - the hyperlink takes priority over the mouse mode click

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 9, 2021

@zadjii-msft, @carlos-zamora -please re-review 😊

DHowett
DHowett reviewed Mar 10, 2021
View reviewed changes
src/cascadia/TerminalControl/TermControl.cpp Outdated
else if (_CanSendVTMouseInput())
{
// GH#9396: hyper-link gets higher priority than mouse mode
_TrySendMouseEvent(point);
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 a bit worried about the flow of code through these conditionals. This will change the block selection state, track click counts, etc. before dispatching a mouse event.

We might be overdue for refactoring the click dispatcher. . . but this seems a bit too entangled for my taste?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DHowett you are right as usual, currently the hyperlink handling was already entangled into the selection processing code. This forced me to push the VT processing there as well (as hyperlink should precede).

I issued another commit, trying to decouple everything into:

  1. try to handle hyperlink
  2. else try to handle VT mouse
  3. else try to update selection (left-click)
  4. else try to handle paste (right-click)

@Don-Vito Don-Vito requested a review from DHowett March 10, 2021 05:27
carlos-zamora
carlos-zamora approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This is such a nit, so I'll approve anyways. But I won't put automerge on it until I get a response. Thanks!

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2021
@ghost
Copy link

ghost commented Mar 10, 2021

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 6cd4e03 into microsoft:main Mar 10, 2021
DHowett pushed a commit that referenced this pull request Mar 15, 2021
@Don-Vito @DHowett
Allow interaction with hyperlinks in mouse mode (#9396)
57f727c 
## PR Checklist
* [x] Closes #9117
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
In mouse mode:
* Underline hyperlinks
* Activate hyperlink on ctrl+click rather than sending input to VT

(cherry picked from commit 6cd4e03)
DHowett pushed a commit that referenced this pull request Mar 15, 2021
@Don-Vito @DHowett
Allow interaction with hyperlinks in mouse mode (#9396)
2d53cb2 
## PR Checklist
* [x] Closes #9117
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
In mouse mode:
* Underline hyperlinks
* Activate hyperlink on ctrl+click rather than sending input to VT

(cherry picked from commit 6cd4e03)
@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@DHowett DHowett Awaiting requested review from DHowett

Assignees

@DHowett DHowett

@carlos-zamora carlos-zamora

Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Don-Vito @zadjii-msft @DHowett @carlos-zamora