Initialize the VT tab stops when a buffer is created in VT mode #2816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zadjii-msft
added
Issue-Bug
carlos-zamora
approved these changes
Sep 19, 2019
DHowett-MSFT
suggested changes
Sep 19, 2019
ghost
added
Needs-Author-Feedback
DHowett-MSFT
approved these changes
Sep 19, 2019
|
Huh, this broke the input line tests somehow. |
Kinda raises more questions tbh
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 23, 2019
* fixes #411 * update this comment to actually match * run this test in isolation so it doesn't break other tests, @DHowett-MSFT * This fixes the test that's broken? Kinda raises more questions tbh (cherry picked from commit dfaaa44)
|
🎉 Handy links: |
ghost
mentioned this pull request
5 tasks
ghost
pushed a commit
that referenced
this pull request
Apr 1, 2020
## Summary of the Pull Request This is essentially a rewrite of the VT tab stop functionality, implemented entirely within the `AdaptDispatch` class. This significantly simplifies the `ConGetSet` interface, and should hopefully make it easier to share the functionality with the Windows Terminal VT implementation in the future. By removing the dependence on the `SCREEN_INFORMATION` class, it fixes the problem of the the tab state not being preserved when switching between the main and alternate buffers. And the new architecture also fixes problems with the tabs not being correctly initialized when the screen is resized. ## References This fixes one aspect of issue #3545. It also supersedes the fix for #411 (PR #2816). I'm hoping the simplification of `ConGetSet` will help with #3849. ## PR Checklist * [x] Closes #4669 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments In the new tab architecture, there is now a `vector<bool>` (__tabStopColumns_), which tracks whether any particular column is a tab stop or not. There is also a __initDefaultTabStops_ flag indicating whether the default tab stop positions need to be initialised when the screen is resized. The way this works, the vector is initially empty, and only initialized (to the current width of the screen) when it needs to be used. When the vector grows in size, the __initDefaultTabStops_ flag determines whether the new columns are set to false, or if every 8th column is set to true. By default we want the latter behaviour - newly revealed columns should have default tab stops assigned to them - so __initDefaultTabStops_ is set to true. However, after a `TBC 3` operation (i.e. we've cleared all tab stops), there should be no tab stops in any newly revealed columns, so __initDefaultTabStops_ is set to false. Note that the __tabStopColumns_ vector is never made smaller when the window is shrunk, and that way it can preserve the state of tab stops that are off screen, but which may come into range if the window is made bigger again. However, we can can still reset the vector completely after an `RIS` or `TBC 3` operation, since the state can then be reconstructed automatically based on just the __initDefaultTabStops_ flag. ## Validation Steps Performed The original screen buffer tests had to be rewritten to set and query the tab stop state using escape sequences rather than interacting with the `SCREEN_INFORMATION` class directly, but otherwise the structure of most tests remained largely the same. However, the alt buffer test was significantly rewritten, since the original behaviour was incorrect, and the initialization test was dropped completely, since it was no longer applicable. The adapter tests were also dropped, since they were testing the `ConGetSet` interface which has now been removed. I also had to make an addition to the method setup of the screen buffer tests (making sure the viewport was appropriately initialized), since there were some tests (unrelated to tab stops) that were previously dependent on the state being set in the tab initialization test which has now been removed. I've manually tested the issue described in #4669 and confirmed that the tabs now produce the correct spacing after a resize.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
If a user has
VirtualTerminalLevelset, thenSetConsoleMode(ENABLE_VIRTUAL_TERMINAL_PROCESSING)won't set our tab stops, because we're never going from vt off -> on.So, if VT mode is enabled, construct the VT tab stops when the buffer is created.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Big shout-out to @j4james for basically doing all the work on what I thought was an impossible bug. Thanks!