Disable win32 input mode on exit #16408
Conversation
lhecker
added
Product-Conpty
src/renderer/vt/state.cpp
Outdated
| // It's important that any additional modes set here are also mirrored in | ||
| // the AdaptDispatch::HardReset method, since that needs to re-enable them | ||
| // in the connected terminal after passing through an RIS sequence. | ||
| RETURN_IF_FAILED(_RequestWin32Input()); | ||
| RETURN_IF_FAILED(_RequestFocusEventMode()); | ||
| RETURN_IF_FAILED(_Write("\033[?9001;1004h")); |
There was a problem hiding this comment.
lol, straight up yeet them in. I love it actually.
There was a problem hiding this comment.
It's the same sequence as in AdaptDispatch::HardReset, which I felt like makes it easier to find.
|
I was thinking along the same lines as this, but my concern was that it might not work if the parent shell did actually want win32-input mode, i.e. you start off with win32-input mode enabled, you initiate a conpty session which enables it again, but when the conpty session ends we're now disabling it, which is not your initial state. Or is that not a possible scenario? |
|
I believe that's possible. For instance, if you run ConPTY inside ConPTY right? I was unsure how to solve that issue, but felt like that doing nothing is worse, as it breaks shell input in WSL instantly. But I just realized that if we encounter a |
I think this is pretty similar to "I need |
There was a problem hiding this comment.
Yea, I think I'm cool with this. I'd assume it's well understood that if you're a client app that needs specific modes and you call child processes, you need to set the modes back to what you need.
and if that isn't well known, well, (how did I not know that) and (yikes)
Absolutely. Even if that is all that it fixes, I'd be happy with this PR. It would be great if we could also restore the win32-input when it was actually wanted, but it wouldn't be the end of the world if we can't.
I honestly don't have a great understanding of what's going on when we're nesting conpty instances like this, but that sounds right to me. |
|
@j4james I tried to draw up a diagram, as I was having trouble understanding it myself. Here's what terror I came up with.
All this aside, this collection of PRs (mainly #16407) doesn't fix the deadlock when run under Terminal Stable. Here's my proposal...
What if we move this down here? It should only serve to improve the DSR-CPR startup exchange. |
When ConPTY exits it should attempt to restore the state as it was before it started. This is particularly important for the win32 input mode sequences, as Linux shells don't know what to do with it. Related to #16343 ## Validation Steps Performed * Replace conhost with this * Launch a Win32 application inside WSL * Exit that application * Shell prompt doesn't get filled with win32 input mode sequences ✅ (cherry picked from commit 70e51ae) Service-Card-Id: 91246943 Service-Version: 1.19
@DHowett For this to be of any benefit, your proposed fix would assumedly have to make its way into the inbox conhost before Terminal Stable gets the #16407 fix. Am I understanding that correctly? If that's a likely scenario, then it probably is worth doing. The only downside I can see is if the CPR response is slow, it might be possible that some keypresses arrive in VT format before win32-input mode kicks in. But that doesn't seem like a massive issue, assuming it's even possible at all. |
When ConPTY exits it should attempt to restore the state as it was
before it started. This is particularly important for the win32
input mode sequences, as Linux shells don't know what to do with it.
Related to #16343
Validation Steps Performed