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 font_feature_settings #2248

Merged
merged 5 commits into from
Jan 7, 2020
Merged

Add font_feature_settings #2248

merged 5 commits into from
Jan 7, 2020

Conversation

ctrlcctrlv
Copy link
Contributor

@ctrlcctrlv ctrlcctrlv commented Jan 4, 2020

Close #2247

This doesn't look like very much code; but as they say, reading code is much harder than writing code, and I had to really understand how Kitty works to write this, so it took a long time. 😄

2020-01-04-141156_2960x294_scrot
2020-01-04-140802_2960x294_scrot
2020-01-04-141025_2960x294_scrot
2020-01-04-140923_2960x294_scrot

@ctrlcctrlv ctrlcctrlv force-pushed the master branch 2 times, most recently from 457794e to 1db613e Compare January 4, 2020 06:35
It was from an earlier stage of development and is not needed
@kovidgoyal
Copy link
Owner

So before I review the actual code, a couple of comments about the design of the option:

  1. disable_ligatures should not be disabled by this setting. This is because it can be used to turn off ligatures only under the cursor as well, a feature I would be loth to loose. To maintain backward compatibility, I think disable ligatures should remain and simply add -calt to the features as per its value + cursor position.

  2. kitty runs on macOS as well where fontconfig is not used, so it would probably be best to modify kitty + list-fonts to have a --postscript flag that will cause it to output postscript names instead.

@ctrlcctrlv
Copy link
Contributor Author

Backwards compatibility is not threatened by this patch because if it's unset, disable_ligatures still works. It only works differently if set.

However, perhaps it is better to make it so that it is also possible to specify a set of feature tags on hover as well?

The reason I don't want to "simply add -calt" is because every font works differently. Fira Code uses calt, but some other fonts use liga

@ctrlcctrlv
Copy link
Contributor Author

For complaint №2, how about I just add the information to kitty --debug-font-fallback?

@ctrlcctrlv
Copy link
Contributor Author

ctrlcctrlv commented Jan 4, 2020

(Sorry for the triple post, more thoughts keep coming to mind.)

@kovidgoyal What do you think about this syntax?

font_feature_settings FiraCode-Retina: +onum
font_feature_settings FiraCode-Retina:hover: +onum -liga -calt

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2020 via email

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2020 via email

@ctrlcctrlv
Copy link
Contributor Author

If you like you can have disable ligatures optionally take feature names, and when no feature names are specified, defaults to CALT.

I do like, but are you sure you don't want it to be font-keyed?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2020 via email

@ctrlcctrlv
Copy link
Contributor Author

Understood, I will do it. No need to review before then. Thanks Kovid

(Also, wow, you wrote Calibre too? That program uses Python extensions better than any software I've ever had the pleasure of working on. I tried to convince another FontForge developer of its superiority, but I haven't quite managed to do so yet. If I had it my way we'd copy how you did it.)

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 4, 2020 via email

@ctrlcctrlv
Copy link
Contributor Author

@kovidgoyal I've pushed a change for your review; it makes disable_ligatures into a global flag which affects all fonts.

Just so you know, I have decided to shelve the hover syntax for now. I couldn't think of a case where it's actually useful, and to implement it would mean more work than I have time for right now for something I won't use myself. If someone has a real use idea for it, and I like it, I might open a new PR for it in future though.

I hope that's okay with you.

@Diaoul
Copy link
Contributor

Diaoul commented Dec 15, 2020

Would that be possible to default reading font_features from fontconfig on linux when available? It seems to be what it is done to read the font_family no?
Let me know if this is something you would consider.

@kovidgoyal
Copy link
Owner

Sure, dont think I am interestedin writing the code for it, but a PR is
welcome.

@ctrlcctrlv
Copy link
Contributor Author

Hi @Diaoul. I don't understand what you mean. I might be interested if you explain. Are you saying that there is a way to instruct Fontconfig as to what features should be enabled whenever a font is used?

@kovidgoyal
Copy link
Owner

@ctrlcctrlv
Copy link
Contributor Author

Glad to see fontconfig support has been done, now using it myself :-) Thanks @Diaoul! Always nice when I implement a PR then my PR is improved. :-D

note 0 glyph:
image

<!-- $ cat ~/.config/fontconfig/conf.d/99-fira-code-fontfeatures.conf -->
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
  <description>Enable select opentype features for FiraGO.</description>

  <!--
	 Typographic features are provided on a per-typeface basis.
	 Tables with all available features are available here:
	 https://en.wikipedia.org/wiki/OpenType_feature_tag_list 

      Also read the fontconfig user spec:
      https://www.freedesktop.org/software/fontconfig/fontconfig-user.html
  -->

  <match target="font">
    <test name="family" compare="eq" ignore-blanks="true">
      <string>Fira Code</string>
    </test>
    <edit name="fontfeatures" mode="append">
      <string>tnum on</string> <!-- tabular numbers -->
      <string>zero on</string> <!-- slashed zero -->
    </edit>
  </match>
</fontconfig>

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.

Disabling ligatures disables other OpenType features (which are enabled by default) too
4 participants