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

html: Add Unicode 11 xterm.js addon #1310

Merged
merged 2 commits into from
Mar 5, 2024
Merged

html: Add Unicode 11 xterm.js addon #1310

merged 2 commits into from
Mar 5, 2024

Conversation

mikelorant
Copy link
Contributor

Add Unicode 11 addon to xterm.js to enable support for newer Unicode standards. This addon significantly improves rendering of emojis.

@mikelorant
Copy link
Contributor Author

@tsl0922 Will need some advice on how I activated the feature. Not sure I activated it in the preferred location. Feel free to recommend any changes or improvements.

For reference this is the line I am uncertain about:

terminal.unicode.activeVersion = '11';

@tsl0922
Copy link
Owner

tsl0922 commented Mar 5, 2024

For reference this is the line I am uncertain about:

terminal.unicode.activeVersion = '11';

Maybe it can be a Client Option, if we want to extend it to support unicode-graphemes addon later.

@mikelorant
Copy link
Contributor Author

I think once the unicode-graphemes addon becomes stable it would be worth switching over.

The reason I didn't make Unicode 11 an option was I can't think of why you wouldn't want this always enabled. What benefit do we gain from this being an option? Without this being set, most emoji are just rendered incorrectly.

@tsl0922
Copy link
Owner

tsl0922 commented Mar 5, 2024

What benefit do we gain from this being an option?

You can enable / disable this addon, or switch to unicode-graphemes later.

@mikelorant
Copy link
Contributor Author

You can enable / disable this addon, or switch to unicode-graphemes later.

Does this mean we want something that has a value of unicode-11 or unicode-graphemes so you select which support you want?

@mikelorant
Copy link
Contributor Author

mikelorant commented Mar 5, 2024

You actually have me thinking, should we add unicode-graphemes and mark it experimental, while setting the default to unicode11. Can do all the work now and later on just switch the default.

@tsl0922
Copy link
Owner

tsl0922 commented Mar 5, 2024

Does this mean we want something that has a value of unicode-11 or unicode-graphemes so you select which support you want?

The value should be valid for term.unicode.activeVersion, for example: 11 / 15-graphemes (6 if both disabled)

Ref: https://github.com/xtermjs/xterm.js/blob/1da0db435f93879a60b9badbb663852ff3223063/demo/client.ts#L659-L679

should we add unicode-graphemes and mark it experimental

No, unicode-graphemes is not published to npm yet.

@mikelorant
Copy link
Contributor Author

So let me clarify this before I get started:

  1. We add a new client side variable: unicodeVersion.
  2. If unset default is 11.
  3. Can also be set to 6 or 15-graphemes.
  4. If set to 11 we enable the unicode11 addon.
  5. If set to 15-graphemes we fallback to default 11 and enable unicode11 addon.
  6. If set to 6 we do nothing (not enable the unicode11 addon).

This is slightly different to your earlier post but I think it makes more sense to have Unicode 11 as the default. Unicode 6 is just way too old and renders very differently.

Happy to adjust this logic though based on your feedback.

@tsl0922
Copy link
Owner

tsl0922 commented Mar 5, 2024

  1. We add a new client side variable: unicodeVersion.
  2. If unset or unknown value, use the default 11.
  3. If set to 6 we do nothing (not enable any unicode addon).

We don't need to handle values for unicode-graphemes before it is included in ttyd.

@mikelorant mikelorant requested a review from tsl0922 March 5, 2024 04:43
Add Unicode 11 addon to xterm.js to enable support for newer Unicode
standards. This addon significantly improves rendering of emojis.

Signed-off-by: Michael Lorant <michael.lorant@nine.com.au>
@mikelorant mikelorant requested a review from tsl0922 March 5, 2024 05:37
@tsl0922 tsl0922 merged commit 61a985e into tsl0922:main Mar 5, 2024
12 checks passed
@mikelorant mikelorant deleted the feat/add-unicode11-addon branch March 5, 2024 07:20
@mikelorant
Copy link
Contributor Author

Thanks for merging this! 🤞 when this is released there are no issues.

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.

None yet

2 participants