font: Refactor font list generation on OpenHarmony platform#44061
Conversation
Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>
|
p.s. I just noticed a while later that I've been running linux tests, so please ignore this and refer to the try run from this PR instead |
|
cc @jschwe |
| UnicodeBlock::HangulSyllables => { | ||
| families.push("Noto Sans CJK"); | ||
| families.push("Noto Serif CJK"); | ||
| families.push("Noto Sans CJK KR"); |
There was a problem hiding this comment.
NotoSansCJK-Regular.ttc contains many fonts, such as Noto Sans CJK KR, Noto Sans CJK SC, etc.
Previously, since the font list creates the family name from the filename itself, the family name created from this specific font file is Noto Sans CJK. However, now that we obtain the family name from the font's name table, we need to change these into more specific family names, such as Noto Sans CJK KR.
| @@ -188,12 +151,6 @@ fn split_noto_font_name(name: &str) -> Vec<String> { | |||
| /// `fontconfig.json` fails for some reason. Beta 1 of OH 5.0 still has a bug in the fontconfig.json | |||
| /// though, so the "normal path" is currently unimplemented. | |||
| fn parse_font_filenames(font_files: Vec<PathBuf>) -> Vec<FontFamily> { | |||
There was a problem hiding this comment.
Perhaps the function name and documentation should be updated.
|
🔨 Triggering try run (#24184326332) for OpenHarmony |
|
Bencher Report| Branch | 44061/PR |
| Testbed | HUAWEI Mate 80 Pro |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
|
✨ Try run (#24184326332) succeeded. |
b4d4c67 to
75af074
Compare
|
I just realized I ran the test on linux... |
|
@jschwe Thanks for the review! I've pushed the suggested changes & removed some redundant tests as well |
| /// Note: For OH 5.0+ this function is intended to only be a fallback path, if parsing the | ||
| /// `fontconfig.json` fails for some reason. Beta 1 of OH 5.0 still has a bug in the fontconfig.json | ||
| /// though, so the "normal path" is currently unimplemented. |
There was a problem hiding this comment.
I think this comment is also outdated now though, right?
There was a problem hiding this comment.
huh, forgot to update it. my bad..
| let style_modifiers = ["Italic"]; | ||
| let width_modifiers = ["Condensed"]; | ||
|
|
||
| fn get_system_font_families(font_files: Vec<PathBuf>) -> Vec<FontFamily> { |
There was a problem hiding this comment.
Maybe we should also instrument this function with #[servo_tracing::instrument(skip_all)] so we can track how long it takes.
There was a problem hiding this comment.
Sure.
BTW, I only mention adding caching mechanism in the near future in the related issue, let me update the PR description as well
| let mut families = enumerate_font_files() | ||
| .inspect_err(|e| error!("Failed to enumerate font files due to `{e:?}`")) | ||
| .map(parse_font_filenames) | ||
| .map(get_system_font_families) |
There was a problem hiding this comment.
If this takes long, we might want to cache this, although that could / should be a followup PR.
jschwe
left a comment
There was a problem hiding this comment.
Looks fine to me, except the nit about the outdated documentation comment, please adjust that
75af074 to
49caddd
Compare
Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>
49caddd to
dd5b99f
Compare
| } | ||
| }, | ||
| Collection(font_collection) => { | ||
| // Process all the font files within the collection one by one. |
There was a problem hiding this comment.
Depending on how long this takes, we could consider using a par_iter here via rayon, to make the first-start faster. (but also future PR stuff)
There was a problem hiding this comment.
There could be many fonts within one collection; for your reference, there are 10 fonts in NotoSansCJK-Regular.ttc. But there aren't many .ttc files in the first place (at least in Huawei Mate60).
But yeah; parallelizing it sounds like a cool idea :)
|
🔨 Triggering try run (#24199200755) for OpenHarmony |
|
| Branch | 44061/PR |
| Testbed | HUAWEI Mate 60 Pro |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
|
✨ Try run (#24199200755) succeeded. |
Removed `hardcoded_font_families()`, which is previously used to create `FontFamily` for HarmonyOS emoji fonts (`HMOS Color Emoji` & `HMOS Color Emoji Flags`). This function is no longer needed since [servo#44061](servo#44061) because we're now using OHOS API to generate `FontList`, which can detect all available system fonts, including the emoji ones. Testing: Manually tested. Fixes: N/A Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>
font: Use
read_fontsmodule to generate font list on OpenHarmony platform. Specifically:Family nameis obtained from the font'snametable.width&weightis obtained from the font'sos2table.styleis obtained from the font'spostscripttable.Additionally, I'd like to mention that I plan to add a caching mechanism to store the font list in the near future (as mentioned in the related issue)
Reference: TrueType reference manual
Testing: No behavior changes expected.
Fixes: Part of #43596