Don't abort early in VT reset operations if one of the steps fails #6763
Conversation
ghost
added
Area-VT
|
We almost certainly need to book work to make passthrough a state decision that isn't based on whether an operation succeeded or failed 😄 |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 15 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 (
|
Maybe. Although I think this will eventually resolve itself once we get to implementing the passthrough mode (#1173). Because I have a hunch that the correct solution is to passthrough by default, and then have the current buffered refresh method only for special cases that can't be handled with direct VT sequences. But we're a long way from even attempting something like that. |
ghost
merged commit
|
Yeah, I think our best course forward for passthrough mode is to PT by default and take it a step further and convert APIs to emit VT wherever possible. We can keep a medium-fidelity grid/buffer representation for readback, and we can figure out what to do with APIs that address scrollback for either reading or writing, but having things be "VT-first" would go a long way... |
…6763) The VT reset operations `RIS` and `DECSTR` are implemented as a series of steps, each of which could potentially fail. Currently these operations abort as soon as an error is detected, which is particularly problematic in conpty mode, where some steps deliberately "fail" to indicate that they need to be "passed through" to the conpty client. As a result, the reset won't be fully executed. This PR changes that behaviour, so the error state is recorded for any failures, but the subsequent steps are still run. Originally the structure of these operations was of the form: bool success = DoSomething(); if (success) { success = DoSomethingElse(); } But I've now changed the code so it looks more like this: bool success = DoSomething(); success = DoSomethingElse() && success; This means that every one of the steps should execute, regardless of whether previous steps were successful, but the final _success_ state will only be true if none of the steps has failed. While this is only really an issue in the conhost code, I've updated both the `AdaptDispatch` and `TerminalDispatch` classes, since I thought it would be best to have them in sync, and in general this seems like a better way to handle multi-step operations anyway. VALIDATION I've manually tested the `RIS` escape sequence (`\ec`) in the Windows Terminal, and confirmed that it now correctly resets the cursor position, which it wasn't doing before. Closes #6545 (cherry picked from commit 0651fcf)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
|
🎉 Handy links: |
The VT reset operations
RISandDECSTRare implemented as a seriesof steps, each of which could potentially fail. Currently these
operations abort as soon as an error is detected, which is particularly
problematic in conpty mode, where some steps deliberately "fail" to
indicate that they need to be "passed through" to the conpty client. As
a result, the reset won't be fully executed. This PR changes that
behaviour, so the error state is recorded for any failures, but the
subsequent steps are still run.
Originally the structure of these operations was of the form:
But I've now changed the code so it looks more like this:
This means that every one of the steps should execute, regardless of
whether previous steps were successful, but the final success state
will only be true if none of the steps has failed.
While this is only really an issue in the conhost code, I've updated
both the
AdaptDispatchandTerminalDispatchclasses, since I thoughtit would be best to have them in sync, and in general this seems like a
better way to handle multi-step operations anyway.
VALIDATION
I've manually tested the
RISescape sequence (\ec) in the WindowsTerminal, and confirmed that it now correctly resets the cursor
position, which it wasn't doing before.
Closes #6545