Add support for DECSCUSR "0" to restore cursor to user default by skyline75489 · Pull Request #7379 · microsoft/terminal

skyline75489 · 2020-08-24T12:16:38Z

This PR is about the behavior of DECSCUSR. This PR changes the meaning
of DECSCUSR 0 to restore the cursor style back to user default. This
differs from what VT spec says but it’s used in popular terminal
emulators like iTerm2 and VTE-based ones. See #1604.

Another change is that for parameter greater than 6, DECSCUSR should be
ignored, instead of restoring the cursor to legacy. This PR fixes it.
See #7382.

Fixes #1604.

skyline75489 · 2020-08-24T12:17:45Z

I don't think this actually fixes #1604 as vim does not attempt to restore cursor shape on exit. So..what do you guys think?

skyline75489 · 2020-08-24T12:45:14Z

Hey seems we got VS 16.7 for CI build now?

src/terminal/adapter/DispatchTypes.hpp

src/cascadia/TerminalCore/TerminalApi.cpp

src/terminal/adapter/adaptDispatch.cpp

src/terminal/parser/OutputStateMachineEngine.cpp

src/terminal/parser/OutputStateMachineEngine.hpp

…e75489/terminal into chesterliu/dev/decscusr-0

src/cascadia/TerminalCore/TerminalApi.cpp

src/terminal/adapter/DispatchTypes.hpp

miniksa

Looks good to me. Thanks @j4james for making the first pass and @skyline75489 for fixing those things up before we got back around to this.

src/terminal/adapter/adaptDispatch.cpp

j4james · 2020-08-25T18:28:50Z

src/terminal/adapter/adaptDispatch.cpp

    case DispatchTypes::CursorStyle::BlinkingBlock:
-    case DispatchTypes::CursorStyle::BlinkingBlockDefault:
        fEnableBlinking = true;
        actualType = CursorType::FullBox;
        break;


I don't think the behaviour for conhost is right. We should either leave UserDefault as an alias for BlinkingBlock, which is at least compatible with the DEC standard, or we should try and map it to the actual user preference (which I'm honestly not sure how to do). Worst case we could possibly map it to blinking legacy, which I think is the system default. But as it stands, it looks like you're going to get non-blinking legacy, which is neither one thing nor the other.

Edit: Sorry this is essentially a long-winded dup of DHowett's comment.

More detail = more good

zadjii-msft

I don't think I really have anything to add that Dustin and James didn't already - We should probably add the plumbing for DispatchTypes::CursorStyle PrivateGetDefaultCursorShape() to getset, conGetSet, outputStream, etc, so that adaptDispatch can do the right thing for conhost, but everything else looks good here.

zadjii-msft

actually you know what, I'll block over that

skyline75489 · 2020-08-25T23:52:12Z

This may be irrelavant but when I'm using stock cmd.exe and then wsl.exe, the cursor will restore to default after vim exits. I don't fully get it why..

Update: it actually restores the cursor style to default, not just vintage. If I change my preference in the propsheet Dustin mentioned, it will restore to that style accordingly.

DHowett · 2020-08-26T00:13:52Z

the cursor will restore to default after vim exits

vim was written for versions of windows that did not support VT, and therefore it almost exclusively uses the traditional console APIs. This includes a custom reimplementation of alternate buffers and manual cursor management.

skyline75489 · 2020-08-26T00:15:34Z

@DHowett But I'm using vim in WSL, right? So it's a Linux Vim, not win32 vim.

DHowett · 2020-08-26T00:18:44Z

using WSL vim

oh, right. whoops. :)

~~I do not believe that it restores the cursor state by default.~~ i will continue to be unable to apply reading comprehension :)

zadjii-msft · 2020-08-28T14:35:31Z

I'm gonna hold off on merging this for a bit to give @j4james a chance to voice any objections, since he's still on "requested changes".

j4james · 2020-08-28T18:53:30Z

src/cascadia/TerminalCore/TerminalApi.cpp

    default:
-        finalCursorType = CursorType::Legacy;
-        shouldBlink = false;
+        return true;


Nit: If we're now treating unrecognised parameters as a no-op in Windows Terminal (which is good), should we not be doing the same thing for conhost in AdaptDispatch?

I don’t know how can we debug conhost now. So I intended to keep this. By the way do we want to fix this kind of small bug in conhost, or do we want to keep its behavior. I mean conhost itself is the legacy thing that shouldn’t be changed much for compatibility

None of the VT code in conhost is legacy, and this area of the code base is changing all the time. But we can always fix it in a follow-up PR if you don't want to do it now.

Thanks. I’m a little uncomfortable that I cannot unittest or manually test conhost. So I don’t feel right about changing that.

I think there’s no need to rush this. And Dustin is not available the next several days. I’m OK waiting for more voices and comments

If you look in the Conhost folder of the of the OpenConsole solution, there's a Host.EXE project that you can run to debug conhost. In the Debugging section of the properties, the Command Arguments specify what shell to use - cmd.exe, bash.exe, etc.

As for the conhost unit testing, it'll depend on what exactly you're doing, but often that will be handled in ScreenBufferTests.cpp and/or adapterTest.cpp.

j4james

Sorry for the delay. I only have one nit, and it's not a big deal, so I'm happy to approve this.

skyline75489 · 2020-08-29T00:11:25Z

src/host/outputStream.cpp

+bool ConhostInternalGetSet::GetUserDefaultCursorStyle(CursorType& style)
+{
+    const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    style = gci.GetCursorType();


If we’re on older Windows 10 or Windows 7, will this still work? I assume it will always return legacy.

I believe the VT support was only added to conhost in build 10586 of Windows 10, so nothing prior to that should have any of this code. Maybe one of the core devs can confirm this, but I don't think we need to worry about older versions of Windows at all.

Yeah, this is totally fine. Conhost is also shipped as a totally independent unit -- so if conhost can run on an older version of Windows, it will bring with it full VT support and this cursor change.

(incidentally, it works on Windows 8.1 and brings full VT support 😉)

skyline75489 · 2020-08-30T11:12:22Z

src/terminal/adapter/adaptDispatch.cpp

+
+    default:
+        // Invalid argument should be ignored.
+        return true;


I think it's fair to treat parameter larger than 6 as a "success". I mean, why bother sending it to the connected terminal when we know it should be ignored.

Well now that you mention it, technically we should probably be passing through any unknown parameters. The connected terminal is not necessarily WT, and it could potentially support values larger than 6. We were actually just discussing this in issue #7382 (comment). I don't think it's a big deal, but returning false here is probably better (assuming that doesn't cause any other problems).

That makes sense. I wasn't really sure about it. Let's try 'false' for this.

src/terminal/adapter/ut_adapter/adapterTest.cpp

DHowett

This is excellent. Thanks!

ghost · 2020-09-04T20:06:10Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2020-09-22T16:01:30Z

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

younger-1 · 2022-02-15T02:45:36Z

I don't think this actually fixes #1604 as vim does not attempt to restore cursor shape on exit. So..what do you guys think?

neovim and vim both do not restore cursor on exit if without configuring vimrc...

ubaldot · 2024-09-19T08:05:39Z

I don't think this actually fixes #1604 as vim does not attempt to restore cursor shape on exit. So..what do you guys think?

neovim and vim both do not restore cursor on exit if without configuring vimrc...

I bumped into the same issue: shall I then explicitly use au VimLeave * set guicursor=a:hor10-blinkon1in my vimrc to get my cursor back?

Also, I have noticed that when accessing Vim the cursor won't take the color of the used colorscheme. Any idea why and how to fix it?

skyline75489 added 3 commits August 24, 2020 19:00

Init

4552c63

Fix

d4bf404

Style

93aa1d8

j4james suggested changes Aug 24, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 24, 2020

skyline75489 added 3 commits August 25, 2020 11:45

Merge branch 'master' into chesterliu/dev/decscusr-0

14b0ff3

Comment

76e6329

Merge branch 'chesterliu/dev/decscusr-0' of https://github.com/skylin…

808e61b

…e75489/terminal into chesterliu/dev/decscusr-0

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 25, 2020

skyline75489 added 2 commits August 25, 2020 14:40

Naming

5de7e1a

Oh well

4c5bdf8

skyline75489 commented Aug 25, 2020

View reviewed changes

src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved

Unused

96dbe12

skyline75489 commented Aug 25, 2020

View reviewed changes

src/terminal/adapter/DispatchTypes.hpp Show resolved Hide resolved

Should you blink, sir

2915aff

DHowett mentioned this pull request Aug 25, 2020

"vintage" cursor shape + blinking with an escape sequence? #7382

Open

miniksa approved these changes Aug 25, 2020

View reviewed changes

DHowett reviewed Aug 25, 2020

View reviewed changes

src/terminal/adapter/adaptDispatch.cpp Show resolved Hide resolved

j4james reviewed Aug 25, 2020

View reviewed changes

zadjii-msft reviewed Aug 25, 2020

View reviewed changes

zadjii-msft assigned skyline75489 Aug 25, 2020

zadjii-msft added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Aug 25, 2020

zadjii-msft requested changes Aug 25, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 25, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 25, 2020

skyline75489 added 2 commits August 28, 2020 14:58

Push

e200a32

Fix

3a186ad

zadjii-msft approved these changes Aug 28, 2020

View reviewed changes

zadjii-msft unassigned skyline75489 Aug 28, 2020

j4james reviewed Aug 28, 2020

View reviewed changes

j4james approved these changes Aug 28, 2020

View reviewed changes

skyline75489 commented Aug 29, 2020

View reviewed changes

AdaptDispatch

c4ae1dc

skyline75489 commented Aug 30, 2020

View reviewed changes

Resolve comments

c38f9a3

DHowett reviewed Sep 4, 2020

View reviewed changes

src/terminal/adapter/ut_adapter/adapterTest.cpp Outdated Show resolved Hide resolved

Update src/terminal/adapter/ut_adapter/adapterTest.cpp

88432d3

DHowett approved these changes Sep 4, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 4, 2020

ghost added the Issue-Task It's a feature request, but it doesn't really need a major design. label Sep 4, 2020

ghost merged commit 7ab4d45 into microsoft:master Sep 4, 2020

skyline75489 deleted the chesterliu/dev/decscusr-0 branch September 22, 2020 10:31

ghost mentioned this pull request Sep 22, 2020

Consider supporting [extension to VT,] DECSCUSR "0" to restore cursor to user default to help vim/others #1604

Closed

KalleOlaviNiemitalo mentioned this pull request Oct 30, 2020

Document Cursor Shape Escape Sequences MicrosoftDocs/Console-Docs#140

Closed

moetayuko mentioned this pull request Jan 27, 2021

feat: Support blinking cursor via DECSCUSR sequence wincent/terminus#49

Open

kladd mentioned this pull request Nov 7, 2023

Use terminfo to reset terminal cursor style helix-editor/helix#8591

Merged

notpeelz mentioned this pull request Apr 25, 2024

DECSCUSR 0 should reset to the user-defined cursorStyle microsoft/vscode#211394

Open

This pull request was closed.

Conversation

skyline75489 commented Aug 24, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyline75489 commented Aug 24, 2020

Uh oh!

skyline75489 commented Aug 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j4james Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

skyline75489 commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Aug 26, 2020

Uh oh!

skyline75489 commented Aug 26, 2020

Uh oh!

DHowett commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j4james left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 4, 2020

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 (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Sep 22, 2020

Uh oh!

younger-1 commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyline75489 commented Aug 24, 2020 •

edited by DHowett

Loading

j4james Aug 25, 2020 •

edited

Loading

skyline75489 commented Aug 25, 2020 •

edited

Loading

DHowett commented Aug 26, 2020 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

younger-1 commented Feb 15, 2022 •

edited

Loading