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.

Sign up for GitHub

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

Jump to bottom

Alt buffer should share more state with main buffer #3545

Closed
j4james opened this issue Nov 12, 2019 · 2 comments · Fixed by #10843
Closed

Alt buffer should share more state with main buffer #3545

j4james opened this issue Nov 12, 2019 · 2 comments · Fixed by #10843
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Console Backlog

Comments

@j4james
Copy link
Collaborator

j4james commented Nov 12, 2019

Environment

Windows build number: Version 10.0.18362.356

Steps to reproduce

Open a WSL shell in conhost and execute the following command:

printf "\ec\e[1;15r\e[3g\e[1;20H\eH\e[?25l\e[9;20H\e[?1049hstart\r\e[99Bmargin\e[H\ttab\n"

This initialises a number of terminal properties.

  • The scrolling margins are set to 1:15 (DECSTBM).
  • The tabstops are cleared and then a single tabstop is set at column 20 (TBC/HTS).
  • The cursor blinking is disabled (DECTCEM).
  • The cursor position is set to line 9 column 20 (CUP).

It then switches to the alternate screen buffer (private mode 1049) and "tests" the state.

  • The text "start" is output to indicate the starting position.
  • The cursor is moved down 99 lines (CUD), and the text "margin" is output to indicate the bottom margin position.
  • The cursor is moved to the home position (CUP) and a tab is output, followed by the text "tab" to indicate the position of the tab stop.

Expected behavior

I would expect all of the initial state to be inherited by the alt buffer, producing the following results:

  • The "start" text should be on line 9, column 20, i.e. the position before the switch to the alt buffer.
  • The "margin" text should be on line 15, the bottom margin.
  • The "tab" text should be in column 20, the position of the tab stop that was set.
  • The cursor should be invisible.

Here's what the output looks like in XTerm:

image

Actual behavior

When switching to the alt buffer in conhost, we reset all those properties, producing the following results:

  • The "start" text is in the top left corner.
  • The "margin" text is at the bottom of the screen.
  • The "tab" text is in column 9.
  • The cursor is visible.

Here's what our output looks like:

image

The reason for this behaviour is because these properties are part of the SCREEN_INFORMATION class, and the alternate buffer is implemented as a separate instance of that class, when it should really be sharing much of that state with the main buffer.

The scroll margins are a little more complicated, since those properties are also duplicated in the AdaptDispatch class. And this can result in some weirdness when switching to the alt buffer since the two sets of values can end up out of sync.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 12, 2019
@DHowett-MSFT DHowett-MSFT added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Nov 14, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 14, 2019
@DHowett-MSFT DHowett-MSFT added Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 14, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Nov 14, 2019
@j4james j4james mentioned this issue Nov 24, 2019
Open
@j4james j4james mentioned this issue Mar 29, 2020
Merged
5 tasks
ghost pushed a commit that referenced this issue Apr 1, 2020
@j4james
Reimplement the VT tab stop functionality (#5173)
9a0b6e3 
## 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.
@j4james j4james mentioned this issue Aug 19, 2020
Closed
@j4james j4james mentioned this issue Aug 1, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 1, 2021
@ghost ghost closed this as completed in #10843 Aug 2, 2021
ghost pushed a commit that referenced this issue Aug 2, 2021
@j4james
Make the alt buffer inherit cursor state from the main buffer (#10843)
6936ee1 
When switching to the alt buffer, the starting cursor position, style,
and visibility is meant to be inherited from the main buffer. Similarly,
when returning to the main buffer, any changes made to those attributes
should be copied back (with the exception of the cursor position, which
is restored to its original state). This PR makes sure we handle that
cursor state correctly.

At some point I'd like to move the cursor state out of the
`SCREEN_INFORMATION` class, which would make this inheritance problem a
non-issue. For now, though, I've just made it copy the state from the
main buffer when creating the alt buffer, and copy it back when
returning to the main buffer.

## Validation Steps Performed

I've added some unit tests to verify the cursor state is inherited
correctly when switching to the alt buffer and back again. I also had to
make a small change to one of the existing alt buffer test that relied
on the initial cursor position being at 0;0, which is no longer the
case.

I've verified that the test case in issue #3545 is now working
correctly. I've also confirmed that this fixes a problem in the
_notcurses_ demo, where the cursor was showing when it should have been
hidden.

Closes #3545
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 2, 2021
DHowett pushed a commit that referenced this issue Aug 25, 2021
@j4james @DHowett
Make the alt buffer inherit cursor state from the main buffer (#10843)
6c46b17 
When switching to the alt buffer, the starting cursor position, style,
and visibility is meant to be inherited from the main buffer. Similarly,
when returning to the main buffer, any changes made to those attributes
should be copied back (with the exception of the cursor position, which
is restored to its original state). This PR makes sure we handle that
cursor state correctly.

At some point I'd like to move the cursor state out of the
`SCREEN_INFORMATION` class, which would make this inheritance problem a
non-issue. For now, though, I've just made it copy the state from the
main buffer when creating the alt buffer, and copy it back when
returning to the main buffer.

## Validation Steps Performed

I've added some unit tests to verify the cursor state is inherited
correctly when switching to the alt buffer and back again. I also had to
make a small change to one of the existing alt buffer test that relied
on the initial cursor position being at 0;0, which is no longer the
case.

I've verified that the test case in issue #3545 is now working
correctly. I've also confirmed that this fixes a problem in the
_notcurses_ demo, where the cursor was showing when it should have been
hidden.

Closes #3545
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10843, which has now been successfully released as Windows Terminal Preview v1.10.2383.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10843, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Console Backlog
Development

Successfully merging a pull request may close this issue.

Make the alt buffer inherit cursor state from the main buffer j4james/terminal
2 participants
@j4james @DHowett-MSFT