-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Some text improvements including falling back to system fonts #4721
Some text improvements including falling back to system fonts #4721
Conversation
…obal scripts" attempt This is a first pass and will need more work to become dynamic as runes require...
Not sure what happened there
How much work is left for this to be ready? |
Same question as Jacalz, also wondering if this will cover Chinese and Korean as well even if the locale is EN. (Those along with Japanese are the most common non-Latin scripts in peoples' music libraries ;) - though the ideal of course would be lookup for all scripts that may need to be rendered regardless of locale setting. |
Yes. It falls back on a rune basis so unidentified characters should all be looked up. I will try and address the test issues this weekend to get the PR landed. |
1bd2de6
to
b4e8ba0
Compare
On Windows and macOS (not Linux, iOS or Android) sometimes the symbol fonts align slightly differently on the sub-pixel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it perhaps would be easier to test this if we could easily disable the built in fonts similar to #2637?
``
I don't see why that makes it easier - if we disable the builtin fonts then it will be down to the OS/setup as to what fonts are found - basically guaranteed to be different unexpectedly on different computers - and may even break on CI. |
I think @Jacalz means that if you can build with no bundled fonts, at least for testing, you don't need an app that has CJK or other less common characters to test the lookup |
Plus now that lookup is in place we probably want to give apps the option to opt out of having any bundled fonts in the binary |
Indeed. I figured it would make things not only easier to test but also allow us to test more parts of the font lookup.
Yup. That would be nice as well down the road as more of a feature than a testing helper :) |
These are good points and were agreed on Slack too as follow up work. This PR still needs a 👍 or 👎 to progress. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me, at least as a first pass. More improvements can be done in follow-ups if needed. Also will be easier to continue testing with it integrated into develop.
Fixes are in separate commits, the fallback code is basically first-pass.
We include lookup for OS font for the current language and whatever font the OS has for japanese, which typically includes most script languages.
This provides good (tested) coverage of rendering out of the box for macOS, iOS, Linux and Android (windows still to be tested).
Memory increase is currently 5% - 20% depending on how many fonts the app uses.
There are more optimisations to come that will counteract the increase.
The code is now faster than before so that's the current tradeoff
Fixes #(issue)
Checklist: