Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces variable-font references in Changes
Sequence DiagramsequenceDiagram
participant Config as fonts.yaml
participant Loader as load_fonts()
participant PathCheck as Path Check
participant DirRead as get_font_files_in_directory()
participant FileLoad as load_font_file()
participant FamilyReg as load_family_font()
participant GlobalReg as load_global_fallback()
participant FontDB as fontdb
Config->>Loader: iterate named families & global fallbacks
Loader->>PathCheck: for each path, is it a directory?
rect rgb(220,245,255)
Note over PathCheck,FileLoad: file path branch
PathCheck->>FileLoad: No → load single file
FileLoad->>FontDB: add font faces, return LoadedFont
FileLoad->>FamilyReg: assign Primary or Fallback based on FamilyFontType
FamilyReg->>FontDB: register family mapping
end
rect rgb(245,230,255)
Note over PathCheck,DirRead: directory path branch
PathCheck->>DirRead: Yes → list & sort *.ttf/*.ttc/*.otf
loop for each sorted file
DirRead->>FileLoad: load file
FileLoad->>FontDB: add font faces, return LoadedFont
alt family entry
FileLoad->>FamilyReg: first→Primary, subsequent→Fallback
FamilyReg->>FontDB: register entries
else global-fallback entry
FileLoad->>GlobalReg: register as global fallback
GlobalReg->>FontDB: register fallback mapping
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/fonts.rs (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the font loading system to support loading fonts from directories in addition to individual files. The refactoring extracts the font loading logic into smaller, focused functions and introduces a FamilyFontType enum to distinguish between primary and fallback fonts.
Key changes:
- Introduces directory support for font loading, allowing fonts to be organized in folders
- Refactors font loading into smaller, more testable functions with better separation of concerns
- Replaces boolean
is_primaryflag with a more expressiveFamilyFontTypeenum
Reviewed Changes
Copilot reviewed 3 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/fonts.rs | Complete refactoring of font loading logic to support directories, with new enum and helper functions for better code organization |
| fonts.yaml | Updates sans-serif font path from a variable font file to a specific NotoSansCJK Medium font |
| fonts/NotoSansCJK/LICENSE | Adds SIL Open Font License for the NotoSansCJK font family |
| fonts/NotoSansCJK-VariableFont.ttc | New font file added to support CJK characters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fonts.yaml(1 hunks)fonts/NotoSansCJK/LICENSE(1 hunks)src/fonts.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fonts.rs (1)
src/yaml_loader.rs (1)
load_binary(22-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Code Coverage
🔇 Additional comments (9)
fonts.yaml (1)
4-4: Verify the intentional move from variable to fixed-weight font.The path changes from a variable font (
NotoSansCJK-VariableFont.ttc) to a fixed-weight font (NotoSansCJK-Medium.ttc). Given the PR title mentions "fix variable font weights", this suggests variable fonts were causing issues.Is this change an intentional workaround to avoid variable font problems, or is fixing proper variable font support planned for the future?
fonts/NotoSansCJK/LICENSE (1)
1-92: Good practice to include font license.Adding the SIL Open Font License file is appropriate for the NotoSansCJK font distribution.
src/fonts.rs (7)
2-18: LGTM: Clean enum design for font type distinction.The
FamilyFontTypeenum withfrom_indexhelper provides a clear way to distinguish primary fonts (index 0) from fallback fonts. The implementation is straightforward and appropriate.
29-30: LGTM: Proper integration of directory-aware loading.The changes correctly determine font type based on index and delegate to the new directory-aware loading functions.
48-71: LGTM: Clear routing logic for files vs directories.Both functions follow a consistent pattern to route file paths and directory paths to appropriate handlers. The implementation is clean and maintainable.
73-101: LGTM: Robust directory scanning with deterministic ordering.The function properly handles directory read failures, filters for valid font extensions, and ensures deterministic loading order through sorting. The error handling and logic are sound.
103-116: LGTM: Correct fallback cascading for directory-based fonts.The logic correctly handles primary vs fallback assignment:
- If the directory path is marked as
Primary(index 0), the first font file becomes primary and subsequent files become fallbacks.- If the directory path is already a fallback (index > 0), all fonts remain fallbacks.
This provides sensible cascading behavior for directories containing multiple font weights.
148-156: LGTM: Clean logging helper.Centralizing the logging format in a helper function improves maintainability and ensures consistent log output.
158-191: LGTM: Proper family and fallback font handling.Both functions correctly implement the distinction between primary fonts (which are set as family defaults) and fallback fonts (which are loaded but not set as defaults). The logging clearly indicates the role of each loaded font.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fonts/Roboto/OFL.txt (1)
9-9: Typographical refinement: use an en dash for date ranges.In formal typography, date ranges are typically separated by an en dash (–) rather than a hyphen (-). Consider updating this line to read:
SIL OPEN FONT LICENSE Version 1.1 – 26 February 2007.This is a minor stylistic improvement and does not affect the legal validity of the license.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (55)
fonts/Roboto-VariableFont.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Black.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-BlackItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Bold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-BoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-ExtraBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-ExtraBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-ExtraLight.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-ExtraLightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Italic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Light.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-LightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Medium.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-MediumItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Regular.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-SemiBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-SemiBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-Thin.ttfis excluded by!**/*.ttffonts/Roboto/Roboto-ThinItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Black.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-BlackItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Bold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-BoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-ExtraBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-ExtraBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-ExtraLight.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-ExtraLightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Italic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Light.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-LightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Medium.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-MediumItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Regular.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-SemiBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-SemiBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-Thin.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_Condensed-ThinItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Black.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-BlackItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Bold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-BoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-ExtraBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-ExtraBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-ExtraLight.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-ExtraLightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Italic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Light.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-LightItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Medium.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-MediumItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Regular.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-SemiBold.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-SemiBoldItalic.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-Thin.ttfis excluded by!**/*.ttffonts/Roboto/Roboto_SemiCondensed-ThinItalic.ttfis excluded by!**/*.ttf
📒 Files selected for processing (2)
fonts.yaml(1 hunks)fonts/Roboto/OFL.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fonts.yaml
🧰 Additional context used
🪛 LanguageTool
fonts/Roboto/OFL.txt
[typographical] ~9-~9: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...------ SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 -----------------------...
(DASH_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Coverage
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fonts.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fonts.rs (1)
src/yaml_loader.rs (1)
load_binary(22-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (4)
src/fonts.rs (4)
4-18: LGTM! Clean enum design for font type semantics.The
FamilyFontTypeenum andfrom_indexconstructor provide clear semantics for distinguishing primary fonts (which become the family default) from fallback fonts. The logic is straightforward and correct.
20-46: LGTM! Well-structured refactoring of the font loading flow.The main
load_fontsfunction now cleanly delegates to helper functions that handle both file and directory paths, with proper primary/fallback semantics based on position in the YAML arrays.
139-140: Previous safety concern has been addressed.The past review comments flagged potential panic on
face.families[0]access. The current code correctly usesface.families.first()?, which safely returnsNoneif the families array is empty, eliminating the panic risk.
193-301: LGTM! Comprehensive test coverage for the new functionality.The tests thoroughly cover edge cases including empty directories, filtering non-font files, deterministic sorting, nonexistent paths, and subdirectory handling. The test structure is clear and assertions are appropriate.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 67 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
twangodev
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 51 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fonts/NotoSans/OFL.txt (1)
1-10: License file is complete and properly attributed.The SIL OFL 1.1 text is correctly included with appropriate copyright attribution. This is a necessary compliance document.
Minor typographical note: Line 9 uses a hyphen to separate the date range. Consider using an en-dash (–) instead of hyphen (-) between "Version 1.1" and "26 February 2007" for typographical accuracy, though this is purely stylistic.
fonts/NotoSansArabic/OFL.txt (1)
1-10: License file is complete and properly attributed.The SIL OFL 1.1 text is correctly included with appropriate copyright attribution pointing to the Arabic font repository. Duplication of the license text across font directories is standard practice for proper distribution compliance.
Same minor typographical note as the NotoSans license: Line 9 uses a hyphen to separate the date range. Consider using an en-dash (–) instead of hyphen (-) between "Version 1.1" and "26 February 2007" for consistency and typographical accuracy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
fonts/NotoSans-VariableFont.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Black.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-BlackItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Bold.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-BoldItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-ExtraBold.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-ExtraBoldItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-ExtraLight.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-ExtraLightItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Italic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Light.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-LightItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Medium.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-MediumItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Regular.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-SemiBold.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-SemiBoldItalic.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-Thin.ttfis excluded by!**/*.ttffonts/NotoSans/NotoSans-ThinItalic.ttfis excluded by!**/*.ttffonts/NotoSansArabic-VariableFont.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Black.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Bold.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-ExtraBold.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-ExtraLight.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Light.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Medium.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Regular.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-SemiBold.ttfis excluded by!**/*.ttffonts/NotoSansArabic/NotoSansArabic-Thin.ttfis excluded by!**/*.ttf
📒 Files selected for processing (3)
fonts.yaml(1 hunks)fonts/NotoSans/OFL.txt(1 hunks)fonts/NotoSansArabic/OFL.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fonts.yaml
🧰 Additional context used
🪛 LanguageTool
fonts/NotoSans/OFL.txt
[typographical] ~9-~9: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...------ SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007 -----------------------...
(DASH_RULE)
fonts/NotoSansArabic/OFL.txt
[typographical] ~9-~9: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...-----
SIL OPEN FONT LICENSE Version 1.1 - 26 February 2007
----------------------...
(DASH_RULE)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 62 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 62 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
Style
Documentation
Refactor