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

DECSCUSR() and DECSCUSR(0) should reflect the current user setting of the cursor shape #3293

Closed
akinomyoga opened this issue Apr 5, 2021 · 9 comments · Fixed by #5265
Closed
Assignees
Labels
Milestone

Comments

@akinomyoga
Copy link

akinomyoga commented Apr 5, 2021

This was first reported at akinomyoga/ble.sh#95. In Visual Studio Code (VSCode), when a Bash plugin ble.sh is loaded in a Bash session, the cursor shape becomes always Block regardless of the user setting of the cursor shape.

This seems to be caused by the behavior of xterm.js for the control function DECSCUSR() (empty) and DECSCUSR(0) (0). I expect these parameters, empty and 0, change the cursor shape to the default one configured by the user, but, in xterm.js, empty and 0 are always treated as 1 (Blinking Block) which is just the default value for the user setting.

Behavior of other terminals

  • In all the other terminals that I tested (including Mintty, Alacritty, RLogin, GNOME Terminal (VTE), and Windows Terminal), when the user changed the default cursor shape from Block to another, DECSCUSR() and DECSCUSR(0) change the cursor shape to the user-configured default rather than to Block. The behavior can be checked with the following commands after the user default is changed.

    $ printf '\e[ q'
    $ printf '\e[0 q'
    $ printf '\e[1 q'

    I couldn't check xterm's behavior because I don't know how to change the default value for the cursor shape in xterm.

  • We can also find a related discussion in a code comment of VTE:

    We treat 0 and 1 differently, assuming that the VT510 does so too.

    See, according to the "VT510 Video Terminal Programmer Information", from vt100.net, paragraph "2.5.7 Cursor Display", there was a menu item in the "Terminal Set-Up" to set the cursor's style. It looks like that defaulted to blinking block. So it makes sense for 0 to mean "set cursor style to default (set by Set-Up)" and 1 to mean "set cursor style to blinking block", since that default need not be blinking block. Access to a VT510 is needed to test this theory, but it seems plausible. And, anyhow, we can even decide we know better than the VT510 designers!

  • In the original DEC implementation of DECSCUSR, the parameters empty and 0 meant the block style, but I guess it is just because the default cursor shape of VT series couldn't be configured so was always the block style (but maybe VT allowed some user configuration, see the previous item of the VTE comment). Anyway, empty and 0 typically meant the default for some other control functions, so I believe it is a natural guess that empty and 0 for DECSCUSR are also intended for the default cursor shape.

Note on ZDM

Here's a related note on the mode ZDM from @jerch:

Hmm good question. Maybe first some background on this on a general note - in the early days there was a so called ZDM* (zero default mode), which would tell the sequence parser that any functionXY() is equivalent to functionXY(0), meant as default value. This is what xterm.js' parser does atm. It works so far for all implemented DEC related features.

Later on that mode was removed from ECMA-48 with the note, that a parameter of 0 does not not mean default value anymore, but should stand for a numercial value 0 instead. Most sequence functions never applied that updated meaning for older models (e.g. cursor movement sequences still follow the default meaning as in CSI A == CSI 0 A == CSI 1 A). But without ZDM a missing parameter should mean default value, while 0 get its zero meaning (thus CSI A == CSI 1 A != CSI 0 A).

[...]

[*] As far as I know ZDM was completely removed from VTE.

Fix

From the same comment by @jerch,

For the sequence at hand I see a simple fix (since we have a default cursor style in the terminal options I wasnt aware of):

  • DECSCUSR() - switch to default value from settings
  • DECSCUSR(0) - switch to default value from settings
  • DECSCUSR(1) - steady block

This would be still in line with ZDM, but correctly map to the overwritten default in settings instead.

This is what other terminals do and I completely agree with this fix (though I think "steady block" is a typo of "blink block").

@jerch
Copy link
Member

jerch commented Apr 6, 2021

This is what other terminals do and I completely agree with this fix (though I think "steady block" is a typo of "blink block").

Woops, this is indeed a typo in the docs 👍

@jerch
Copy link
Member

jerch commented Apr 6, 2021

@akinomyoga Just looked at the code - we have indeed no fixed default setting in the options for the cursor style, instead the setting gets used as initial value, but later changes overwrite that setting (with hardcoded 0|1 as BLINKING BLOCK).

@Tyriar To get rid of the hardcoded default I suggest to add the cursor style to DEC private modes in CoreService, this way we can keep the option setting untouched and use it as default on DECSCUSR() or DECSCUSR(0) and reset.

@kevinc16
Copy link

kevinc16 commented May 5, 2024

Hey @jerch, I came from microsoft/vscode#211394 - I've looked into this a bit and would like to work on this issue.

If I understand correctly, it seems that we want to add cursorStyle in coreService.decPrivateModes and use that as the terminal cursor, and use optionsService.options.cursorStyle as the default value to revert to? What are your thoughts on adding a defaultCursor variable in optionsService.options instead? Thanks in advance!

@akinomyoga
Copy link
Author

I'm unfamiliar with the codebase of xterm.js, but I don't think the cursor style should be stored in decPrivateModes. By its name and its members, it's obvious that decPrivateModes contains (a part of) the states of DEC Modes, which are set by the control functions DECSET and DECRST. DECSCUSR is unrelated to DECSET/DECRST.

I also think the cursor set by DECSCUSR is not just cursorStyle in the codebase, but both cursorStyle and cursorBlink should be considered here.

export class CoreService extends Disposable implements ICoreService {
public serviceBrand: any;
public isCursorInitialized: boolean = false;
public isCursorHidden: boolean = false;
public modes: IModes;
public decPrivateModes: IDecPrivateModes;

I think it is natural to manage the current values of cursorStyle and cursorBlink along with isCursorHidden above. I guess the optionsService.options should contain the default values of cursorStyle and cursorBlink.

@tisilent
Copy link
Contributor

tisilent commented May 9, 2024

Record the initial cursorStyle in DEC?

@jerch
Copy link
Member

jerch commented Jul 9, 2024

@kevinc16 Sorry for the late response.

Yes you are right - we basically lack a separation of initial default state to current state, which makes it impossible to revert to the initial state since it is not preserved.

@akinomyoga Yepp, decPrivateModes is meant as a stateholder of the currently active/inactive instance modes. This gets initialized during startup from the option's service, if there is a configurable option, otherwise from a hard-coded value.

To get things into perspective, we actually would need both for the full deal:

  • a setting in options for default cursor shapes and whether shape/hide change are applicable at all (as some integrators like to disable those features fully effectively disabling messing with cursor shapes & visibility)
  • a runtime setting reflecting the active shape & hide state, linked to DEC sequences or fully deactivated

@Arup-Chauhan
Copy link

@Tyriar , am coming here from VSCode issue #211394

I am resuming work after a while as I was away.

I am proposing that we can update the InputHandler in Xterm.js to correctly handle DECSCUSR 0 by resetting the cursor to the user's preferred style. This ensures consistency with expected terminal behavior.

Would that be a good step? @Tyriar @jerch inputs will be appreciated.

@Tyriar
Copy link
Member

Tyriar commented Aug 14, 2024

@Arup-Chauhan yep

Arup-Chauhan added a commit to Arup-Chauhan/xterm.js that referenced this issue Aug 25, 2024
@Arup-Chauhan
Copy link

Hi @Tyriar , raised a draft PR to work on this, have some questions, please take a look, thanks!

@Tyriar Tyriar self-assigned this Dec 20, 2024
@Tyriar Tyriar added this to the 6.0.0 milestone Dec 20, 2024
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Dec 20, 2024
@Tyriar Tyriar closed this as completed in b579649 Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants