Skip to content

putty-style ED2 sequence handling as terminal option#5224

Merged
Tyriar merged 9 commits intoxtermjs:masterfrom
Blashaq:master
Apr 21, 2025
Merged

putty-style ED2 sequence handling as terminal option#5224
Tyriar merged 9 commits intoxtermjs:masterfrom
Blashaq:master

Conversation

@Blashaq
Copy link
Copy Markdown
Contributor

@Blashaq Blashaq commented Nov 21, 2024 •
edited by Tyriar
Loading

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.

Fixes #1727

jerch
jerch reviewed Nov 21, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jerch jerch left a comment •
edited
Loading

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 👍

@Blashaq
Copy link
Copy Markdown
Contributor Author

Blashaq commented Nov 21, 2024

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

@jerch
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Blashaq commented Nov 23, 2024

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

@jerch
Copy link
Copy Markdown
Member

jerch commented Nov 23, 2024

There are still linter some issues.

@Blashaq
Copy link
Copy Markdown
Contributor Author

Blashaq commented Nov 23, 2024

Everything should be fine now 🙏

jerch
jerch approved these changes Nov 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jerch jerch left a comment •
edited
Loading

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:

xterm.js/src/common/InputHandler.ts

Line 1238 in 41e8ae3

this._onScroll.fire(0);

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

Tyriar
Tyriar approved these changes Apr 21, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

LGTM 👏 sorry about the delay

@Tyriar Tyriar added this to the 6.0.0 milestone Apr 21, 2025
@Tyriar Tyriar self-assigned this Apr 21, 2025
@Tyriar Tyriar enabled auto-merge April 21, 2025 13:39
This was referenced Apr 21, 2025
Closed
Closed
@Tyriar Tyriar merged commit 1771825 into xtermjs:master Apr 21, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tyriar Tyriar Tyriar approved these changes

+1 more reviewer

@jerch jerch jerch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Atypical "Erase In Display" behaviour

3 participants

@Blashaq @jerch @Tyriar