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

Make sure RIS re-enables win32 input and focus events #15476

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented May 29, 2023

When an RIS (hard reset) sequence is executed, ConHost will pass that
through to the connected conpty terminal, which results in all modes
being reset to their initial state. To work best, though, conpty needs
to have the win32 input mode enabled, as well as the focus event mode.
This PR addresses that by explicitly requesting the required modes after
an RIS is passed through.

Originally these modes were only set if the --win32input option was
given, but there is really no need for that, since terminals that don't
support them should simply ignore the request. To avoid that additional
complication, I've now removed the option (i.e. ConHost will now always
attempt to set the modes it needs).

I've manually confirmed that keypresses are still passed through with
win32 input sequences after a hard reset, and that focus events are
still being generated. I've also updated the existing conpty round-trip
test for RIS to confirm that it's now also passing through the mode
requests that it needs.

Closes #15461

@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label May 29, 2023
Comment on lines 518 to 523
else if (arg == WIN32_INPUT_MODE)
else if (std::regex_match(arg, std::wregex(L"--[A-Za-z0-9]+")))
{
_win32InputMode = true;
// Any other alphanumeric sequence starting with -- is likely
// an unrecognized argument that we need to ignore.
s_ConsumeArg(args, i);
hr = S_OK;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if you'll be happy with using a regex here, if that's going to add bloat to conhost. It looks to me like we're already using regex searches in the TextBuffer, but I'm not sure if that actually gets linked into conhost.

Worst case, we could make this only ignore the defunct --win32input option, but I thought it would be nice to have it dropping all the unsupported args, since there was already a TODO for that a few lines down from here.

Copy link
Member

@lhecker lhecker May 30, 2023

Choose a reason for hiding this comment

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

It's not linked into conhost as far as I know. I personally would like to replace it with ICU in the future so that we can get proper Unicode aware regex queries. ICU is also significantly faster, like in the order 10-100x. It can go through the entire 120x9001 text buffer full of enwik8 in <3ms with a worst-case query like e (or some other common ASCII character).

Is there any reason to not just check if the arg starts with -- and then ignore it? What other arguments could start with -- and not be followed by a-z0-9?

Copy link
Member

Choose a reason for hiding this comment

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

This, I am somewhat worried about. We go from explicitly rejecting unknowns to explicitly accepting them...

Since conpty.dll and openconsole.exe ship in lock-step, I would be comfortable just yanking all support for the argument and the special flag and everything. :)

After all . . . it never made it up to our public API surface on Windows!

Copy link
Member

Choose a reason for hiding this comment

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

This got me thinking, and I'd be interested in what you and Leonard and @zadjii-msft think.

This is speculative and not for this PR.

"In the future", we might want to add more types of conpty flags. Maybe. I don't love it, but maybe we will.

Mike and Michael used to prefer --real --arguments that set flags, rather than --flags 0xABCD to bulk enable all of 0xABCD. The argument was that it was easier spot in a debugger or a process list.

I still think there's value in having --flags 0xABCD, but this PR made me realize that maybe what we actually want is --flags.required 0x0001 --flags.optional 0x0002. Conforming versions of conhost can say, "I don't support flag 0x0001" if 0x0001 is too new for their blood... but they can ignore 0x0002 if they don't know what it is.

Is that YAGNI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there any reason to not just check if the arg starts with -- and then ignore it? What other arguments could start with -- and not be followed by a-z0-9?

That was actually my first attempt, but it caused this test case to fail:

commandline = L"conhost.exe --headless \"--vtmode bar this is the commandline\"";

So then I thought I should maybe also check that the arg contained no spaces, but that still caused this test to fail:

commandline = L"conhost.exe --headless\\ foo\\ --outpipe\\ bar\\ this\\ is\\ the\\ commandline";

And that's when I figured I'd need to go with a regex.

But maybe those test cases aren't really something we need to support. I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

But I need to ask again, are we sure this won't break any third-party terminals?

Fffffair, and sorry I didn't answer it at first. My official stance is, "we haven't built the conpty API for use without conpty.dll"; if somebody is invoking it directly and passing their own pipes, they are allowed to keep the pieces when it breaks.

The command line args are effectively an internal interface. Technically. I know somebody's caught us out for breaking them before, but that's because they were setting "conhost --pty" as their debugger to stop certain Windows processes from starting . . .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, that's just me not understanding how it works - that seems reasonable. But do I keep the PSEUDOCONSOLE_WIN32_INPUT_MODE flag, and just not do anything with it, or should that be nuked as well?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it would be fine for you to remove that flag in its entirety. :)

Copy link
Member

Choose a reason for hiding this comment

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

That's the other half of the equation. We haven't made a publicly supported API-stable release of ConPTY's new undocked interface, so anybody using it is taking on 0.x version level support. I'm happy to be the one with whom the buck stops if they come to ask!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is done now. I've dropped the regex test for the invalid args, and removed all the --win32input command line parameters as well as the PSEUDOCONSOLE flag. Everything still appears to work.

@j4james
Copy link
Collaborator Author

j4james commented May 29, 2023

I should also note that I haven't removed the code that sets the --win32input command line option when starting up a conpty connection, because I wasn't sure if that needed to remain for backwards compatibility, e.g. if connecting to an older version of conhost. If that's not something we need to worry about, then there's probably a bit more code we could get rid of.

@j4james j4james marked this pull request as ready for review May 30, 2023 00:24
@microsoft-github-policy-service microsoft-github-policy-service bot added Area-VT Virtual Terminal sequence support Product-Conpty For console issues specifically related to conpty labels Jun 1, 2023
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Outstanding. Thanks so much for tearing out all the vestiges of this thing 😄

@DHowett DHowett merged commit 8aefc7a into microsoft:main Jun 2, 2023
DHowett pushed a commit that referenced this pull request Jul 17, 2023
When an `RIS` (hard reset) sequence is executed, ConHost will pass that
through to the connected conpty terminal, which results in all modes
being reset to their initial state. To work best, though, conpty needs
to have the win32 input mode enabled, as well as the focus event mode.
This PR addresses that by explicitly requesting the required modes after
an `RIS` is passed through.

Originally these modes were only set if the `--win32input` option was
given, but there is really no need for that, since terminals that don't
support them should simply ignore the request. To avoid that additional
complication, I've now removed the option (i.e. ConHost will now always
attempt to set the modes it needs).

I've manually confirmed that keypresses are still passed through with
win32 input sequences after a hard reset, and that focus events are
still being generated. I've also updated the existing conpty round-trip
test for `RIS` to confirm that it's now also passing through the mode
requests that it needs.

Closes #15461

(cherry picked from commit 8aefc7a)
Service-Card-Id: 89409030
Service-Version: 1.17
DHowett pushed a commit that referenced this pull request Jul 27, 2023
When an `RIS` (hard reset) sequence is executed, ConHost will pass that
through to the connected conpty terminal, which results in all modes
being reset to their initial state. To work best, though, conpty needs
to have the win32 input mode enabled, as well as the focus event mode.
This PR addresses that by explicitly requesting the required modes after
an `RIS` is passed through.

Originally these modes were only set if the `--win32input` option was
given, but there is really no need for that, since terminals that don't
support them should simply ignore the request. To avoid that additional
complication, I've now removed the option (i.e. ConHost will now always
attempt to set the modes it needs).

I've manually confirmed that keypresses are still passed through with
win32 input sequences after a hard reset, and that focus events are
still being generated. I've also updated the existing conpty round-trip
test for `RIS` to confirm that it's now also passing through the mode
requests that it needs.

Closes #15461

(cherry picked from commit 8aefc7a)
Service-Card-Id: 89384907
Service-Version: 1.18
@j4james j4james deleted the fix-win32-input-reset branch January 2, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Projects
Development

Successfully merging this pull request may close these issues.

Hard reset disables win32-input-mode
3 participants