Move MouseInput from TermAdapter to TermInput #4848

carlos-zamora · 2020-03-09T18:05:13Z

Summary of the Pull Request

Move the contents and functionality of MouseInput from TerminalAdapter
to TerminalInput.

References

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

Detailed Description of the Pull Request / Additional comments

Pretty straightforward. The MouseInput class was a bit large though so I
split it up into a few files. This should make TerminalInput a bit
easier to manage.

mouseInputState: enable some of the modes for mouse input. All saved
to _mouseInputState.
mouseInput: basically just HandleMouse() and any helper functions

Validation Steps Performed

Tests should still pass.

src/terminal/input/mouseInput.cpp

zadjii-msft

Yea I'm okay with this, but I'm curious why you decided to merge the classes into one rather than leave MouseInput encapsulated

src/terminal/input/terminalInput.hpp

src/terminal/input/mouseInput.cpp

DHowett-MSFT · 2020-03-10T19:17:40Z

I was wondering about the merging too. I dunno; did we set a bad precedent with TerminalCore? 😄

DHowett-MSFT · 2020-03-10T19:18:21Z

At the same time, if you don't dislike it too much (@zadjii-msft), i guess it consolidates "things about generating VT input" into a single place!

src/host/server.h

zadjii-msft · 2020-03-10T19:58:51Z

I didn't hate it enough to block it 😋

carlos-zamora · 2020-03-10T20:52:26Z

I was wondering about the merging too. I dunno; did we set a bad precedent with TerminalCore? 😄

That's exactly what I was looking at... uh oh. But the nice thing now is that all kind of input is consistently in TerminalInput, rather than a random MouseInput object off on the side.

That said, I'll just redirect all calls to terminalMouseInput to go through the input buffer instead. I feel like that's cleaner but both of y'all know the code better so let me know if I'm going down the wrong path here.

src/host/server.h

src/terminal/adapter/lib/adapter.vcxproj

src/terminal/input/lib/terminalinput.vcxproj

ghost · 2020-03-10T23:24:41Z

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

Broke mouse mode in conhost

carlos-zamora · 2020-03-11T00:34:46Z

@DHowett-MSFT and I investigation findings:

the input sequences are getting added to the input queue but they just stay there
The removed callback would call WakeUpReadersWaitingForData(), whereas the new callback _HandleTerminalInputCallback() does not.

Looks like we need to make the input buffer wake up the reader if it's still asleep. I'll work on a proper fix for this tomorrow morning.

DHowett-MSFT · 2020-03-11T18:12:23Z

@msftbot make sure @miniksa signs off

ghost · 2020-03-11T18:12:27Z

Hello @DHowett-MSFT!

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'll only merge this pull request if it's approved by @miniksa

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".

carlos-zamora · 2020-03-11T21:22:24Z

Verified that #4859 still works with the newest change.

DHowett-MSFT

I don't care that this variable is called _vtInputShouldSuppress. We need a followup Area-CodeHealth + Area-Input workitem to clean up whose responsibility it is to notify connected clients in conhost.

DHowett-MSFT · 2020-03-12T22:24:15Z

@msftbot forget everything i just told you

## 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 - Hookup (re)setting the various modes to handle VT Input - Terminal is _always_ in VT Input mode (important for #4856) ### TerminalDispatch - Hookup (re)setting the various modes to handle VT Input ### TerminalInput - Convert the mouse input position from viewport position to buffer position - Then send it over to the MouseInput in TerminalInput to actually do it (#4848) ## Validation Steps Performed Tests should still pass.

carlos-zamora added the Area-Input Related to input processing (key presses, mouse, etc.) label Mar 9, 2020

carlos-zamora requested a review from zadjii-msft March 9, 2020 18:05

DHowett-MSFT reviewed Mar 9, 2020

View reviewed changes

src/terminal/input/mouseInput.cpp Show resolved Hide resolved

carlos-zamora mentioned this pull request Mar 9, 2020

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

Merged

zadjii-msft approved these changes Mar 10, 2020

View reviewed changes

src/terminal/input/terminalInput.hpp Show resolved Hide resolved

src/terminal/input/mouseInput.cpp Show resolved Hide resolved

zadjii-msft added Needs-Second It's a PR that needs another sign-off Product-Meta The product is the management of the products. labels Mar 10, 2020

ghost requested review from miniksa and leonMSFT March 10, 2020 12:55

carlos-zamora mentioned this pull request Mar 10, 2020

Pass Mouse Button State into HandleMouse #4869

Closed

DHowett-MSFT suggested changes Mar 10, 2020

View reviewed changes

src/host/server.h Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2020

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

DHowett-MSFT reviewed Mar 10, 2020

View reviewed changes

src/host/server.h Outdated Show resolved Hide resolved

DHowett-MSFT suggested changes Mar 10, 2020

View reviewed changes

src/terminal/adapter/lib/adapter.vcxproj Show resolved Hide resolved

src/terminal/input/lib/terminalinput.vcxproj Show resolved Hide resolved

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 Mar 10, 2020

DHowett-MSFT approved these changes Mar 10, 2020

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2020

DHowett-MSFT removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2020

DHowett-MSFT suggested changes Mar 10, 2020

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 Mar 10, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 11, 2020

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 11, 2020

DHowett-MSFT removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 11, 2020

carlos-zamora added 7 commits March 11, 2020 14:11

Copy MouseInput to TerminalInput

dfecb2f

Fully remove MouseInput and use TerminalInput

d2016e0

code format + static analysis fix

7e25e6a

add TODO and missing onecore ifdef

df35803

remove duplicate TerminalInput. Always use the InputBuffer's

41129e5

update sources and fix conhost build failure

fb6e6ba

fix conhost

863fab4

carlos-zamora force-pushed the dev/cazamor/vtmm/mouse-input branch from 97916aa to 863fab4 Compare March 11, 2020 21:11

DHowett-MSFT approved these changes Mar 12, 2020

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 12, 2020

ghost merged commit 23f7420 into master Mar 12, 2020

ghost deleted the dev/cazamor/vtmm/mouse-input branch March 12, 2020 22:25

carlos-zamora mentioned this pull request Jul 2, 2020

Pass mouse button state into HandleMouse instead of asking win32 #6765

Merged

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move MouseInput from TermAdapter to TermInput #4848

Move MouseInput from TermAdapter to TermInput #4848

carlos-zamora commented Mar 9, 2020 •

edited by DHowett-MSFT

Loading

zadjii-msft left a comment

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

zadjii-msft commented Mar 10, 2020

carlos-zamora commented Mar 10, 2020

ghost commented Mar 10, 2020

DHowett-MSFT left a comment

carlos-zamora commented Mar 11, 2020

DHowett-MSFT commented Mar 11, 2020

ghost commented Mar 11, 2020

carlos-zamora commented Mar 11, 2020

DHowett-MSFT left a comment

DHowett-MSFT commented Mar 12, 2020

Move MouseInput from TermAdapter to TermInput #4848

Move MouseInput from TermAdapter to TermInput #4848

Conversation

carlos-zamora commented Mar 9, 2020 • edited by DHowett-MSFT Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

zadjii-msft commented Mar 10, 2020

carlos-zamora commented Mar 10, 2020

ghost commented Mar 10, 2020

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 left a comment

Choose a reason for hiding this comment

carlos-zamora commented Mar 11, 2020

DHowett-MSFT commented Mar 11, 2020

ghost commented Mar 11, 2020

carlos-zamora commented Mar 11, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 12, 2020

carlos-zamora commented Mar 9, 2020 •

edited by DHowett-MSFT

Loading

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.