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

putty-style ED2 sequence handling as terminal option #5224

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Blashaq
Copy link

@Blashaq Blashaq commented Nov 21, 2024

This PR is related to the problem discussed in #1727.

goals:

  • Allow xterm.js based applications to change CSI 2 J sequence behaviour to non-canon, which is sometimes preferred by users.
  • Keep canon handling of CSI 2 J (clear whole screen) as default.
  • Alternative handling will push erased text to the scrollback and trim empty lines. This is the way PuTTY and some other terminals do it by default.

changes made:

  • New configuration option: scrollOnDisplayErase in ITerminalOptions.
  • InputHandler.eraseInDisplay will behave differently if this option is set.
  • Added some tests.

Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

Thx for looking into this.

Beside the minor code suggestion below - LGTM 👍

src/common/InputHandler.ts Outdated Show resolved Hide resolved
Co-authored-by: jerch <jerch@rockborn.de>
@Blashaq
Copy link
Author

Blashaq commented Nov 21, 2024

Thanks for suggestions, this is indeed cleaner (and also works, to no suprise :octocat: ).

@jerch
Copy link
Member

jerch commented Nov 22, 2024

@Blashaq The CI tests dont run yet, seems scrollOnDisplayErase is missing in src/common/services/Services.ts in ITerminalOptions?

@Blashaq
Copy link
Author

Blashaq commented Nov 23, 2024

You're right, sorry. I have accidentally skipped that file in my commit.

@jerch
Copy link
Member

jerch commented Nov 23, 2024

There are still linter some issues.

@Blashaq
Copy link
Author

Blashaq commented Nov 23, 2024

Everything should be fine now 🙏

Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

@Blashaq LGTM 👍, works like a charm, many thx for the PR.

@Tyriar There seems to be an issue with clear under with linux with ED3 - the new scrollbar gets not updated by ED3 yet. Maybe this not enough to announce the changes:

this._onScroll.fire(0);

Imho its not linked to this PR, thus I create a new ticket. --> #5228

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