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

Fix VT mouse capture issues in Terminal and conhost #7166

Merged
8 commits merged into from
Aug 4, 2020
Merged

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 3, 2020

This pull request fixes capture and event generation in VT mouse mode
for both conhost and terminal.

Fixes #6401.

[1/3] Terminal: clamp mouse events to the viewport, don't throw them away

gnome-terminal (at least) sends mouse events whose x/y are at the
extreme ends of the buffer when a drag starts inside the terminal and
then exits it.

We would previously discard any mouse events that exited the borders of
the viewport. Now we will keep emitting events where X/Y=0/w/h.

[2/3] conhost: clamp VT mouse to viewport, capture pointer

This is the same as (1), but for conhost. conhost wasn't already
capturing the pointer when VT mouse mode was in use. By capturing, we
ensure that events that happen outside the screen still result in events
sent to an application (like a release after a drag)

[3/3] wpf: capture the pointer when VT mouse is enabled

This is the same as (2), but for the WPF control. Clamping is handled
in TerminalCore in (1), so we didn't need to do it in WPF.

gnome-terminal (at least) sends mouse events whose x/y are at the
extreme ends of the buffer when a drag starts inside the terminal and
then exits it.

We would previously discard any mouse events that exited the borders of
the viewport. Now we will keep emitting events where X/Y=0/w/h.

Fixes #6401 in Terminal.
This is the same as 9d727a4, but for conhost. conhost wasn't already
capturing the pointer when VT mouse mode was in use. By capturing, we
ensure that events that happen outside the screen still result in events
sent to an application (like a release after a drag)

Fixes #6401 in conhost.
@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Aug 3, 2020
@DHowett DHowett changed the title Dev/duhowett/mousecap Fix VT mouse capture issues in Terminal and conhost Aug 3, 2020
@DHowett
Copy link
Member Author

DHowett commented Aug 3, 2020

wiffed it with the name. fixed.

@DHowett DHowett requested a review from miniksa August 3, 2020 21:33
@DHowett
Copy link
Member Author

DHowett commented Aug 3, 2020

Tagged @miniksa in because it touches the conhost wndproc

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Looks technically good, but I'd try to leave a few more breadcrumbs of reasoning behind for the next person in comments above the code.

src/cascadia/TerminalCore/Terminal.cpp Outdated Show resolved Hide resolved
@@ -123,7 +123,10 @@ bool HandleTerminalMouseEvent(const COORD cMousePosition,
// Virtual terminal input mode
if (IsInVirtualTerminalInputMode())
{
fWasHandled = gci.GetActiveInputBuffer()->GetTerminalInput().HandleMouse(cMousePosition, uiButton, sModifierKeystate, sWheelDelta);
auto clampedPosition{ cMousePosition };
Copy link
Member

Choose a reason for hiding this comment

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

Same here. What's the reason to clamp before a future maintainer get the bright idea to change it again.

src/interactivity/win32/windowio.cpp Show resolved Hide resolved
@DHowett
Copy link
Member Author

DHowett commented Aug 3, 2020

@msftbot merge this in 9 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 3, 2020
@ghost
Copy link

ghost commented Aug 3, 2020

Hello @DHowett!

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 Mon, 03 Aug 2020 23:33:54 GMT, which is in 9 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 DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 3, 2020
@miniksa
Copy link
Member

miniksa commented Aug 4, 2020

Looks good to me with wpf control and comments.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 4, 2020
@ghost
Copy link

ghost commented Aug 4, 2020

Hello @DHowett!

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 3 hours 42 minutes. No worries though, I will be back when the time is right! 😉

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
Copy link
Member Author

DHowett commented Aug 4, 2020

@msftbot merge this in 1 minutes

@ghost
Copy link

ghost commented Aug 4, 2020

Hello @DHowett!

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 Tue, 04 Aug 2020 01:42:23 GMT, which is in 1 minute

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

@ghost ghost merged commit d29be59 into master Aug 4, 2020
@ghost ghost deleted the dev/duhowett/mousecap branch August 4, 2020 01:43
DHowett added a commit that referenced this pull request Aug 4, 2020
This pull request fixes capture and event generation in VT mouse mode
for both conhost and terminal.

Fixes #6401.

[1/3] Terminal: clamp mouse events to the viewport, don't throw them away

 gnome-terminal (at least) sends mouse events whose x/y are at the
 extreme ends of the buffer when a drag starts inside the terminal and
 then exits it.

 We would previously discard any mouse events that exited the borders of
 the viewport. Now we will keep emitting events where X/Y=0/w/h.

[2/3] conhost: clamp VT mouse to viewport, capture pointer

 This is the same as (1), but for conhost. conhost wasn't already
 capturing the pointer when VT mouse mode was in use. By capturing, we
 ensure that events that happen outside the screen still result in events
 sent to an application (like a release after a drag)

[3/3] wpf: capture the pointer when VT mouse is enabled

 This is the same as (2), but for the WPF control. Clamping is handled
 in TerminalCore in (1), so we didn't need to do it in WPF.

(cherry picked from commit d29be59)
DHowett added a commit that referenced this pull request Aug 4, 2020
This pull request fixes capture and event generation in VT mouse mode
for both conhost and terminal.

Fixes #6401.

[1/3] Terminal: clamp mouse events to the viewport, don't throw them away

 gnome-terminal (at least) sends mouse events whose x/y are at the
 extreme ends of the buffer when a drag starts inside the terminal and
 then exits it.

 We would previously discard any mouse events that exited the borders of
 the viewport. Now we will keep emitting events where X/Y=0/w/h.

[2/3] conhost: clamp VT mouse to viewport, capture pointer

 This is the same as (1), but for conhost. conhost wasn't already
 capturing the pointer when VT mouse mode was in use. By capturing, we
 ensure that events that happen outside the screen still result in events
 sent to an application (like a release after a drag)

[3/3] wpf: capture the pointer when VT mouse is enabled

 This is the same as (2), but for the WPF control. Clamping is handled
 in TerminalCore in (1), so we didn't need to do it in WPF.

(cherry picked from commit d29be59)
DHowett added a commit that referenced this pull request Aug 5, 2020
Carlos Zamora (1)
* UIA: use full buffer comparison in rects and endpoint setter (GH-6447)

Dan Thompson (2)
* Tweaks: normalize TextAttribute method names (adjective form) (GH-6951)
* Fix 'bcz exclusive' typo (GH-6938)

Dustin L. Howett (4)
* Fix VT mouse capture issues in Terminal and conhost (GH-7166)
* version: bump to 1.3 on master
* Update Cascadia Code to 2007.15 (GH-6958)
* Move to the TerminalDependencies NuGet feed (GH-6954)

James Holderness (3)
* Render the SGR "underlined" attribute in the style of the font (CC-7148)
* Add support for the "crossed-out" graphic rendition attribute (CC-7143)
* Refactor grid line renderers with support for more line types (CC-7107)

Leonard Hecker (1)
* Added til::spsc, a lock-free, single-producer/-consumer FIFO queue (CC-6751)

Michael Niksa (6)
* Update TAEF to 10.57.200731005-develop (GH-7164)
* Skip DX invalidation if we've already scrolled an entire screen worth of height (GH-6922)
* Commit attr runs less frequently by accumulating length of color run (GH-6919)
* Set memory order on slow atomics (GH-6920)
* Cache the viewport to make invalidation faster (GH-6918)
* Correct comment in this SPSC test as a quick follow up to merge.

Related work items: MSFT-28208358
@ghost
Copy link

ghost commented Aug 13, 2020

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

Handy links:

@ghost
Copy link

ghost commented Aug 13, 2020

🎉Windows Terminal Preview v1.2.2234.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
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-VT Virtual Terminal sequence support 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. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VT Mouse Mode] Release is not fired when a drag begins inside and exits the terminal window
4 participants