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

Do not render ligatures under cursor (optionally?) #461

Closed
maximbaz opened this issue Apr 15, 2018 · 37 comments
Closed

Do not render ligatures under cursor (optionally?) #461

maximbaz opened this issue Apr 15, 2018 · 37 comments

Comments

@maximbaz
Copy link
Contributor

maximbaz commented Apr 15, 2018

I'm playing with font ligatures using Fira Code, it is fun to see them rendered and they convey a meaning quite clearly, however it is sometimes confusing to actually edit the rendered ligature.

For example, looking at this, the meaning is clear:

image

However, what's not immediately obvious is what will happen if I press Del or Backspace in this case:

image

In fact, in this particular case the code is:

if (a !== b) {

What I suggest to do is to not render a ligature if a cursor is currently over it, just render !==.
Then it is very obvious what Del or Backspace will do 🙂

I guess it should be relatively easy to achieve, because a ligature always takes the same number of cells as all of the the underlying "raw" characters.

I have seen people express this opinion for font creators (e.g. be5invis/Iosevka#257), where they suggest to edit the font and choose a better design. However, I think rendering "raw" characters instead of a ligature when the cursor is above the glyph is a more generic and font-agnostic solution.

I'm not sure if this requires a configuration option or should become a new behavior for everyone.


UPDATE: looks like I'm not the first one who thought of this (hehe), and Qterminal and Konsole already have such behavior:

decomposition

@kovidgoyal
Copy link
Owner

Patches are welcome

@egmontkob
Copy link

This is "accidental" in Konsole, as it uses Qt's (or KDE's) text rendering methods for runs of identical attributes (and also stops at the cursor). See e.g. Konsole issues 361547 and 379535 for some troubles this approach causes, and it even goes worse when combined with RTL/BiDi.

(Makes me wonder what the best behavior is if cells to be ligatured don't share the exact same attributes. Should a ligature be avoided in all these cases? Kitty seems to avoid a ligature if a non-color attribute (e.g. boldness) changes, but manages to use varying colors for the ligature's fragments. E.g. a red "!" followed by yellow "==" appears as red "≡" followed by yellow "≢≡", which may hide some important semantics (e.g. the intent of the differently colored "!" in this example) but definitely looks awesome. Ideally ligature definitions in fonts could define which parts of the ligatured glyph corresponds to which codepoint and then foreground coloring could happen accordingly [background coloring would still remain an issue], but I'd guess it's not possible to define this in fonts.)

Back to the original suggestion: Should the cursor only prevent ligatures across its left edge, or across both edges? Or should it depend on the cursor's shape (i-beam vs. rectangle)?

@maximbaz
Copy link
Contributor Author

Thanks, this is interesting.

The first Konsole's issue 361547 will not be a problem in kitty due to the way how ligatures are implemented, the characters will never move because by design the "ligaturized" text takes exactly the same number of cells as the "raw" text.

ligature_size

Very interesting observation with colors 🙂

Should the cursor only prevent ligatures across its left edge, or across both edges? Or should it depend on the cursor's shape (i-beam vs. rectangle)?

Hm, very good question.

My first thought was that with rectangle and underline cursors I care to see the "raw characters" only when the cursor is directly over some part of the operator. The beam cursor usually indicates an insert mode where I can press Del or Backspace (so I also care about the previous and the next character comparing to my cursor's position). However, the cursor shape is not a true indicator of the "mode", for example I just opened nano, I see rectangle cursor and yet I can press Del and Backspace.

So, in summary, I think regardless of the cursor shape, the ligatures should not be rendered when the cursor is touching the operator with any of its sides.

This gif shows all of the cursor positions where I think the operator should be rendered as >= instead of the ligature:

ligature_place

@kovidgoyal
Copy link
Owner

It depends very much on how fonts do the ligatures.You can have fonts that convert m codepoints to n glyphs where n can be one. So in general it is not possible to detect parts of ligatures corresponding to code-points. kitty does it crudely by simply splitting the ligature into cell blocks and assuming each block corresponds to the codepoint, as you can see with differently colored codepoints. But this is, in general not true, take for example !== becoming ≢ there is no real sense in which some part of that ligature corresponds to ! and the rest to ==.

I'm actually not a fan of the cursor preventing ligatures. After all, I find symbols changing on the screen when moving the cursor distracting, and the cursor moves a lot more than it edits (in editors as opposed to the command line). Not to mention that if this is to be implemented it means you have to re-layout potentially two entire lines every time the cursor moves. That said, I wont refuse a PR to implement an option for this.

@egmontkob
Copy link

I'm not a fan of ligatures in terminal emulators at all :) (VTE bug 762832 comment 1)

@kovidgoyal
Copy link
Owner

Ligatures that span end of line are not a problem in my experience. They are simply rendered as normal characters. I haven't seen a single bug report complaining about that in kitty. This happens automatically if you do cell based rendering. You do have to jump through a lot of hoops to make harfbuzz work with cell based rendering, but once you do that, there are no major issues.

@egmontkob
Copy link

The beam cursor usually indicates an insert mode where I can press Del or Backspace

"insert mode" is a concept of very few apps (maybe vi clones only, plus the vi editing mode of readline/bash, I've heard about folks using the latter). The vast majority of apps don't have this concept. If the text underneath the cursor is editable by its nature, I expect Del and Backspace to edit it in the usual ways; if they don't, I throw that utility out of the window and hope never to have to use it again.

I've been using I-Beam cursor for years, and (apart from vim) it always gives me the expected behavior: Backspace erases to the left, Del erases from the right of the I-Beam cursor. As such, for me (and as you've probably guessed, I'm not using vim and clones) preventing a ligature only across the left edge of the cursor would be more logical. But sure for other folks, in other situations preventing ligatures at both edges of the cursor is the better. I can't see the right answer for this dilemma.

@hendrysadrak
Copy link

hendrysadrak commented Apr 17, 2018

For example how WebStorm does it.

asd: () => {

asd1

if ( a >= b ) {

asd2

@kovidgoyal
Copy link
Owner

I'm confused, I dont see any ligatures changing in your screencaps?

@hendrysadrak
Copy link

@kovidgoyal Yeah, just an example how webstorm is handling this situation. There it doesn't change, shows ligatures as they are. First one is two letters long, second one is one letter long.

@maximbaz
Copy link
Contributor Author

The current behavior in kitty is exactly the same, and this issue describes the imperfections of such behavior. Please read the description 😉

@maximbaz
Copy link
Contributor Author

maximbaz commented Sep 9, 2018

@kovidgoyal could you give some hints on implementing this, where would I need to start looking?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Sep 10, 2018

It's complex. You'd need to get a good grasp of the way text rendering works in kitty. Every cell is represented by two structures, the CPUCell and the GPUCell. The CPUCell contains the text, the GPUCell contains the index into the texture map for the glyph corresponding to the text in the cell. The GPUCell is updated whenever any text on that line changes. You would then need to update it whenever the cursor changes position as well.

Then you would need to convince harfbuzz to render the text without ligatures when the cursor is over it and store that in the texture map separately. and change the indices in the GPUCell to point to it.

The relevant code is mostly in shaders.c and fonts.c

@Luflosi
Copy link
Contributor

Luflosi commented Oct 28, 2018

Do you know how to make harfbuzz render without the ligatures? Would rendering each letter individually work? Is there a better alternative?

@kovidgoyal
Copy link
Owner

Nope sorry, I'm not really a harfbuzz expert, whatever I know about it, I learned on the job, so to speak :)

I vaguely recall that harbuzz has the ability to turn on and off opentype features, maybe turning off hte liga feature will do the trick, but I'm not sure.

@Luflosi
Copy link
Contributor

Luflosi commented Oct 29, 2018

Ok, here is my (not clean) test code in fonts.c that manages to turn off ligatures for FiraCode for anyone interested:

    int nFeatures = 0;
    hb_feature_t* features = NULL;
    hb_tag_t tag;

    tag = hb_tag_from_string("liga", strlen("liga"));
    features = realloc(features, (nFeatures + 1) * sizeof(hb_feature_t));
    features[nFeatures].tag = tag;
    features[nFeatures].start = 0;
    features[nFeatures].end = (unsigned int) -1;
    features[nFeatures].value = 0;
    nFeatures++;

    tag = hb_tag_from_string("rlig", strlen("rlig"));
    features = realloc(features, (nFeatures + 1) * sizeof(hb_feature_t));
    features[nFeatures].tag = tag;
    features[nFeatures].start = 0;
    features[nFeatures].end = (unsigned int) -1;
    features[nFeatures].value = 0;
    nFeatures++;

    tag = hb_tag_from_string("dlig", strlen("dlig"));
    features = realloc(features, (nFeatures + 1) * sizeof(hb_feature_t));
    features[nFeatures].tag = tag;
    features[nFeatures].start = 0;
    features[nFeatures].end = (unsigned int) -1;
    features[nFeatures].value = 0;
    nFeatures++;

    tag = hb_tag_from_string("calt", strlen("calt"));
    features = realloc(features, (nFeatures + 1) * sizeof(hb_feature_t));
    features[nFeatures].tag = tag;
    features[nFeatures].start = 0;
    features[nFeatures].end = (unsigned int) -1;
    features[nFeatures].value = 0;
    nFeatures++;

    hb_shape(font, harfbuzz_buffer, features, nFeatures);

The final code may only want to turn off the calt feature as the other three don't do anything for FiraCode.
I may continue this some other time, in the meantime anyone is free to try to make progress on this.

@Luflosi
Copy link
Contributor

Luflosi commented Mar 17, 2019

@maximbaz did you see my PR yet? Feel free to try it out yourself.

@blueyed
Copy link
Contributor

blueyed commented Mar 20, 2019

Awesome, thanks @Luflosi, and @kovidgoyal, of course!

I will give this a try for a while - I can imagine this to be useful to be changed on-the-fly, i.e. via a shortcut to change the config during runtime.

@Luflosi
Copy link
Contributor

Luflosi commented Mar 20, 2019

Would something like #1292 do what you want or would you like a keyboard shortcut specifically for changing this option? Maybe there are other ways to implement this too, e.g. remote control.
@kovidgoyal what would be the best option if we were to implement something like this?

@kovidgoyal
Copy link
Owner

Let's not jump the gun. Let @blueyed first come back with
whether a way to change it dynamically is useful or not, and if so, in
what use case.

@blueyed
Copy link
Contributor

blueyed commented Mar 20, 2019

Sure, it was just a quick thought.

Some feedback already: with something like "win->w_p_pvw" the "->" ligature is only displayed when after the whole word/expression - I would expect it to be displayed as a ligature already if after the "->w". I.e. with "foo->b_ar" (cursor at "_") it should render the ligature already, since the ligature is only "->", not the whole word(s).

@Luflosi
Copy link
Contributor

Luflosi commented Mar 20, 2019

If I understand what you mean correctly, then this is exactly how it behaves for me:
screenshot
screenshot
Is it different for you?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 20, 2019

@Luflosi if I type ->a i get no ligature, if I type -><space> I do get a ligature, this is with

kitty -o font_family=Fira\ Code -o font_size=16 --config NONE -o disable_ligatures_under_cursor=y sh -c read

@blueyed
Copy link
Contributor

blueyed commented Mar 20, 2019

@Luflosi
That's how I would expect it, but I seem to only get it after whitespace also.
Using kitty -o font_family=Iosevka\ Term -o font_size=16 --config NONE -o disable_ligatures_under_cursor=y sh -c read (Iosevka via be5invis/Iosevka#248 (comment)).

@Luflosi
Copy link
Contributor

Luflosi commented Mar 20, 2019

@kovidgoyal I can reproduce the issue, very interesting. Seems like there is more work to be done to get this working properly. Don't count on me til next week though, I'm a little busy.

@kovidgoyal
Copy link
Owner

@Luflosi you have to break up the run at the transition point when the value of disable_ligature changes.

@kovidgoyal
Copy link
Owner

In fact to make this work robustly, the only way I can see is to RENDER the full line, then check if any ligatures on the line intersect with the cursor position, if they do, then re-render just the cells in that ligature. Which means you will need to work at the level of the Group. For a multi-codepoint group check if it intersects with the cursor and split it up if so. Going to be a fair bit of work.

@kovidgoyal
Copy link
Owner

I have implemented it using glyph groups. Not really tested it a lot. I leave testing/fine-tuning to you.

@Luflosi
Copy link
Contributor

Luflosi commented Mar 28, 2019

Awesome. I'll use it for a while and see if I can find any issues.

@blueyed
Copy link
Contributor

blueyed commented Apr 15, 2019

I've noticed some delays/hangs with writing lately (might have started with the last update of this, but am not sure), where e.g. the shell prompt does not appear, until some redrawing is triggered (e.g. pressing a key, but also moving to another tmux pane).
Have not really investigated (and it is not reproducible then by just doing the same action / output again), but thought that it might be related to this feature.

@kovidgoyal
Copy link
Owner

No, it's more likely due to the refactoring to move the event loop into
GLFW. It's likely that some of the timer/event code is slightly buggy, I
just haven't had the time to investigate it. It's why I am not releasing
0.14 until I have a chance to investigate.

@josswright
Copy link
Contributor

As this code is already written, would it be realistic to extend this to an option to allow disabling ligatures entirely? I'm seeing more and more fonts that are otherwise very nicely designed including them, and they''re often more annoying than useful.

@kovidgoyal
Copy link
Owner

This code is not really suitable for turning off all ligatures. For that, the correct way to do it would be to disable the liga and calt features in harfbuzz at startup, unconditionally.

@kovidgoyal
Copy link
Owner

See 934336e

@josswright
Copy link
Contributor

Amazing response, as always!

Oddly enough, this seems to work as intended for the ligatures I've tried in Hasklig and for some ligatures in Space Mono, but not for others. For reference, maths characters <- and -> are correctly disabled in Space Mono, but the fi and fl ligatures are still enabled (and not rendering correctly).

If there's something I can do to test, please do let me know!

@kovidgoyal
Copy link
Owner

It will be because those ligatures are not using the calt OpenType
feature but some other table for it. IMO a monospace font really has no
business using fi ligatures anyway. The whole point of the fi ligature
was to save space while maintaining legibility, which does not apply to
monospace fonts.

You could probably fix it by disabling more OpenType features, in
addition to calt such as liga and maybe kern.

@josswright
Copy link
Contributor

josswright commented Apr 20, 2019

Looking around it more, it looks that lots of people are having this issue with Space Mono as it's designed as a monospace display font, not a coding font. I did find someone who made a version with ligatures removed Space Mono NL, so that's probably the best solution here rather than fighting a very non-standard font.

Great to have this ligature-disabling option built into kitty, though. Thanks again!

EDIT: As an update, there's a patched version for Vim Powerline that also resolves this, and that's probably a better place to link: https://github.com/powerline/fonts/tree/master/SpaceMono

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants