Improve the DECSC/DECRC implementation #3160
Conversation
…ctive screen buffer.
…rent text attributes and active character set.
…ain buffer, and save or restore the state when switching buffers.
…rolling region if the origin mode is relative.
DHowett-MSFT
added
Product-Conhost
There was a problem hiding this comment.
I kinda got a little off-topic on this review. I think it's probably a fine and safe change, though I'm holding off with an approval mostly due to where we are in the internal release cycle. @DHowett-MSFT can we accept conhost PRs currently?
| // - pAttrs: Receives the TextAttribute value. | ||
| // Return Value: | ||
| // - TRUE if successful. FALSE otherwise. | ||
| BOOL ConhostInternalGetSet::PrivateGetTextAttributes(TextAttribute* const pAttrs) const |
There was a problem hiding this comment.
I suppose this change was inevitable. We've kinda just been patching around not just exposing the entire TextAttribute through this layer, but this might be easier. I know there was previously reluctance to doing this, something about it exposing too much of the implementation details of how conhost itself was implemented. @miniksa might remember more. I think we've come a long way since then, and this might be reasonable nowadays.
I guess from a design perspective, it would change things pretty dramatically - because the adapter could directly control the state of the text attributes, it could just avoid using any of the other functions, and put all the combined attribute manipulation logic directly in the adapter. There might be other things that conhost needs to do during some of these calls that might get circumnavigated by just using this method. These things might be a little trickier to move up into the adapter. I haven't dug in on this too much, so it might not be a problem.
If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.
The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...
There was a problem hiding this comment.
If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.
Absolutely! I had exactly the same thought when I added this. There's a lot of weirdness in the current SGR implementation which I think could be greatly simplified if we had access to an API like this. Was going to raise a separate issue for that once I knew you were happy with the API and I had some time to play around with potential implementations.
The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...
If there was a need for conhost and Terminal to handle this functionality differently, that's already not a problem, since Terminal shares none of the AdaptDispatch implementation. That said, I'm not sure that's the right approach in the long term, because it seems we're slowly reimplementing all of the AdaptDispatch code in TerminalDispatch when those implementations should really be identical 99% of the time. But that's a topic for another issue.
There was a problem hiding this comment.
If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.
Absolutely! I had exactly the same thought when I added this. There's a lot of weirdness in the current SGR implementation which I think could be greatly simplified if we had access to an API like this. Was going to raise a separate issue for that once I knew you were happy with the API and I had some time to play around with potential implementations.
I'm OK with additional private methods being added here, I just don't want to expose any more publicly. @zadjii-msft, I'm not sure I had an extreme reticence to adding new private APIs given we've done a ton of them. I think it was more "I'm going to try to accomplish as much as possible without adding new APIs at all" and it led us to the non-ideal situations that we had and discovered over time.
The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...
If there was a need for conhost and Terminal to handle this functionality differently, that's already not a problem, since Terminal shares none of the
AdaptDispatchimplementation. That said, I'm not sure that's the right approach in the long term, because it seems we're slowly reimplementing all of theAdaptDispatchcode inTerminalDispatchwhen those implementations should really be identical 99% of the time. But that's a topic for another issue.
Yes, if we're duplicating a big portion of the dispatch... then we probably need to somehow abstract the buffers or whatever the two adapters are calling and make that a shared component. Feel free to file another issue for that if you feel you can articulate it.
|
We can accept conhost changes, but I am only flowing high-priority fixes back into Windows 20H1. |
| // - pAttrs: Receives the TextAttribute value. | ||
| // Return Value: | ||
| // - TRUE if successful. FALSE otherwise. | ||
| BOOL ConhostInternalGetSet::PrivateGetTextAttributes(TextAttribute* const pAttrs) const |
There was a problem hiding this comment.
If it was possible to move this logic into the adapter, then we could probably get rid of a bunch of these more specific boilerplate-y functions. That would probably make life easier for everyone.
Absolutely! I had exactly the same thought when I added this. There's a lot of weirdness in the current SGR implementation which I think could be greatly simplified if we had access to an API like this. Was going to raise a separate issue for that once I knew you were happy with the API and I had some time to play around with potential implementations.
I'm OK with additional private methods being added here, I just don't want to expose any more publicly. @zadjii-msft, I'm not sure I had an extreme reticence to adding new private APIs given we've done a ton of them. I think it was more "I'm going to try to accomplish as much as possible without adding new APIs at all" and it led us to the non-ideal situations that we had and discovered over time.
The alternative here would be exposing a PrivateSaveState/PrivateRestoreState pair that lets conhost implement it however it likes. Though, when I think about it, the implementation would be highly similar between conhost and Terminal...
If there was a need for conhost and Terminal to handle this functionality differently, that's already not a problem, since Terminal shares none of the
AdaptDispatchimplementation. That said, I'm not sure that's the right approach in the long term, because it seems we're slowly reimplementing all of theAdaptDispatchcode inTerminalDispatchwhen those implementations should really be identical 99% of the time. But that's a topic for another issue.
Yes, if we're duplicating a big portion of the dispatch... then we probably need to somehow abstract the buffers or whatever the two adapters are calling and make that a shared component. Feel free to file another issue for that if you feel you can articulate it.
There was a problem hiding this comment.
This looks great. Thanks, @j4james! Quick question about standards: this implementation seems to support restoring the saved cursor position any number of times. Is that expected? If so, awesome. If not, is there a lack of consensus on whether to support it across other terminal emulators?
Yep, this is intentional. Quoting the DEC STD 070 documentation for
i.e. the |
|
Excellent. Thanks! |
|
🎉 Handy links: |
ghost
mentioned this pull request
Summary of the Pull Request
The current DECSC implementation only saves the cursor position and origin mode. This PR improves that functionality with additional support for saving the SGR attributes, as well as the active character set.
It also fixes the way the saved state interacts with the alt buffer (private mode 1049), triggering a save when switching to the alt buffer, and a restore when switching back, and tracking the alt buffer state independently from the main state.
PR Checklist
Detailed Description of the Pull Request / Additional comments
In order to properly save and restore the SGR attributes, we first needed to add a pair of APIs in the
ConGetSetinterface which could round-trip the attributes with full 32-bit colors (the existing methods only work with legacy attributes).I then added a struct in the
AdaptDispatchimplementation to make it easier to manage all of the parameters that needed to be saved. This includes the cursor position and origin mode that we were already tracking, and now also the SGR text attributes and the active character set (via theTermOutputclass).Two instances of this structure are required, since changes made to the saved state in the alt buffer need to be tracked separately from changes in the main buffer. I've added a boolean property that specifies whether we're in the alt buffer or not, and use that to decide which of the two instances we're working with.
I also needed to explicitly trigger a save when switching to the alt buffer, and a restore when switching back, since we weren't already doing that (the existing implementation gave the impression that the state was saved, because each buffer has its own cursor position, but that doesn't properly match the XTerm behaviour).
For the state tracking itself, we've now got two additional properties - the SGR attributes, which we obtain via the new private API, and the active character set, which we get from a local
AdaptDispatchfield. I'm saving the wholeTermOutputclass for the character set, since I'm hoping that will make it automatically supports future enhancements.When restoring the cursor position, there is also now a fix to handle the relative origin mode correctly. If the margins are changed between the position being saved and restored, it's possible for the cursor to end up outside of the new margins, which would be illegal. So there is now an additional step that clamps the Y coordinate within the margin boundaries if the origin mode is relative.
Validation Steps Performed
I've added a couple of screen buffer tests which check that the various parameters are saved and restored as expected, as well as checking that the Y coordinate is clamped appropriately when the relative origin mode is set.
I've also tested manually with Vttest and confirmed that the SAVE/RESTORE CURSOR test (the last page of the Test of screen features)) is now working a lot better than it used to.