feat: Add ligatures support

[aminya]

This inherits the font setting from the editor.

Now the terminal is the same as my editor:

[UziTech]

I'm not sure we should change this. I don't think most people would want the terminal to use the same font as the editor and if they do they can already change it. I think monospace is a good default.

[aminya]

@UziTech No it is the other way around. That is why in Atom we import the variables in Less and we use @font-family and var(--editor-font-family) everywhere. It is because the Atom wants all the plugins to use what the user defines in their font-family. There should be only one single point of font settings, and plugins should not introduce a new font configuration. Otherwise, every single plugin should define its font configuration (which is not the case).

Not doing this is also a bug. Monospace does not support ligatures and Pipelines which results in a very bad default configuration. See the missing fonts.

[the-j0k3r]

Not doing this is also a bug. Monospace does not support ligatures and Pipelines which results in a very bad default configuration. See the missing fonts.

I agree theres an issue with font ligature support, the problem you forgot is that Cascadia font types arent available as a default/preinstalled font on most OS's.

Until fonts with ligatures support are shipped in Windows/macOS/Linux this will always be an issue, I dont think its the x-terminal responsibility to ship these fonts, but the OS's responsibility.

To go along with the PR we would need to ship and install cascadia font types to suit most users setups not just one.

Also I disagree that we should enforce ANY font by default, it should always be inherited or with monospace fallback, then if a user want ligatures, then its up to user.

[aminya]

To go along with the PR we would need to ship and install cascadia font types to suit most users setups not just one.

If you like we can do that! The installation is very easy.

Also I disagree that we should enforce ANY font by default, it should always be inherited or with monospace fallback, then if a user want ligatures, then its up to user.

Yes, that is why I made this pull request. You should not hardcode monospace. It should be inherited from what the user has set in their Atom settings. I do not want to go through every single plugin and change their font settings individually.

This code actually means inheritance from what the default Atom setting:

atom.config.get('editor.fontFamily').split(',')[0] 

[the-j0k3r]

Yes, that is why I made this pull request. You should not hardcode monospace

We shouldnt hardcode cascadia either

If you like we can do that! The installation is very easy.

I clearly said that this isnt an option IMO

Until fonts with ligatures support are shipped in Windows/macOS/Linux this will always be an issue, I dont think its the x-terminal responsibility to ship these fonts, but the OS's responsibility.

@UziTech thoughts?

[aminya]

Yes, that is why I made this pull request. You should not hardcode monospace

We shouldnt hardcode cascadia either

We did not. That one was just a test for running the spec. 😄

[the-j0k3r]

We did not. That one was just a test for running the spec. 😄

You're now using Menlo in tests which is also not a common available font by default in all 3 supported OS's either.

Defining monospace will use the first monospaced font available in OS so if you have Menlo it will use that and so on and so on.. Im not sure it matters in tests but best to leave as it was.

[aminya]

Menlo is used by default in Atom. The tests were failing that is why I changed it to Menlo.

I will revert it but the tests will fail.

[the-j0k3r]

I will revert it but the tests will fail.

Before the PR tests worked or not?

Menlo is used by default in Atom. The tests were failing that is why I changed it to Menlo.

Depends on OS, Menlo was a macOS font IIRC

[aminya]

I will revert it but the tests will fail.

Before the PR tests worked or not?

That was because you were hardcoding monospace as the font. I don't want us to hardcode a font. If you don't hardcode, then Menlo, which is Atom's default font, is chosen.

[the-j0k3r]

That was because you were hardcoding monospace as the font. I don't want us to hardcode a font. If you don't hardcode, then Menlo, which is Atom's default font, is chosen.

Again the default font chosen depends on OS, Windows will not ship Menlo at all, youseem to be designing the PR assuming fonts exist when they dont.

FYI the correct is to define Menlo, Consolas, DejaVu Sans Mono, monospace If Menlo is not available it will select Consolas, if thats not available it uses DejaVu Sans Mono, if thats not available it uses the first monospaced font it finds

monospace isnt an actual font its a catch all for the monospace font familly

Also related https://github.com/atom/atom/search?q=--editor-font-family&unscoped_q=--editor-font-family from atom/atom#16731

[aminya]

I did not assume Menlo comes by default 😄 There is some miscommunication here.

This PR is about: "Do not override a font in a package when Atom already chooses a font". This font may be whatever the user chooses, or whatever Atom decides.

I just set Menlo for the sake of the tests.
https://github.com/bus-stop/x-terminal/pull/126/checks?check_run_id=914455011#step:7:13

Menlo is the first choice by Atom, and in the CI, it comes preinstalled, so the tests will pass.
https://github.com/atom/atom/blob/4feca894e1feb21d6caa33d5e089190d8dac19ed/src/config-schema.js#L433

[UziTech]

Menlo is the first choice by Atom

Won't atom.config.get('editor.fontFamily').split(',')[0] always return Menlo if the user doesn't change the editor.fontFamily setting? And if they don't have that font on their system it will fail over to serif?

[aminya]

I fixed this in c6110e4. Now it uses the whole font family.

[UziTech]

It might be better to add a "Use Atom Font Family Setting" checkbox setting which is off by default since I still believe that most people don't want the same font in the editor as in the terminal.

[aminya]

I added the configuration.

It might be better to add a "Use Atom Font Family Setting" checkbox setting which is off by default since I still believe that most people don't want the same font in the editor as in the terminal.

No, it is the other way around. No Atom plugins ever change the fonts in their plugin. Everyone assumes that the font settings they have set in the settings are used everywhere unless they explicitly request it. Not doing this makes the Atom ecosystem complex and hard to use for the users. They should go through each plugin and change the fonts.

[UziTech]

Should we just always have ligatures enabled, since it won't do anything unless the font supports ligatures, and add a note to webgl about ligatures not working if webgl is enabled?

[the-j0k3r]

Should we just always have ligatures enabled, since it won't do anything unless the font supports ligatures, and add a note to webgl about ligatures not working if webgl is enabled?

Isn't it better to conditionally disable ligatures the actual checkbox and functionality whenever webgl is enabled. Seems more intuitive

[UziTech]

Isn't it better to conditionally disable ligatures the actual checkbox and functionality whenever webgl is enabled.

How do we disable the check box in the settings view?

[UziTech]

Seems more intuitive

I feel like it would be most intuitive if the user sets the font to one that supports ligatures they just work without needing to also enable the addon.

[aminya]

I didn't follow the discussion much. But I don't have any issue with ligatures that come with a font.

[UziTech]

@aminya ligatures work for you in x-terminal without enabling the addon? What font are you using and what version of Atom?

[aminya]

Cascadia Code: https://github.com/microsoft/cascadia-code#installation
The version of Atom is not important, but I think 1.51 currently 🤔

[UziTech]

It was the version of electron I was looking for. Cascadia Code doesn't show ligatures in x-terminal for me unless ligatures addon is enabled and the webgl addon is disabled. I tried using atom v1.51.0 and v1.52.0-beta0.

[aminya]

Oh. Yeah. You are right. It only works inside the git status showers oh-my-posh, not in the text itself.
I have WebGL enabled. I didn't touch addons.

[the-j0k3r]

I feel like it would be most intuitive if the user sets the font to one that supports ligatures they just work without needing to also enable the addon.

The we would need to ship the necessary fonts in such a case. Cascadia Code and Fira Code would be ideal as they are maintained and make semi-regular releases.

Both Atom and x-terminal are set t use Cascadia Code on my end however I dont have any powerline prompts as I think Windows + git bash requires a modded git bash https://github.com/sschleemilch/gitbash-powerline but thats outdated and Ive never tried it. Ide prefer to use the default git bash

[aminya]

I want to include a better font as the default font in atom-community build. I like juliamono among all. It is a mono like font (unlike Cascadia which is aligned a little strange). It has a huge number of ligatures.

[UziTech]

we would need to ship the necessary fonts in such a case.

We wouldn't need to ship anything. If the ligatures addon is not enabled then no fonts use ligatures (even ones like Fira Code that support ligatures). If the ligatures addon is enabled then only fonts that support ligatures (e.g. Fira Code) show ligatures.

Therefore I think we should always enable the ligatures addon and add a note to the webgl setting that ligatures might not work when webgl is enabled. If a user wants ligatures in x-terminal all they would have to do is set the font to a font that supports ligatures. If the user does not want ligatures they would set the font to on that does not support ligatures. No need for a checkbox to enable or disable them.

[the-j0k3r]

Therefore I think we should always enable the ligatures addon and add a note to the webgl setting that ligatures might not work when webgl is enabled.

I disagree, as per my previous comments.

We shouldnt let something break in these condiitions, and according to you thats what we are going to do, adding more words is not a user friendly substitution.

[UziTech]

We shouldnt let something break in these condiitions

What is breaking?

[the-j0k3r]

What is breaking?

According to you

I think we should always enable the ligatures addon and add a note to the webgl setting that ligatures might not work when webgl is enabled

Emphasys on the bolded part, also might is not a definitive, it eiter works or doesnt, and since webgl hasnt had support for ligatures (according to your earlier post) then it wont work and thus breaks.

We shouldnt allow this.

[UziTech]

We shouldn't allow this.

How can we prevent this? How can we disable a checkbox in the settings view? Answer: we can't.

Therefore all we can do is inform people that if they use a font that supports ligatures and enable webgl then ligatures won't work.

Either way having a checkbox to enable/disable ligatures is not useful. Since the ligatures addon is essentially a no-op when webgl is enabled, it doesn't help to have a checkbox to enable/disable it.

[UziTech]

As I've said before the ligatures addon does not break the webgl addon so there is no point in disabling it ever. The webgl addon does break the ligatures addon so we should (and do) have a way for the user to enable/disable it if they want, or don't care about, ligatures.

[UziTech]

also might is not a definitive, it eiter works or doesnt

I used might because it most likely will work in the future, so if we forget to remove that note when it does start working then it might work.

[the-j0k3r]

s I've said before the ligatures addon does not break the webgl addon

lol, seriously, the webgl does break the ligatures addon. Im pretty sure we already talked about this.

I used might because it most likely will work in the future, so if we forget to remove that note when it does start working then it might work.

I know that it doesnt work now in present, which is what matters

[UziTech]

lol, seriously, the webgl does break the ligatures addon. Im pretty sure we already talked about this.

I didn't say webgl doesn't break the ligature addon. I said the ligature addon doesn't break the webgl addon. There is a difference.

If you read the whole thing I agree that the webgl addon breaks the ligature addon which is why we need a checkbox for the webgl addon not the ligatures addon.

[UziTech]

Let me try to say this as explicitly as I can:

If the user enables the webgl addon then x-terminal works the same whether the ligatures addon is enabled or disabled. Therefore there is no point in disabling the ligatures addon when the webgl addon is enabled.

[the-j0k3r]

So now, ligatures addon is permanently enabled and if a user enables webgl the ligatures support will no longer work. is this correct?

[UziTech]

Yes. There is currently no way to support ligatures with the webgl addon enabled. Ligatures being always enabled doesn't hurt anything.

Essentially if a user has webgl enabled then everything will work the same as it currently does without this PR. If the user does not have webgl enabled then ligatures will work provided they have the font setting set to a font that supports ligatures.

[UziTech]

Furthermore once webgl does support ligatures then they will work with webgl enabled without us needing to change any code. (Other than updating the webgl addon)

[UziTech]

I'm going to merge this so we can get ligature support in x-terminal. If we want to add a checkbox for the addon we can do it in another PR.

[github-actions]

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

• GitHub release
• Atom package

Your semantic-release bot 📦🚀