-
Notifications
You must be signed in to change notification settings - Fork 66
refactor: make font install suggestions generic instead of JetBrains-specific (#728) #757
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -797,6 +797,85 @@ impl FontConfigInner { | |||||||||||||||||||||||||||||
| Ok((handles, loaded)) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Returns the download URL for well-known programming fonts, | ||||||||||||||||||||||||||||||
| /// or `None` for unrecognised font names. | ||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+802
to
+821
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This 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
} |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Suggest installation command for a missing font based on the platform | ||||||||||||||||||||||||||||||
| fn suggest_font_install(&self, font_name: &str) -> String { | ||||||||||||||||||||||||||||||
| let font_slug = font_name.to_lowercase().replace(" ", "-"); | ||||||||||||||||||||||||||||||
| let download_hint = match Self::known_font_url(font_name) { | ||||||||||||||||||||||||||||||
| Some(url) => format!("\n - Or download from: {}", url), | ||||||||||||||||||||||||||||||
| None => String::from("\n - Or manually download the font from its official website"), | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| #[cfg(target_os = "linux")] | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| // Map well-known fonts to their Debian/Ubuntu package names | ||||||||||||||||||||||||||||||
| let lower = font_name.to_lowercase(); | ||||||||||||||||||||||||||||||
| let deb_package = if lower.contains("jetbrains") { | ||||||||||||||||||||||||||||||
| "fonts-jetbrains-mono".to_string() | ||||||||||||||||||||||||||||||
| } else if lower.contains("fira") { | ||||||||||||||||||||||||||||||
| "fonts-firacode".to_string() | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| format!("fonts-{}", font_slug) | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| format!( | ||||||||||||||||||||||||||||||
| "On Linux, you can install '{}' using your package manager:\n\ | ||||||||||||||||||||||||||||||
| - Debian/Ubuntu: sudo apt install {}\n\ | ||||||||||||||||||||||||||||||
| - Fedora: sudo dnf install {}\n\ | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggested Fedora package name, which is derived from To improve this, you could create a |
||||||||||||||||||||||||||||||
| - Or manually install to ~/.local/share/fonts/{}", | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| - Or manually install to ~/.local/share/fonts/{}", | |
| - Or manually install to ~/.local/share/fonts/{}\n{}", |
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.
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.
Copilot
AI
Mar 10, 2026
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.
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()) |
Copilot
AI
Mar 10, 2026
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.
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); |
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.
Match known fonts by normalized family name, not substring.
contains()can classify the wrong family here. A request forFira Mono, for example, will currently get theFira CodeURL, 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
🤖 Prompt for AI Agents