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

Synthesize VT mouse events and add mouse support to Terminal #4859

Merged
merged 9 commits into from
Mar 13, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Mar 9, 2020

Summary of the Pull Request

Make TerminalControl synthesize mouse events and Terminal send them to
the TerminalInput's MouseInput module.

The implementation here takes significant inspiration from how we handle
KeyEvents.

References

Closes #545 - VT Mouse Mode (Terminal)
References #376 - VT Mouse Mode (ConPty)

TerminalControl

  • _TrySendMouseEvent attempts to send a mouse event via TermInput.
    Similar to _TrySendKeyEvent
  • Use the above function to try and send the mouse event before
    deciding to modify the selection

TerminalApi

TerminalDispatch

  • Hookup (re)setting the various modes to handle VT Input

TerminalInput

Validation Steps Performed

Tests should still pass.

@carlos-zamora carlos-zamora added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Mar 9, 2020
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 9, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'll hold signing off till we get some of the points outlined in that mail thread cleaned up but here's some of the other feedback I have

src/cascadia/TerminalCore/Terminal.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalDispatch.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

A couple of comments, but this looks pretty cool 😎

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2020
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/vtmm/wt-synthesis branch 2 times, most recently from 98a2fbf to 7184a01 Compare March 10, 2020 23:15
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/vtmm/mouse-input branch from 97916aa to 863fab4 Compare March 11, 2020 21:11
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/vtmm/wt-synthesis branch from 7184a01 to 125e502 Compare March 11, 2020 21:25
@DHowett-MSFT
Copy link
Contributor

Okay, I played with a build of this extensively and only found two issues. One isn't even a bug, it's just ~ ~ weird ~ ~.

  1. (the bug) if you launch mc and un-focus the window, your first click-drag inside the window will begin a selection and send a mouse event. All click-drags after that will extend the selection and send mouse events.
  2. (the weirdo) if you're using standard X10 coordinate mode, which can only address cells up to 94x94, clicking outside of the 94x94 rectangle in the top left of the terminal causes normal mouse events to fire!

2 is a weird case and the encoding is not often used, i bet, so we can ignore it.

@DHowett-MSFT
Copy link
Contributor

Also, yes, gotta figure out how that "revert" commit got in here.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

These are definitely nits, so fix them and I'll sign off on this too. I know Dustin mentioned some other bugs, but IMO we can just fix them in post. They seem pretty trivial comparatively. Let's file them, and get these all in :shipit:

src/cascadia/TerminalCore/TerminalDispatch.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

LET'S DO THIS.

@DHowett-MSFT DHowett-MSFT requested a review from leonMSFT March 12, 2020 22:22
@DHowett-MSFT
Copy link
Contributor

do not automerge this -- it is not into master.

@ghost ghost closed this Mar 12, 2020
@DHowett-MSFT DHowett-MSFT reopened this Mar 12, 2020
@DHowett-MSFT DHowett-MSFT changed the base branch from dev/cazamor/vtmm/mouse-input to master March 12, 2020 22:29
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/vtmm/wt-synthesis branch from 6e5c928 to 5c720be Compare March 12, 2020 22:36
@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 12, 2020
@ghost
Copy link

ghost commented Mar 12, 2020

Hello @DHowett-MSFT!

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.

@DHowett-MSFT DHowett-MSFT changed the title Synthesize VT Mouse Events Synthesize VT mouse events from the terminal control Mar 12, 2020
@DHowett-MSFT DHowett-MSFT changed the title Synthesize VT mouse events from the terminal control Synthesize VT mouse events and add mouse support to Terminal Mar 12, 2020
@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett-MSFT DHowett-MSFT dismissed leonMSFT’s stale review March 13, 2020 00:28

issues addressed; carlos worked with Leon on this

@DHowett-MSFT
Copy link
Contributor

image

old status cannot be cleared.

@DHowett-MSFT DHowett-MSFT merged commit ae71dce into master Mar 13, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/cazamor/vtmm/wt-synthesis branch March 13, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Terminal] VT Mouse Support
4 participants