linebender / parley

Rich text layout library
Apache License 2.0
204 stars 24 forks source link

Retina weight is loaded instead of regular weight for FiraCode Nerd Font #92

Open fredizzimo opened 1 month ago

fredizzimo commented 1 month ago

When trying to load "FiraCode Nerd Font" using StyleProperty::FontStack(FontStack::Source("FiraCode Nerd Font")) the retina weight (450) is loaded instead of the regular 400 weight.

FiraCodeNerdFont-Regular.ttf has the normal 400 weight, so it should be loaded instead.

NotoSerif Nerd Font has a similar problem, but there it loads the Medium weight instead of Normal

This happens at least on Arch Linux with commit 5b60803d1256ab821f79eaa06721a3112de7202a

fredizzimo commented 1 month ago

On another Linux machine, it's actually loading the correct weight.

I'm not sure what the difference is, both are Arch Linux install and use the same version of the FiraCode NerdFont, and on both installs the Retina weight is available.

xorgy commented 1 month ago

@fredizzimo Yeah I was getting a bit confused by that, as a fellow Arch Linux user. It's possible that you have overriding configs on one machine that you do not have on the other, or that if you forced cache reconstruction on both they would become the same. Have you tried running fc-cache -sf? It might be an issue with post-install steps or something silly like that.

fredizzimo commented 1 month ago

I will try to debug exactly what happens during the weekend when I have access to that machine.

BTW, I have tests for all the bugs I have reported here: https://github.com/fredizzimo/vide/blob/305d11fd76dd54cd07d7b757172fe65482ec033e/src/test.rs#L305-L756

Those goes through our code, but it would probably be worth introducing something similar here as well.

xorgy commented 1 month ago

Thanks, I'll take another look.

fredizzimo commented 1 month ago

I have now debugged it, and here's what's happening

The fontconfig information is correct

❯ fc-list :family style file weight family | grep FiraCodeNerdFont-
/usr/share/fonts/TTF/FiraCodeNerdFont-Bold.ttf: FiraCode Nerd Font:style=Bold:weight=200
/usr/share/fonts/TTF/FiraCodeNerdFont-Light.ttf: FiraCode Nerd Font,FiraCode Nerd Font Light:style=Light,Regular:weight=50
/usr/share/fonts/TTF/FiraCodeNerdFont-Medium.ttf: FiraCode Nerd Font,FiraCode Nerd Font Med:style=Medium,Regular:weight=100
/usr/share/fonts/TTF/FiraCodeNerdFont-Retina.ttf: FiraCode Nerd Font,FiraCode Nerd Font Ret:style=Retina,Regular:weight=90
/usr/share/fonts/TTF/FiraCodeNerdFont-SemiBold.ttf: FiraCode Nerd Font,FiraCode Nerd Font SemBd:style=SemiBold,Regular:weight=180
/usr/share/fonts/TTF/FiraCodeNerdFont-Regular.ttf: FiraCode Nerd Font:style=Regular:weight=80

But not every font has a weight defined here https://github.com/linebender/parley/blob/6ecf702d68bfe4402dd0a9a362afccffbeef76f4/fontique/src/backend/fontconfig/cache.rs#L118-L209

Because the weight and some other properties are not cleared here https://github.com/linebender/parley/blob/6ecf702d68bfe4402dd0a9a362afccffbeef76f4/fontique/src/backend/fontconfig/cache.rs#L77-L82

Then the weight remains set to what it was before parse_font was called. In my case for the normal style that happens to be 800 - ExtraBold.

Then later on the maybe_override_attributes forces the loaded normal font to have a weight of 800. https://github.com/linebender/parley/blob/6ecf702d68bfe4402dd0a9a362afccffbeef76f4/fontique/src/font.rs#L208-L224

Now, it's no longer matching in this condition https://github.com/linebender/parley/blob/6ecf702d68bfe4402dd0a9a362afccffbeef76f4/fontique/src/matching.rs#L380, so the Retina variant becomes the lowest weight closest to normal to use.

Changing the clear function to clear everything to the default values might fix the problem, but I would rather see that something more stable like https://docs.rs/fontconfig/latest/fontconfig/ was used instead of trying to parse the caches manually.

fredizzimo commented 1 month ago

Yes adding:

diff --git a/fontique/src/backend/fontconfig/cache.rs b/fontique/src/backend/fontconfig/cache.rs
index e13c93e..8b48501 100644
--- a/fontique/src/backend/fontconfig/cache.rs
+++ b/fontique/src/backend/fontconfig/cache.rs
@@ -79,6 +79,9 @@ impl CachedFont {
         self.path.clear();
         self.index = 0;
         self.coverage.clear();
+        self.weight = Weight::default();
+        self.style = Style::default();
+        self.stretch = Stretch::default();
     }
 }

Seems to fix this issue, and also these

xorgy commented 1 month ago

Thank you for drilling down on this. I think that your last fix here is good. If you want you can put it up as a PR or I will do it.

As for linking against actual fontconfig, we would like to avoid that for a variety of reasons.