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

Add ligature support to terminal take 3 #157008

Closed
wants to merge 17 commits into from
Closed

Add ligature support to terminal take 3 #157008

wants to merge 17 commits into from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Aug 3, 2022

Fixes #34103

Supersedes #139537

This adds an experimental ligatures setting, this will get us self hosting on the ligatures addon without any additional work and we can then stabilize it over time. It mostly works currently, known issues are:

This adds some node modules so we'll need to make sure that's all good as well.

@Tyriar Tyriar added this to the August 2022 milestone Aug 3, 2022
@Tyriar Tyriar self-assigned this Aug 3, 2022
@Tyriar Tyriar changed the title Tyriar/ligatures 3 Add ligature support to terminal take 3 Aug 3, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2022

Might be overlapping ranges here?

Recording 2022-08-03 at 07 25 40

xtermjs/xterm.js#3998

@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2022

Also (xtermjs/xterm.js#3303) still happens when toggling off and on:

image

@Tyriar Tyriar requested review from deepak1556 and meganrogge August 3, 2022 15:40
@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2022

@deepak1556 can you please review the permissions related parts in src/main.js, vs/codeandvs/platform/protocol`?

@Tyriar Tyriar marked this pull request as ready for review August 3, 2022 15:52
Pointing at distro tyriar/34103_distro_ligatures
meganrogge
meganrogge previously approved these changes Aug 3, 2022
meganrogge
meganrogge previously approved these changes Aug 3, 2022
Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

just two questions

@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2022

The long ===== is actually expected:

image
image

Here it is in Windows Terminal:

image

The width is correct there though.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2022

We ceil the cell width, that's my guess of why the width thing doesn't happen. In order to workaround that we would need sub-pixel offset rendering and then we would hit more glyph overlap problems.

deepak1556
deepak1556 previously approved these changes Aug 4, 2022
src/main.js Show resolved Hide resolved
@Tyriar Tyriar dismissed stale reviews from deepak1556 and meganrogge via 8690bdb August 4, 2022 12:15
@Tyriar
Copy link
Member Author

Tyriar commented Aug 5, 2022

Unfortunately this is getting put on hold again this iteration due to the packaging issues I've been having. They're related to the font-ligatures package using node APIs and the way webpack is bundling it doesn't work inside sandboxed electron renderers. Here's the webpack config if anyone want to investigate https://github.com/xtermjs/xterm.js/blob/master/addons/xterm-addon-ligatures/webpack.config.js

@Tyriar Tyriar modified the milestones: August 2022, September 2022 Aug 5, 2022
@lhecker
Copy link
Member

lhecker commented Aug 22, 2022

We ceil the cell width, that's my guess of why the width thing doesn't happen. In order to workaround that we would need sub-pixel offset rendering and then we would hit more glyph overlap problems.

@Tyriar Hey I just came across this PR and saw your long-ligature problem (long ==== ligatures seemingly shrink shorter and shorter). I'm not sure whether you already solved it in this PR, but I just so happen to have worked on this exact problem just a few weeks ago when I added ligature support for the new Windows Terminal text renderer. If you have any questions or would like to collaborate on this in any way, please let me know. 🙂
This is the PR that fixes it: microsoft/terminal#13549 and this part is the relevant addition (for Direct2D however): https://github.com/microsoft/terminal/blob/dfa92559611f5c982f78cba73d123638a5e54227/src/renderer/atlas/AtlasEngine.r.cpp#L352-L365
advanceScale is simply ceil(glyphAdvance) / glyphAdvance - it's basically the approach that you already thought of and it seems to work pretty great so far.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 22, 2022

@lhecker thanks for the info! Good to know you hit the same problem and upscale

@Tyriar Tyriar modified the milestones: September 2022, On Deck Aug 30, 2022
@andrewgazelka
Copy link

Any updates?

@junaga
Copy link

junaga commented Feb 9, 2023

oh my god it's happening

@canopus1io
Copy link

what's the current status on this?

@Why-not-now
Copy link

I hope it passes asap

@junaga
Copy link

junaga commented Jul 16, 2023

We got ChatGPT, and NVIDIA moved the Nasdaq by 2% on it's own, while this PR was open.

@Why-not-now
Copy link

Let's hope it goes through this time 🙏

@andrewgazelka
Copy link

Let's hope it goes through this time 🙏

It won't. There are still conflicts.

@Why-not-now
Copy link

Let's hope it goes through this time 🙏

It won't. There are still conflicts.

Rip, glad it is getting attention again though.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 4, 2023

Sorry, but closing to clean up PRs as this is stale and would need a new PR when we retry this.

@Tyriar Tyriar closed this Aug 4, 2023
@Tyriar Tyriar removed this from the On Deck milestone Aug 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2023
@Tyriar Tyriar deleted the tyriar/ligatures_3 branch December 19, 2024 14:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ligatures in terminal
8 participants