Conversation
There was a problem hiding this comment.
Pull request overview
Adds fuzzy font family resolution to FontCache so mis-cased or prefix font family names can still produce a usable skia::Font, and introduces tests around the new matching behavior.
Changes:
- Added
FontCache::fuzzy_match_font(case-insensitive exact match + prefix match). - Updated
make_font_with_fallbackto try fuzzy matching before generic fallbacks. - Added unit tests for exact/case-insensitive/prefix matching and fallback behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // These tests require fonts-dejavu-core (Ubuntu/Debian) or ttf-dejavu (Arch) | ||
| // to be installed, providing "DejaVu Sans", "DejaVu Sans Condensed", etc. | ||
|
|
There was a problem hiding this comment.
These unit tests depend on specific system fonts (DejaVu) being installed, which is likely to be flaky across CI runners and developer machines (macOS/Windows/minimal containers). Consider refactoring the “pick best family name” logic into a pure helper that can be tested with a fixed list of family names, or otherwise gate/ignore these tests when the required font families aren’t present.
| let font = cache.fuzzy_match_font("DejaVu", style, 12.0); | ||
| assert!( | ||
| font.is_some(), | ||
| "prefix match for 'DejaVu' should find a font" |
There was a problem hiding this comment.
prefix_match_picks_shortest currently only asserts that some font is returned, but it doesn’t verify the “picks shortest” behavior described in the test name/comment. If you keep this test, assert the selected family name (or at least that the chosen match has the minimal length among the prefix candidates) so the intended behavior is actually covered.
| let font = cache.fuzzy_match_font("DejaVu", style, 12.0); | |
| assert!( | |
| font.is_some(), | |
| "prefix match for 'DejaVu' should find a font" | |
| let font = cache | |
| .fuzzy_match_font("DejaVu", style, 12.0) | |
| .expect("prefix match for 'DejaVu' should find a font"); | |
| let family_name = font.typeface().family_name(); | |
| assert_eq!( | |
| family_name, | |
| "DejaVu Sans", | |
| "expected shortest DejaVu family ('DejaVu Sans'), got '{}'", | |
| family_name |
| fn fallback_with_fuzzy_returns_font() { | ||
| let cache = make_test_cache(); | ||
| let style = layers::skia::FontStyle::normal(); | ||
| // make_font_with_fallback should use fuzzy matching before falling to generic fallbacks |
There was a problem hiding this comment.
Comment grammar: “before falling to generic fallbacks” reads like a typo; it should be “before falling back to generic fallbacks” (or rephrase to avoid repeating “fallback”).
| // make_font_with_fallback should use fuzzy matching before falling to generic fallbacks | |
| // make_font_with_fallback should use fuzzy matching before falling back to generic fallbacks |
| // Try fuzzy matching (case-insensitive, prefix) | ||
| if let Some(font) = self.fuzzy_match_font(family.as_ref(), style, size) { | ||
| return font; | ||
| } |
There was a problem hiding this comment.
make_font_with_fallback is called from render paths, so if the configured font name differs only by case/prefix, this will run fuzzy_match_font on every frame (iterating all system families and allocating lowercased strings). Consider caching the resolved family name (e.g., map requested->matched) or normalizing the config font family once, so the O(N) scan happens at most once per distinct family string.
| if name_lower == family_lower { | ||
| tracing::info!("Font '{}' matched (case-insensitive) to '{}'", family, name); | ||
| return self.make_font(&name, style, size); |
There was a problem hiding this comment.
The tracing::info! logs inside fuzzy matching can become very noisy if the user’s configured font family casing doesn’t match the system (since this path can run repeatedly during rendering). Consider lowering these to debug/trace and/or logging only once per distinct (requested->matched) mapping when the fuzzy match is first learned/cached.
No description provided.