refactor: make font install suggestions generic instead of JetBrains-specific (#728)#757
Conversation
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
📝 WalkthroughWalkthroughAdded font existence validation with user-friendly error messages to FontConfigInner. Introduced helper methods to identify known fonts, generate platform-specific installation guidance, and provide download hints. Enhanced error handling logs which font is used as fallback when requested font unavailable. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the font validation and suggestion mechanism. It moves away from JetBrains Mono-specific hardcoding to provide more generic, platform-aware installation instructions for a wider range of popular programming fonts. The changes also enhance the clarity of error messages when fonts are not found and improve the logging around font selection, ultimately providing a better user experience when configuring fonts. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wezterm-font/src/lib.rs`:
- Around line 802-818: known_font_url currently uses substring matching
(contains) which misclassifies families (e.g., "Fira Mono" matches "fira" and
returns the Fira Code URL); replace the contains-based logic with an explicit
allow-list mapping of normalized family names/aliases to their URLs and perform
exact equality checks on a normalized family string (e.g., lowercased and
trimmed). Update known_font_url to look up the normalized font_name in that
mapping (a static slice/array of (&str, &str) or a lazy/static HashMap) and
return the mapped Option<&'static str> for exact matches only, ensuring aliases
like "fira mono" map to the correct URL and preventing unintended substring
matches.
- Around line 843-849: The Linux install message places download_hint into the
final "{}" placeholder causing the path to render incorrectly; update the format
argument order (or placeholders) so the final placeholder uses font_slug (or the
intended package/path variable) instead of download_hint—specifically adjust the
format call that references font_name, deb_package, font_slug, download_hint so
the last placeholder is populated by font_slug and download_hint is used only
where its hint text belongs.
| fn known_font_url(font_name: &str) -> Option<&'static str> { | ||
| let lower = font_name.to_lowercase(); | ||
| if lower.contains("jetbrains") { | ||
| Some("https://github.com/JetBrains/JetBrainsMono") | ||
| } else if lower.contains("fira") { | ||
| Some("https://github.com/tonsky/FiraCode") | ||
| } else if lower.contains("cascadia") { | ||
| Some("https://github.com/microsoft/cascadia-code") | ||
| } else if lower.contains("source code") { | ||
| Some("https://github.com/adobe-fonts/source-code-pro") | ||
| } else if lower.contains("hack") { | ||
| Some("https://github.com/source-foundry/Hack") | ||
| } else if lower.contains("iosevka") { | ||
| Some("https://github.com/be5invis/Iosevka") | ||
| } else if lower.contains("victor mono") { | ||
| Some("https://github.com/rubjo/victor-mono") | ||
| } else { |
There was a problem hiding this comment.
Match known fonts by normalized family name, not substring.
contains() can classify the wrong family here. A request for Fira Mono, for example, will currently get the Fira Code URL, so the remediation hint points users at the wrong font. Use a normalized allow-list of exact family names/aliases instead.
Proposed fix
fn known_font_url(font_name: &str) -> Option<&'static str> {
- let lower = font_name.to_lowercase();
- if lower.contains("jetbrains") {
- Some("https://github.com/JetBrains/JetBrainsMono")
- } else if lower.contains("fira") {
- Some("https://github.com/tonsky/FiraCode")
- } else if lower.contains("cascadia") {
- Some("https://github.com/microsoft/cascadia-code")
- } else if lower.contains("source code") {
- Some("https://github.com/adobe-fonts/source-code-pro")
- } else if lower.contains("hack") {
- Some("https://github.com/source-foundry/Hack")
- } else if lower.contains("iosevka") {
- Some("https://github.com/be5invis/Iosevka")
- } else if lower.contains("victor mono") {
- Some("https://github.com/rubjo/victor-mono")
- } else {
- None
- }
+ match font_name.trim().to_ascii_lowercase().as_str() {
+ "jetbrains mono" | "jetbrainsmono" => {
+ Some("https://github.com/JetBrains/JetBrainsMono")
+ }
+ "fira code" | "firacode" => Some("https://github.com/tonsky/FiraCode"),
+ "cascadia code" | "cascadia" => {
+ Some("https://github.com/microsoft/cascadia-code")
+ }
+ "source code pro" | "source code" => {
+ Some("https://github.com/adobe-fonts/source-code-pro")
+ }
+ "hack" => Some("https://github.com/source-foundry/Hack"),
+ "iosevka" => Some("https://github.com/be5invis/Iosevka"),
+ "victor mono" => Some("https://github.com/rubjo/victor-mono"),
+ _ => None,
+ }
}As per coding guidelines, "Use allow-lists for input validation to prevent injection attacks".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn known_font_url(font_name: &str) -> Option<&'static str> { | |
| let lower = font_name.to_lowercase(); | |
| if lower.contains("jetbrains") { | |
| Some("https://github.com/JetBrains/JetBrainsMono") | |
| } else if lower.contains("fira") { | |
| Some("https://github.com/tonsky/FiraCode") | |
| } else if lower.contains("cascadia") { | |
| Some("https://github.com/microsoft/cascadia-code") | |
| } else if lower.contains("source code") { | |
| Some("https://github.com/adobe-fonts/source-code-pro") | |
| } else if lower.contains("hack") { | |
| Some("https://github.com/source-foundry/Hack") | |
| } else if lower.contains("iosevka") { | |
| Some("https://github.com/be5invis/Iosevka") | |
| } else if lower.contains("victor mono") { | |
| Some("https://github.com/rubjo/victor-mono") | |
| } else { | |
| fn known_font_url(font_name: &str) -> Option<&'static str> { | |
| match font_name.trim().to_ascii_lowercase().as_str() { | |
| "jetbrains mono" | "jetbrainsmono" => { | |
| Some("https://github.com/JetBrains/JetBrainsMono") | |
| } | |
| "fira code" | "firacode" => Some("https://github.com/tonsky/FiraCode"), | |
| "cascadia code" | "cascadia" => { | |
| Some("https://github.com/microsoft/cascadia-code") | |
| } | |
| "source code pro" | "source code" => { | |
| Some("https://github.com/adobe-fonts/source-code-pro") | |
| } | |
| "hack" => Some("https://github.com/source-foundry/Hack"), | |
| "iosevka" => Some("https://github.com/be5invis/Iosevka"), | |
| "victor mono" => Some("https://github.com/rubjo/victor-mono"), | |
| _ => None, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wezterm-font/src/lib.rs` around lines 802 - 818, known_font_url currently
uses substring matching (contains) which misclassifies families (e.g., "Fira
Mono" matches "fira" and returns the Fira Code URL); replace the contains-based
logic with an explicit allow-list mapping of normalized family names/aliases to
their URLs and perform exact equality checks on a normalized family string
(e.g., lowercased and trimmed). Update known_font_url to look up the normalized
font_name in that mapping (a static slice/array of (&str, &str) or a lazy/static
HashMap) and return the mapped Option<&'static str> for exact matches only,
ensuring aliases like "fira mono" map to the correct URL and preventing
unintended substring matches.
| format!( | ||
| "On Linux, you can install '{}' using your package manager:\n\ | ||
| - Debian/Ubuntu: sudo apt install {}\n\ | ||
| - Fedora: sudo dnf install {}\n\ | ||
| - Or manually install to ~/.local/share/fonts/{}", | ||
| font_name, deb_package, font_slug, download_hint | ||
| ) |
There was a problem hiding this comment.
Fix the Linux install message formatting.
The last {} currently receives download_hint, so the rendered output turns into ~/.local/share/fonts/<download hint> and drops the intended path/package text. That makes the Linux remediation message malformed right in the user-facing error path.
Proposed fix
format!(
"On Linux, you can install '{}' using your package manager:\n\
- Debian/Ubuntu: sudo apt install {}\n\
- Fedora: sudo dnf install {}\n\
- - Or manually install to ~/.local/share/fonts/{}",
- font_name, deb_package, font_slug, download_hint
+ - Or manually install to ~/.local/share/fonts/{}{}",
+ font_name, deb_package, font_slug, font_slug, download_hint
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| format!( | |
| "On Linux, you can install '{}' using your package manager:\n\ | |
| - Debian/Ubuntu: sudo apt install {}\n\ | |
| - Fedora: sudo dnf install {}\n\ | |
| - Or manually install to ~/.local/share/fonts/{}", | |
| font_name, deb_package, font_slug, download_hint | |
| ) | |
| format!( | |
| "On Linux, you can install '{}' using your package manager:\n\ | |
| - Debian/Ubuntu: sudo apt install {}\n\ | |
| - Fedora: sudo dnf install {}\n\ | |
| - Or manually install to ~/.local/share/fonts/{}{}", | |
| font_name, deb_package, font_slug, font_slug, download_hint | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wezterm-font/src/lib.rs` around lines 843 - 849, The Linux install message
places download_hint into the final "{}" placeholder causing the path to render
incorrectly; update the format argument order (or placeholders) so the final
placeholder uses font_slug (or the intended package/path variable) instead of
download_hint—specifically adjust the format call that references font_name,
deb_package, font_slug, download_hint so the last placeholder is populated by
font_slug and download_hint is used only where its hint text belongs.
There was a problem hiding this comment.
Pull request overview
Refactors font-missing validation messaging in wezterm-font to be more generic (not JetBrains Mono-specific) and to provide platform-specific install/download suggestions when a configured font cannot be loaded, addressing Issue #728’s request for user-friendly warnings and guidance.
Changes:
- Added a
known_font_urlhelper covering 7 popular programming fonts. - Added
suggest_font_installto generate platform-specific installation/download guidance. - Updated the missing-font warning path to report the chosen fallback font and emit additional logs.
| // default) so the `unwrap_or` branch is a safety-net only. | ||
| let fallback_font = handles | ||
| .first() | ||
| .map(|h| h.names().family_name.clone()) |
There was a problem hiding this comment.
ParsedFont::names() returns a Names struct with fields like family and full_name; there is no family_name field. As written, this won’t compile. Use h.names().family.clone() (or full_name if you prefer) when deriving fallback_font.
| .map(|h| h.names().family_name.clone()) | |
| .map(|h| h.names().family.clone()) |
|
|
||
| // Log which font is actually being used for the primary font | ||
| if let Some(first_handle) = handles.first() { | ||
| log::info!("Using font: {}", first_handle.names().full_name); |
There was a problem hiding this comment.
log::info!("Using font: ...") in resolve_font_helper will run on every cache miss (and potentially twice when cap-height scaling triggers), which can be quite noisy at the default log level. Consider downgrading to debug/trace, or gating it so it only logs once per config generation / only when the primary font differs from the requested family.
| log::info!("Using font: {}", first_handle.names().full_name); | |
| log::debug!("Using font: {}", first_handle.names().full_name); |
| "On Linux, you can install '{}' using your package manager:\n\ | ||
| - Debian/Ubuntu: sudo apt install {}\n\ | ||
| - Fedora: sudo dnf install {}\n\ | ||
| - Or manually install to ~/.local/share/fonts/{}", |
There was a problem hiding this comment.
In the Linux install suggestion, the last {} placeholder is being used to append download_hint after the ~/.local/share/fonts/ path. This works, but it’s hard to read/maintain because the placeholder visually looks like part of the path. Consider removing the {} from the path and appending download_hint explicitly at the end of the format string (with a preceding newline) so the intent is clearer.
| - Or manually install to ~/.local/share/fonts/{}", | |
| - Or manually install to ~/.local/share/fonts/{}\n{}", |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the font validation to provide generic installation suggestions for missing fonts, which is a great improvement for user experience. The implementation introduces platform-specific hints for Linux, macOS, and Windows.
My review includes a couple of suggestions:
- A medium-severity suggestion to refactor the
known_font_urlfunction for better maintainability. - A high-severity correctness issue regarding the generated package names for Fedora, which are likely to be incorrect and cause installation commands to fail.
| format!( | ||
| "On Linux, you can install '{}' using your package manager:\n\ | ||
| - Debian/Ubuntu: sudo apt install {}\n\ | ||
| - Fedora: sudo dnf install {}\n\ |
There was a problem hiding this comment.
The suggested Fedora package name, which is derived from font_slug, is likely incorrect for most of the well-known fonts. For example, for 'Fira Code', the package is fira-code-fonts, but the suggestion would be sudo dnf install fira-code. This will lead to a 'package not found' error for users on Fedora.
To improve this, you could create a fedora_package variable with correct names for known fonts, similar to how deb_package is handled. If a consistent naming pattern can't be found for all fonts, it might be safer to suggest a search command, like sudo dnf search <keywords>.
| fn known_font_url(font_name: &str) -> Option<&'static str> { | ||
| let lower = font_name.to_lowercase(); | ||
| if lower.contains("jetbrains") { | ||
| Some("https://github.com/JetBrains/JetBrainsMono") | ||
| } else if lower.contains("fira") { | ||
| Some("https://github.com/tonsky/FiraCode") | ||
| } else if lower.contains("cascadia") { | ||
| Some("https://github.com/microsoft/cascadia-code") | ||
| } else if lower.contains("source code") { | ||
| Some("https://github.com/adobe-fonts/source-code-pro") | ||
| } else if lower.contains("hack") { | ||
| Some("https://github.com/source-foundry/Hack") | ||
| } else if lower.contains("iosevka") { | ||
| Some("https://github.com/be5invis/Iosevka") | ||
| } else if lower.contains("victor mono") { | ||
| Some("https://github.com/rubjo/victor-mono") | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
This if/else if chain can be refactored to be more maintainable and idiomatic. Using a static array of tuples and a loop would make it easier to add or modify fonts in the future and is generally more readable than a long chain of if/else if statements.
fn known_font_url(font_name: &str) -> Option<&'static str> {
let lower = font_name.to_lowercase();
const FONT_URLS: &[(&str, &str)] = &[
("jetbrains", "https://github.com/JetBrains/JetBrainsMono"),
("fira", "https://github.com/tonsky/FiraCode"),
("cascadia", "https://github.com/microsoft/cascadia-code"),
("source code", "https://github.com/adobe-fonts/source-code-pro"),
("hack", "https://github.com/source-foundry/Hack"),
("iosevka", "https://github.com/be5invis/Iosevka"),
("victor mono", "https://github.com/rubjo/victor-mono"),
];
for (name, url) in FONT_URLS {
if lower.contains(name) {
return Some(url);
}
}
None
}
Summary
Refactors the font validation implementation (Issue #728) to:
fonts-jetbrains-monofrom generic Linux suggestionsFontAttributes::default()Fixes #728
Summary by CodeRabbit