Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions editor/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ vello = { workspace = true }
base64 = { workspace = true }
spin = { workspace = true }
image = { workspace = true }
parley = { workspace = true }

# Optional local dependencies
wgpu-executor = { workspace = true, optional = true }
Expand Down
40 changes: 38 additions & 2 deletions editor/src/messages/portfolio/portfolio_message_handler.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::document::utility_types::document_metadata::LayerNodeIdentifier;
use super::document::utility_types::network_interface;
use super::persistent_state::{PersistentStateMessage, PersistentStateMessageContext, PersistentStateMessageHandler};
use super::utility_types::{CachedData, PanelLayoutSubdivision, PanelType, WorkspacePanelLayout};
use super::utility_types::{CachedData, FontCatalog, FontCatalogFamily, FontCatalogStyle, PanelLayoutSubdivision, PanelType, WorkspacePanelLayout};
use crate::application::{Editor, generate_uuid};
use crate::consts::{DEFAULT_DOCUMENT_NAME, DEFAULT_STROKE_WIDTH, FILE_EXTENSION};
use crate::messages::animation::TimingInformation;
Expand Down Expand Up @@ -34,6 +34,7 @@ use graphene_std::subpath::BezierHandles;
use graphene_std::text::Font;
use graphene_std::vector::misc::HandleId;
use graphene_std::vector::{PointId, SegmentId, Vector, VectorModificationType};
use parley::{FontContext, FontStyle};
use std::path::PathBuf;
use std::vec;

Expand Down Expand Up @@ -441,7 +442,42 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageContext<'_>> for Portfolio
let catalog = &self.cached_data.font_catalog;

if catalog.0.is_empty() {
responses.add_front(FrontendMessage::TriggerFontCatalogLoad);
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


// shove font metadata into cached data catalog
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
log::error!("fonts are: {:?}", system_font_family_names);
log::debug!("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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
let family = system_font_collection_again.family_by_name(name).unwrap();
let Some(family) = system_font_collection_again.family_by_name(name) else { continue };

let mut styles = Vec::new();
for font in family.fonts() {
let style = FontCatalogStyle {
weight: (font.weight().value()) as u32,
italic: font.style() == FontStyle::Italic,
url: "".to_owned(),
};
let mut font_data_vec = Vec::new();
font_data_vec.extend(font.load(None).unwrap().data());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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());

responses.add(PortfolioMessage::FontLoaded {
font_family: name.to_owned(),
font_style: style.to_named_style(),
data: font_data_vec,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

styles.push(style);
}
let family_for_catalog = FontCatalogFamily { name: name.to_owned(), styles };
families_for_catalog.push(family_for_catalog);
}
self.cached_data.font_catalog = FontCatalog(families_for_catalog);
} else {
responses.add_front(FrontendMessage::TriggerFontCatalogLoad);
}
return;
}

Expand Down