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

Working on fixing Issue #3293 #5137

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Arup-Chauhan
Copy link

@Arup-Chauhan Arup-Chauhan commented Aug 25, 2024

@Tyriar Hi, I was working on #3293 and for that I have done the following things and created this draft PR to discuss.

Here are some changes I made:

  1. Terminal.ts:

    • I added logic to save the custom cursor style (userCursorStyle) when the terminal initializes. This ensures that the terminal keeps my preferred cursor style even when control sequences are sent.
  2. InputHandler.ts:

    • I adjusted the setCursorStyle function to apply userCursorStyle when handling the reset cursor style sequence (\x1b[0 q). I also included some console log statements to help debug the cursor style changes.

Possible Issue with Control Sequences:

While testing, I noticed something off with how the control sequences are being processed. Specifically, the output logs don't match the expected behavior.

Current Log Outputs:

Script is running
User Cursor Style: underline
DOM fully loaded and parsed
Sending control sequence as given 1
User Cursor Style: underline
Current Style: underline
Sending control sequence as given 2
User Cursor Style: underline
Current cursor style: underline

Expected Log Outputs:

Script is running
User Cursor Style: underline
DOM fully loaded and parsed
Sending control sequence as given 1
User Cursor Style: underline
Current Style: bar
Sending control sequence as given 2
User Cursor Style: underline
Current cursor style: underline

Am I testing / emulating the xterm terminal properly?
The current output suggests that the control sequences may not be applied correctly, particularly when resetting the cursor style. This might need a closer look to figure out what's going on.

Help would be appreciated.

Comment on lines +2737 to +2740
case 0:
console.log('Case 0 triggerd');
this._optionsService.options.cursorStyle = this._optionsService.options.userCursorStyle || 'block';
break;
Copy link
Member

Choose a reason for hiding this comment

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

I think the approach you're taking it likely to cause problems (maybe what's causing the issue you mention now). Instead of trying to replace this._optionsService.options.cursorStyle or add a new this._optionsService.options.userCursorStyle to this._optionsService.options which is an object created by the user and not meant to be touched by us, we would want some way to setting an override which starts out at undefined. Makes sense to put this override in IDecPrivateModes as mentioned in #3293 (comment)

Then when we go to use the actual cursor style from the renderer, we would use devPrivateModes.cursorStyle ?? this._optionsService.options.cursorStyle ?? 'block'

Copy link
Author

Choose a reason for hiding this comment

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

I think the approach you're taking it likely to cause problems (maybe what's causing the issue you mention now). Instead of trying to replace this._optionsService.options.cursorStyle or add a new this._optionsService.options.userCursorStyle to this._optionsService.options which is an object created by the user and not meant to be touched by us, we would want some way to setting an override which starts out at undefined. Makes sense to put this override in IDecPrivateModes as mentioned in #3293 (comment)

Then when we go to use the actual cursor style from the renderer, we would use devPrivateModes.cursorStyle ?? this._optionsService.options.cursorStyle ?? 'block'

That makes sense, I will do this and get back to you, give me a while, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Hi @Tyriar, wanted to update you that I got some academic commitments mid-way so paused work on this.
I am continuing working on this, will keep you updated.

Once again, thanks for your support!

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.

2 participants