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.

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

Update so that cursor size/type/isblinkingallowed get restored when switching back to main buffer from alt buffer #17154

Conversation

e82eric
Copy link
Contributor

@e82eric e82eric commented Apr 29, 2024

Summary of the Pull Request

When switching to a application that uses the alt buffer, the cursor continues to use the alt buffer applications cursor type when switched back to the main buffer.

I think that this is from this logic in Terminal::UseMainScreenBuffer().

{
const auto& altCursor = altBuffer->GetCursor();
auto& mainCursor = _mainBuffer->GetCursor();
mainCursor.SetStyle(altCursor.GetSize(), altCursor.GetType());
mainCursor.SetIsVisible(altCursor.IsVisible());
mainCursor.SetBlinkingAllowed(altCursor.IsBlinkingAllowed());
auto tgtCursorPos = altCursor.GetPosition();
tgtCursorPos.y += _mutableViewport.Top();
mainCursor.SetPosition(tgtCursorPos);
}

Based on these comments, I made the updates in AdaptDispatch::CursorRestoreState where it looked like it was restoring the cursor position.

// After exiting the alt buffer, the main buffer should adopt the current cursor position and style.
// This is the equal and opposite effect of what we did in UseAlternateScreenBuffer and matches xterm.
//
// We have to do this after the call to UserResize() to ensure that the TextBuffer sizes match up.
// Otherwise the cursor position may be temporarily out of bounds and some code may choke on that.

https://github.com/e82eric/terminal/blob/91fed9ca3c7b9a89612a7ab9569330a642684a84/src/terminal/adapter/adaptDispatch.cpp#L540

BlockCursor

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

switching back to main buffer from alt buffer
@@ -514,6 +514,9 @@ bool AdaptDispatch::CursorSaveState()
savedCursorState.IsOriginModeRelative = _modes.test(Mode::Origin);
savedCursorState.Attributes = attributes;
savedCursorState.TermOutput = _termOutput;
savedCursorState.CursorType = textBuffer.GetCursor().GetType();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refer to the standards if this is allowed:
https://vt100.net/docs/vt510-rm/DECSC.html
https://vt100.net/docs/vt510-rm/DECRC.html

Do you know what other terminals do in this case? Do they restore the cursor style too?

Copy link
Contributor Author

@e82eric e82eric Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, didn't even realize there was a spec for this. (I think we can close this if it doesn't match) Do I have it right that it doesn't say if the cursor style should be saved or not?

Did a quick pass over some emulators.
conhost - did restore the blinking _
alacrity - did not restore the blinking |
kitty - did restore the blinking |
wezterm - did restore the blinking |

Copy link
Contributor

@tusharsnx tusharsnx Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two things at play:

  1. Switching Alternate buffer -> Main buffer.
  2. Cursor save and restore.

And I suspect (1) is responsible for restoring the cursor style.

To test the theory:

  1. Started with blinking block cursor:
pwsh $ "`e[1 q"
  1. Then ran the command which:
    1. Activates the alternate buffer.
    2. Sets the cursor to steady block.
    3. Activates the main buffer again.
pwsh $ "`e[?1049h `e[2 q `e[?1049l"
  1. I can still see the blinking cursor.

That means at least wezterm keeps a copy of the cursor style to restore when the alternate buffer exits.


And DECSC & DECRC don't restore cursor style even on Wezterm:

Again, if I start with a blinking cursor, then save the cursor, change the cursor to a steady block, and restore the cursor, the result is a steady block, not the old style.

pwsh $ "`e7 `e[2 q `e8"

I've only Wezterm installed so I couldn't test the other terminals 😅

Copy link
Contributor Author

@e82eric e82eric Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand now, AdaptDispatch::CursorRestoreState is the implementation of DECSC/DECSR and is only meant to restore the cursor position and not the cursor style. (Will see if I can figure out how wezterm and kitty are handling the cursor style and alt/main buffer)

It looked like Alacrity and Kitty both just moved the cursor when I ran this.
"`e7 `e[2 q `e8"

One thing that is weird is that my local build with the change didn't restore the blinking cursor when I ran.
"`e7 `e[2 q `e8" (Will try to understand this also)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked like Alacrity and Kitty both just moved the cursor when I ran this.
"`e7 `e[2 q `e8"

That is to be expected as I added extra spaces to visually differentiate the escape codes in the comment 😅. You can use "`e7`e[2 q`e8" instead (note the single space here which is a required one).

Copy link
Contributor Author

@e82eric e82eric Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I kind of understand what is going on now. I tried setting a breakpoint on AdaptDispatch::CursorSaveState() and tried pwsh $ "`e7" and noticed that the break point wasn't hit.

My theory is that powershell wasn't sending the DECSC. (I think this was why wezterm didn't restore the cursor. (Because powershell didn't execute the DECSC or the DECSR)

I tried the following in kitty and observed the position and style was restored this time using bash.
echo -e "\e[4 q"
echo -e "\e7"
echo -e "\e[2 q"
echo -e "\e8"

I tried the same with wezterm using bash instead of powershell and observed the same.

Looking at the wezterm source code, it looks like they do use DECSC/DECSR to save/restore the cursor style.
https://github.com/wez/wezterm/blob/main/termwiz/src/cell.rs#L63
https://github.com/wez/wezterm/blob/main/termwiz/src/cell.rs#L45
https://github.com/wez/wezterm/blob/main/term/src/terminalstate/mod.rs#L136
https://github.com/wez/wezterm/blob/main/term/src/terminalstate/mod.rs#L2594
https://github.com/wez/wezterm/blob/main/term/src/terminalstate/mod.rs#L2637

For Kitty, it looks similar where it copies the whole cursor state (including the style) on save and restores the whole cursor state on restore (assuming I am looking at the right location).
https://github.com/kovidgoyal/kitty/blob/aecf07bcbaa74507526d3f700954842176fef4f2/kitty/screen.c#L1650
https://github.com/kovidgoyal/kitty/blob/master/kitty/cursor.c#L247
https://github.com/kovidgoyal/kitty/blob/aecf07bcbaa74507526d3f700954842176fef4f2/kitty/data-types.h#L298
https://github.com/kovidgoyal/kitty/blob/aecf07bcbaa74507526d3f700954842176fef4f2/kitty/screen.c#L1737

I guess this doesn't answer the question of if DECSC/DECSR should be overloaded to save/restore the style though.

Weztem with bash.
wezterm-bash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My theory is that Powershell wasn't sending the DECSC. (I think this was why wezterm didn't restore the cursor. (Because powershell didn't execute the DECSC or the DECSR)

On Windows, all sequences are first processed by Conpty which sits in between the terminal and applications (e.g., Powershell). Conpty then sends only the end result of such sequences to the terminal. In this case, DECSC & DECRC are intercepted and only the end result, i.e. the new cursor position is sent to the terminal via CUP sequence, e.g., "`e[21;1H" which just says, place the cursor at row: 21, col: 1.

Unfortunately, this also means that even the terminals that do support cursor-style restoration with DECRC won't be able to do that because Conpty wouldn't forward those sequences to them.

If you'd like to see what happens within Conpty when it receives DECSC or DECRC, then you need to attach to the openconsole.exe that is acting as a Conpty for that particular WT (Dev) instance. It should be running in the background.

@zadjii-msft
Copy link
Member

@j4james is usually The Guy to weigh in on things like this

@j4james
Copy link
Collaborator

j4james commented Apr 30, 2024

As @tusharsnx noted above, DECSC and DECRC are defined in the DEC documentation, and there is no mention of the cursor style being saved and restored by those sequences (both the VT510 and the VT520/525 manuals say the same thing).

And private mode 1049 is defined by XTerm with reference to DECSC and DECRC.

When set: Save cursor as in DECSC, xterm. After saving the cursor, switch to the Alternate Screen Buffer, clearing it first.
When reset: Use Normal Screen Buffer and restore cursor as in DECRC, xterm.

I also had a look through my terminal collection, and other than Kitty and WezTerm, the only other terminal I could find that restores the cursor style with DECRC is PangoTerm.

Given the evidence above, I'm fairly certain our existing behavior is correct as is.

@e82eric
Copy link
Contributor Author

e82eric commented Apr 30, 2024

As @tusharsnx noted above, DECSC and DECRC are defined in the DEC documentation, and there is no mention of the cursor style being saved and restored by those sequences (both the VT510 and the VT520/525 manuals say the same thing).

And private mode 1049 is defined by XTerm with reference to DECSC and DECRC.

When set: Save cursor as in DECSC, xterm. After saving the cursor, switch to the Alternate Screen Buffer, clearing it first.
When reset: Use Normal Screen Buffer and restore cursor as in DECRC, xterm.

I also had a look through my terminal collection, and other than Kitty and WezTerm, the only other terminal I could find that restores the cursor style with DECRC is PangoTerm.

Given the evidence above, I'm fairly certain our existing behavior is correct as is.

@j4james that makes sense, I think this can be closed.

Just out of curiosity, am I right that "`e7" doesn't send a DECSC in PowerShell? Is there a way to actually send that?

@j4james
Copy link
Collaborator

j4james commented Apr 30, 2024

Just out of curiosity, am I right that "`e7" doesn't send a DECSC in PowerShell?

No, it definitely should work. Simple test case: "`e7`e[31m RED`e8NOT"

image

Although if you're using conhost rather than Windows Terminal, and you're on Windows 10, then you're likely using a version that's many years out of date, so you may be running into some bugs in the original DECSC implementation.

And if you're debugging Windows Terminal, you're not going to get a breakpoint in AdaptDispatch::CursorSaveState, because the DECSC is not handled at that level. You would need to debug the underlying conhost instance, by attaching to the appropriate OpenConsole process. But for VT development you're generally better off just running OpenConsole directly.

@zadjii-msft
Copy link
Member

Thanks for the discussion all! I think we all learned something about well-defined standards (and the lack thereof) 😄

@e82eric
Copy link
Contributor Author

e82eric commented Apr 30, 2024

@tusharsnx @j4james @zadjii-msft thank you for walking me through all of this. I learned a lot from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants