Use system fonts as catalog on Desktop App#4035
Use system fonts as catalog on Desktop App#4035Macman1234 wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
![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 adds system font loading for desktop environments using the parley crate. The implementation eagerly loads all system font data and sends it through the message system, which presents significant performance and memory efficiency concerns. Feedback suggests transitioning to lazy loading, avoiding expensive collection clones, and replacing unwrap() calls with safer error handling to prevent application panics. Additionally, the logging level for font discovery should be downgraded from error to debug.
| let mut font_data_vec = Vec::new(); | ||
| font_data_vec.extend(font.load(None).unwrap().data()); | ||
| responses.add(PortfolioMessage::FontLoaded { | ||
| font_family: name.to_owned(), | ||
| font_style: style.to_named_style(), | ||
| data: font_data_vec, | ||
| }); |
There was a problem hiding this comment.
Loading the full data for every system font and sending it through the message system is extremely inefficient. This will cause significant performance degradation and high memory consumption on startup, especially for users with many installed fonts. Font data should be loaded lazily only when a specific font is required for rendering, rather than populating the entire cache upfront.
| // simultaneously?? call font loaded a bunch of times to shove data into font data cache | ||
| let mut system_font_family_names = Vec::new(); | ||
| system_font_family_names.extend(system_font_collection.family_names()); | ||
| log::error!("fonts are: {:?}", system_font_family_names); |
There was a problem hiding this comment.
Using log::error for informational logging of font names is inappropriate. This should be changed to log::debug or log::trace to avoid cluttering the error logs.
| log::error!("fonts are: {:?}", system_font_family_names); | |
| log::debug!("fonts are: {:?}", system_font_family_names); |
| log::error!("fonts are: {:?}", system_font_family_names); | ||
| let mut families_for_catalog = Vec::new(); | ||
| for name in system_font_family_names { | ||
| let family = system_font_collection_again.family_by_name(name).unwrap(); |
There was a problem hiding this comment.
Avoid using unwrap() on family_by_name. If a font family name returned by the collection is unexpectedly missing, this will cause the application to panic. It is safer to handle this case gracefully.
| let family = system_font_collection_again.family_by_name(name).unwrap(); | |
| let Some(family) = system_font_collection_again.family_by_name(name) else { continue }; |
| url: "".to_owned(), | ||
| }; | ||
| let mut font_data_vec = Vec::new(); | ||
| font_data_vec.extend(font.load(None).unwrap().data()); |
There was a problem hiding this comment.
font.load(None) can fail if a font file is corrupt or inaccessible. Using unwrap() here makes the application fragile. Use a guard or if let to skip fonts that cannot be loaded.
let Some(font_data) = font.load(None) else { continue };
font_data_vec.extend(font_data.data());| if Editor::environment().is_desktop() { | ||
| let system_font_context = FontContext::new(); // create system font context | ||
| let mut system_font_collection = system_font_context.collection; | ||
| let mut system_font_collection_again = system_font_collection.clone(); // because parley was not made correctly |
There was a problem hiding this comment.
Cloning the entire font collection is an expensive operation. If parley requires ownership or a mutable reference that prevents using the original system_font_collection, consider if there is a way to restructure the access to avoid this clone.
1 participant
WIP. Current code is definitely placed in the wrong part of the codebase, may not work on platforms other than Linux, needs an option to switch between system and remote font catalogs, and does not populate the default font. Also I need to write a better description here of what the code actually does.