SourceResolver loading by yanone · Pull Request #390 · linebender/norad

yanone · 2026-03-13T07:54:24Z

Summary
This PR introduces a path-based source abstraction for UFO loading so norad can load from non-filesystem backends without exposing a HashMap-specific API.

Motivation
Downstream wasm and in-memory workflows need to provide UFO files from virtual storage.
The goal is to keep norad’s public API backend-agnostic while still supporting those use cases cleanly.

What changed
Added a source resolver abstraction for reading UFO resources by relative path.
Added a filesystem resolver implementation for current disk-based behavior.
Added resolver-based font loading flow, used by existing load entry points.
Added resolver-specific error handling for missing files and IO failures.
Kept default filesystem behavior intact for existing callers.

Compatibility
Existing filesystem loading behavior is preserved.
No HashMap-specific type is introduced in the public API.
This creates a clear extension point for custom loaders in downstream crates.

cmyr

so... lots of notes inline, but I think the general shape of this is okay?

going through this does make me wonder if the closure-based version might not be preferable, since it's easier to get right without needing a lot of back-and-forth?
In either case I think I'd like to see all of this new code live in a new module, like non_file_loading.rs or similar, just so that we can keep it a bit more contained.

cmyr · 2026-03-19T14:42:19Z

src/designspace.rs

+    /// Returns a [`DesignSpaceDocument`] loaded from an XML string.
+    pub fn load_str(contents: &str) -> Result<DesignSpaceDocument, DesignSpaceLoadError> {
+        quick_xml::de::from_str(contents).map_err(DesignSpaceLoadError::DeError)
+    }


total nit but I would prefer this to be right below the main load fn, above.

cmyr · 2026-03-19T14:47:12Z

src/error.rs

+        /// The requested path.
+        path: PathBuf,
+        /// The underlying error.
+        source: IoError,


I don't think io::Error is the right error type, here; the error type on SourceResolver also isn't right, see below.

cmyr · 2026-03-19T14:55:22Z

src/font.rs

+    /// Return UTF-8 contents for the provided path.
+    ///
+    /// Returning `Ok(None)` indicates a missing file.
+    fn get_contents(&self, path: &Path) -> Result<Option<String>, FontLoadError>;


What we want here is some very customizable error type that the implementor can use to communicate why the given resource isn't available; FontLoadError covers a lot of specific cases that aren't relevant to the job of "give me a string for this path".

The 'best' option is probably an associated type, so this would look like:

trait SourceResolver { type Error; fn get_contents(&self, path: &Path) -> Result<Option<String>, Self::Error>; }

but this might be a bit fiddly if you're not that comfortable with rust? Because we would probably need bounds on the error type elsewhere, so that the top-level Resolver variant would contain a Box<dyn std::error::Error>...

cmyr · 2026-03-19T14:56:39Z

src/font.rs

+    /// Resolve a raw request path.
+    fn resolve_raw_path(&self, path: &Path, _requested_from: Option<&Path>) -> PathBuf {
+        path.to_path_buf()
+    }
+
+    /// Canonicalize a path if needed.
+    fn canonicalize(&self, path: &Path) -> Result<PathBuf, FontLoadError> {
+        Ok(path.to_path_buf())
+    }


both of these methods are important for resolving include statements in a FEA file but are (I believe) irrelevant for your use-case, and can be removed.

cmyr · 2026-03-19T14:58:38Z

src/font.rs

+/// Filesystem-backed [`SourceResolver`].
+#[derive(Default)]
+pub struct FileSystemResolver {
+    project_root: PathBuf,
+}
+
+impl FileSystemResolver {
+    /// Create a resolver rooted at `project_root`.
+    pub fn new(project_root: PathBuf) -> Self {
+        Self { project_root }
+    }
+}
+
+impl SourceResolver for FileSystemResolver {
+    fn get_contents(&self, path: &Path) -> Result<Option<String>, FontLoadError> {
+        match fs::read_to_string(path) {
+            Ok(contents) => Ok(Some(contents)),
+            Err(source) if source.kind() == ErrorKind::NotFound => Ok(None),
+            Err(source) => Err(FontLoadError::ResolverIo { path: path.to_path_buf(), source }),
+        }
+    }
+
+    fn resolve_raw_path(&self, path: &Path, requested_from: Option<&Path>) -> PathBuf {
+        if path.is_absolute() {
+            return path.to_path_buf();
+        }
+        if let Some(parent) = requested_from.and_then(Path::parent) {
+            return parent.join(path);
+        }
+        self.project_root.join(path)
+    }
+
+    fn canonicalize(&self, path: &Path) -> Result<PathBuf, FontLoadError> {
+        Ok(path.to_path_buf())
+    }
+}
+


do you even need a file-system backed resolver? this is used in fea-rs because the resolver API is the only API for handling include statements, but norad already has an API for loading from the file system...

cmyr · 2026-03-19T15:01:13Z

src/font.rs

+    fn load_with_resolver_impl(
+        request: DataRequest,
+        resolver: &dyn SourceResolver,
+    ) -> Result<Font, FontLoadError> {


have a separate _impl method that we delegate too is unnecessary here (and is actually unnecessary in our main load fn too now, I believe it's a relic from an earlier time)

cmyr · 2026-03-19T15:21:02Z

src/font.rs

+        request: DataRequest,
+        resolver: &dyn SourceResolver,
+    ) -> Result<Font, FontLoadError> {
+        let metainfo_str = required_file(resolver, Path::new(METAINFO_FILE), None)?;


this required_file method is only called once, i'd just inline that bit?

cmyr · 2026-03-19T15:22:24Z

src/font.rs

+        }
+
+        let mut lib = if request.lib {
+            match optional_file(resolver, Path::new(LIB_FILE), None)? {


I think optional_file can also just be removed, we don't need to worry about canonicalization etc so should just call resolver.get_contents directly?

cmyr · 2026-03-19T15:25:38Z

src/font.rs

+
+        let mut lib = if request.lib {
+            match optional_file(resolver, Path::new(LIB_FILE), None)? {
+                Some(lib_str) => plist::Value::from_reader(Cursor::new(lib_str.as_bytes()))


you shouldn't need Cursor here, just Value::from_reader_xml(lib_str.as_bytes())?

cmyr · 2026-03-19T15:26:11Z

src/font.rs

+
+        let layers = load_layer_set_from_resolver(resolver, &request.layers)?;
+
+        let (groups, kerning) = (groups, kerning);


yanone · 2026-03-23T12:42:02Z

I'm addressing all of your remarks in a new branch. Before making a PR, see diffs here: https://github.com/linebender/norad/compare/master...yanone:norad:non-file-io-save-and-includes?expand=1

This branch is a follow-up over both #387 and #390.

Compared to #387, it keeps the non-file use case but drops the HashMap-specific API shape in favor of closure-based source/sink callbacks over UFO-root-relative paths.

Compared to #390, it removes the public resolver/filesystem-resolver abstraction and moves the non-file code into a separate module. It also generalizes the error path away from io::Error/resolver-specific types.

In addition, it goes beyond both earlier PRs in two ways that matter for my editor use case:

feature file include() support from the same non-file source, while preserving included .fea files structurally instead of flattening them on save
non-file writing APIs for both UFOs and designspace documents

So the current branch is basically: closure-based non-file load/save, isolated implementation, include-aware feature handling, and structural round-tripping of included feature files.

Please review. If you're generally happy with this, I'll make a PR and then we can discuss details.

cmyr · 2026-04-02T16:44:47Z

yan please just open the new PR and I can review there :)

yanone added 2 commits March 12, 2026 14:37

Add source resolver-based UFO loading

055b339

Format source resolver loader code

b56858d

yanone mentioned this pull request Mar 13, 2026

Add in-memory UFO loader and path helpers #387

Closed

cmyr reviewed Mar 19, 2026

View reviewed changes

yanone mentioned this pull request Apr 6, 2026

Non file io save and includes #395

Open


		let layers = load_layer_set_from_resolver(resolver, &request.layers)?;

		let (groups, kerning) = (groups, kerning);

Conversation

yanone commented Mar 13, 2026

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanone commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmyr commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yanone commented Mar 23, 2026 •

edited

Loading