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

Using SetConsoleMode to set/clear ENABLE_MOUSE_INPUT does not take effect (unless a WriteConsoleW call occurs as well) #15711

Closed
chrisant996 opened this issue Jul 14, 2023 · 1 comment · Fixed by #15991
Closed

Using SetConsoleMode to set/clear ENABLE_MOUSE_INPUT does not take effect (unless a WriteConsoleW call occurs as well) #15711

chrisant996 opened this issue Jul 14, 2023 · 1 comment · Fixed by #15991
Assignees
@lhecker
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.19

Comments

@chrisant996
Copy link

Windows Terminal version

1.18.1462.0

Windows build number

10.0.19045.3208

Other Software

I've provided a minimal standalone repro here to eliminate dependencies on anything other than Terminal.
But for completeness: this was discovered in the context of Clink.

Steps to reproduce

This is a regression in 1.18; the problem does not exist in 1.17 and earlier.

Minimal repro program is available at https://github.com/chrisant996/repro_enable_mouse_input.
The repo also includes a pre-built code-signed Repro.exe executable.

See the Readme.md file in that repo for full details and repro instructions.

  1. Start Windows Terminal 1.18
  2. Run the Repro.exe program from the repro_enable_mouse_input repo (you can build it yourself from the sources, or you can run the pre-built code-signed copy)
  3. Move the mouse over the terminal window; you should see no output
  4. Click the primary mouse button on the terminal window; you should see no output
  5. Hold Ctrl+Alt and move the mouse or click the mouse button

Expected Behavior

  • You should see output acknowledging that ReadConsoleInputW received mouse input records.

Actual Behavior

  • Terminal 1.18 -- there is no output (incorrect behavior).
  • Terminal 1.17 -- the output reports received mouse input records (correct behavior).
@chrisant996 chrisant996 added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 14, 2023
@DHowett
Copy link
Member

DHowett commented Jul 14, 2023

@zadjii-msft this is likely due to the flushing changes you made in #14677

@DHowett DHowett added this to the Terminal v1.19 milestone Jul 14, 2023
@DHowett DHowett added Product-Conpty For console issues specifically related to conpty Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Priority-1 A description (P1) labels Jul 14, 2023
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 17, 2023
@zadjii-msft zadjii-msft self-assigned this Jul 17, 2023
@chrisant996 chrisant996 mentioned this issue Aug 3, 2023
Closed
@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Aug 24, 2023
@lhecker lhecker self-assigned this Sep 11, 2023
@lhecker lhecker mentioned this issue Sep 18, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Sep 18, 2023
@zadjii-msft zadjii-msft removed their assignment Sep 18, 2023
DHowett pushed a commit that referenced this issue Sep 21, 2023
@lhecker
ConPTY: Fix missing flush on console mode changes (#15991)
41f7ed7 
Previously, all unknown escape sequences would lead to an immediate call
to `VtEngine::_Flush()`. This lead to problems with nushell which uses
FTCS marks that were unknown to us. Combined with the linewise redrawing
that nushell does, Terminal would get the prompt in two separate frames,
causing a slight flickering.

#14677 fixed this by suppressing the `_Flush()` call when unknown
sequences are encountered. Unfortunately, this triggered a bug due
to our somewhat "inconsistent" architecture in conhost:
`XtermEngine::WriteTerminalW` isn't just used to flush unknown sequences
but also used directly by `InputBuffer::PassThroughWin32MouseRequest`
to write its mouse sequence directly to the ConPTY host.
`VtEngine` already contains a number of specialized member functions
like `RequestWin32Input()` to ensure that `_Flush()` is called
immediately and another member could've been added to solve this issue.
This commit now adds `RequestMouseMode` in the same vein.

But I believe we can make the system more robust in general by using
eager flushing by default (= safe), similar to how a `write()` on a
TCP socket flushes by default, and instead only selectively pause and
unpause flushing with a system similar to `TCP_CORK`.

This seems to work fairly well, as it solves:
* The original nushell bug
* The new bug
* Improves overall throughput by ~33% (due to less flushing)

In particular the last point is noteworthy, as this commit removes
the last performance bottleneck in ConPTY that isn't `VtEngine`.
Around ~95% of all CPU and wall time is spent in there now and any
improvements to `VtEngine` should yield immediately results.

Closes #15711

## Validation Steps Performed
* Clone/Run https://github.com/chrisant996/repro_enable_mouse_input
* Hold Ctrl+Alt and circle with the mouse over the viewport
* Repro.exe prints the current cursor coordinates ✅
* Run nushell
* No flickering when typing in the prompt ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.19
Development

Successfully merging a pull request may close this issue.

ConPTY: Fix missing flush on console mode changes microsoft/terminal
4 participants
@DHowett @lhecker @chrisant996 @zadjii-msft