xtermjs / xterm.js

A terminal for the web
https://xtermjs.org/
MIT License
17.56k stars 1.62k forks source link

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

Open akinomyoga opened 3 years ago

akinomyoga commented 3 years ago

This was first reported at https://github.com/akinomyoga/ble.sh/issues/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

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 commented 3 years ago

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 :+1:

jerch commented 3 years ago

@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 commented 5 months ago

Hey @jerch, I came from https://github.com/microsoft/vscode/issues/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 commented 5 months ago

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.

https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/common/services/CoreService.ts#L26-L33

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 commented 5 months ago

Record the initial cursorStyle in DEC?

jerch commented 3 months ago

@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:

Arup-Chauhan commented 1 month ago

@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 commented 1 month ago

@Arup-Chauhan yep

Arup-Chauhan commented 1 month ago

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