Skip to content

Conversation

@Diaoul
Copy link
Contributor

@Diaoul Diaoul commented Dec 16, 2020

As discussed here: #2248

@Diaoul
Copy link
Contributor Author

Diaoul commented Dec 16, 2020

I'll do a pass on the docs when I get the validation of the implementation 📝

For reference:

cat ~/.config/fontconfig/fonts.conf
<?xml version="1.0"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
  <alias>
    <family>monospace</family>
      <prefer>
        <family>Fira Code</family>
      </prefer>
  </alias>
  <match target="font">
    <test name="family">
      <string>Fira Code</string>
    </test>
    <edit name="fontfeatures" mode="append">
      <string>ss01</string>
    </edit>
  </match>
</fontconfig>

@kovidgoyal
Copy link
Owner

Looks generally fine with a quick glance over. Add the docs in config_data.py and I will review in detail.

@Diaoul
Copy link
Contributor Author

Diaoul commented Dec 16, 2020

Actually there is a bug in pattern_as_dict: it does not handle multiple values correctly. This applies to FC_FONT_FEATURES but maybe others as well. For reference, here is how multiple values are handled by FcPatternPrint using FcValueListPrint.

@kovidgoyal
Copy link
Owner

Feel free to send a separate PR to fix it or wait and I will get around
to it when I have a moment.

@kovidgoyal
Copy link
Owner

Also, what's the bug, as far as I know, patter_as_dict doesnt get any non-scalar values currently.

@Diaoul
Copy link
Contributor Author

Diaoul commented Dec 16, 2020

I just fixed it for font_features. As for the others I don't know if they are lists or not and I could not find a reliable source of information for this. Here FC_FONT_FEATURES is defined as a String...

I've seen references to fonts being able to be part of multiple families somewhere (sorry lost the reference) and despite family being singular, the description reads Font family names. 🤷‍♂️

} \
if (PyDict_SetItemString(ans, #name, l) != 0) { Py_CLEAR(l); Py_CLEAR(ans); return NULL; } \
Py_CLEAR(l); \
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please doublecheck that one @kovidgoyal, I tried to make sure I've cleaned up properly but I might have missed something.

@kovidgoyal kovidgoyal merged commit 6681652 into kovidgoyal:master Dec 17, 2020
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.

2 participants