Conversation
Store and preserve included feature files instead of flattening them into a single features string. Font now has a feature_files: BTreeMap<PathBuf, String> field and a features_expanded() helper to produce the expanded feature text. Loading was updated to collect included feature files (with cycle/missing-file checks) via non_file_io::load_feature_files, and saving writes out the main features.fea plus any included files, creating parent dirs as needed. Line endings in feature files are normalized on write. Tests updated/added to verify round-tripping and expansion behavior.
RickyDaMa
left a comment
RickyDaMa
left a comment
There was a problem hiding this comment.
Hey Yanone, was just having an idle nosy at your PR and thought I'd leave some feedback/questions on some of the low-hanging stuff I noticed. Very interested to see how a sans-io interface shapes up in norad 🤓 I'll let Colin guide the direction we take though
| SeError(#[from] SeError), | ||
|
|
||
| /// Failed to write the designspace via a custom sink. | ||
| #[error("failed to write designspace file '{path}' via sink")] |
There was a problem hiding this comment.
| #[error("failed to write designspace file '{path}' via sink")] | |
| #[error("failed to write designspace file '{path}' via sink: {source}")] |
| path: PathBuf, | ||
| }, | ||
| /// Failed to load a file through a custom non-file source. | ||
| #[error("failed to read '{path}' from non-file source")] |
There was a problem hiding this comment.
| #[error("failed to read '{path}' from non-file source")] | |
| #[error("failed to read '{path}' from non-file source: {source}")] |
| #[error("the `public.objectLibs` lib key is managed by norad and must not be set manually")] | ||
| PreexistingPublicObjectLibsKey, | ||
| /// Failed to write a file through a custom non-file sink. | ||
| #[error("failed to write '{path}' via non-file sink")] |
There was a problem hiding this comment.
| #[error("failed to write '{path}' via non-file sink")] | |
| #[error("failed to write '{path}' via non-file sink: {source}")] |
| #[error("failed to write layerinfo.plist file")] | ||
| LayerInfo(#[source] CustomSerializationError), | ||
| /// Failed to write a layer file via a custom sink. | ||
| #[error("failed to write layer file '{path}' via sink")] |
There was a problem hiding this comment.
| #[error("failed to write layer file '{path}' via sink")] | |
| #[error("failed to write layer file '{path}' via sink: {source}")] |
| fn load_features(features_path: &Path) -> Result<String, FontLoadError> { | ||
| let features = fs::read_to_string(features_path).map_err(FontLoadError::FeatureFile)?; | ||
| Ok(features) | ||
| fn normalize_feature_text(contents: &str) -> Vec<u8> { |
There was a problem hiding this comment.
Is there a reason why you transform to a bytes buffer here? Based on the function name I would have expected fn normalize_feature_text<'a>(contents: &'a str) -> Cow<'a, str>, which allows you to return the original str if it's unedited, or an owned buffer otherwise
| fn read_required_text<F, E>(source: &mut F, path: &Path) -> Result<String, FontLoadError> | ||
| where | ||
| F: FnMut(&Path) -> Result<Option<String>, E>, | ||
| E: StdError + Send + Sync + 'static, | ||
| { | ||
| read_optional_text(source, path)?.ok_or(FontLoadError::MissingMetaInfoFile) | ||
| } |
There was a problem hiding this comment.
This method's name makes it seem generic, but the error is specific to metainfo
| let layercontents_str = read_optional_text(source, Path::new(LAYER_CONTENTS_FILE))? | ||
| .ok_or(FontLoadError::MissingLayerContentsFile)?; |
There was a problem hiding this comment.
Unless I'm misreading the code, aren't you basically doing read_required_text with a different error variant?
| read_optional_text(source, &glif_path)?.ok_or(FontLoadError::Layer { | ||
| name: layer_name.to_string(), | ||
| path: glif_path.clone(), | ||
| source: Box::new(LayerLoadError::MissingContentsFile), |
There was a problem hiding this comment.
Is this the right error variant?
| fn normalize_relative_path(path: &Path) -> PathBuf { | ||
| let mut normalized = PathBuf::new(); | ||
| for component in path.components() { | ||
| match component { | ||
| Component::CurDir => {} | ||
| Component::ParentDir => { | ||
| normalized.pop(); | ||
| } | ||
| Component::Normal(part) => normalized.push(part), | ||
| Component::RootDir | Component::Prefix(_) => normalized.push(component.as_os_str()), | ||
| } | ||
| } | ||
| normalized | ||
| } |
There was a problem hiding this comment.
Is this function doing the same thing as your impl here?
| }) | ||
| } | ||
|
|
||
| fn normalize_feature_text(contents: &str) -> Vec<u8> { |
There was a problem hiding this comment.
How come this ended up duplicated?
2 participants
Follow up to #390, a way to load and save UFO files from non-file sources such as hashmaps for instance in was, where native file system access doesn't exist (and can't be spoofed)