Add FontSource trait for source-agnostic UFO loading by cmyr · Pull Request #405 · linebender/norad

cmyr · 2026-05-14T19:42:00Z

Okay so after thinking about this more I realized that supporting this functionality would be a good opportunity for us to also implement #262, by generally decoupling reading from the file-system.

This PR does that; it closes #401.

@yanone sorry for the runaround, but this ultimately feels like the right approach, let me know if it works for your purposes?

RickyDaMa

I really like that you've rewritten the standard Font loading methods to use the new FontSource under the hood.

Have we benchmarked this PR at all to see how it compares for loading speed? Mainly curious

yanone · 2026-05-19T07:55:15Z

@yanone sorry for the runaround, but this ultimately feels like the right approach, let me know if it works for your purposes?

Sure, the direction works for me. What seems to be still missing here is the ability to open .fea includes(). In the end, after writing is supported, I need these to roundtrip cleanly.

Thanks.

cmyr · 2026-06-24T18:02:06Z

okay I think this is ready to go; we can add designspace support afterwards.

@yanone I don't totally understand your issue with FEA includes, could you explain in a bit more detail? fea-rs does have support for handling includes in a way that doesn't require the file system.

It would be helpful for me if we can make sure that this branch will work before I commit to the new API, so if there's anything else you need to make that possible let me know, I have a bit more capacity right now and can be more responsive.

cmyr · 2026-06-24T18:14:35Z

I really like that you've rewritten the standard Font loading methods to use the new FontSource under the hood.

Have we benchmarked this PR at all to see how it compares for loading speed? Mainly curious

no measurable difference in time to load across ~40 test fonts.

RickyDaMa · 2026-06-25T10:14:45Z

+    /// Returns `None` if the file does not exist, `Some(Ok(data))` if the
+    /// file was read successfully, or `Some(Err(..))` if the file exists but
+    /// could not be read.
+    fn try_read(&self, path: &Path) -> Option<Result<Vec<u8>, io::Error>>;


The choice of io::Error throughout this trait feels unintuitive to me considering that the aim of the PR was to support I/O-less workflows. Sure, you can build an io::Error with moreorless what you want, but maybe we should make the error type an associated constant so the implementer can choose it?

i.e.

pub trait FontSource: Sync { type Error; // -- snip -- }

fair.. realistically i'm not sure what the error conditions would be in the wasm case? Moving to an associated type would introduce additional cascading changes, so my inclination right now would be to just leave this as a minor wart, and if it turns out to be an actual ergonomics problem in the wasm case then we can reconsider?

Moving to an associated type would introduce additional cascading changes

Would it? For the existing I/O loading you'd just set the error type to be io::Error and then it's no different to the current state of the PR, but it opens up the flexibility to other implementors.

A lot of the basis for my comments is to try and get this in a good shape for its first version/release because it's easier to make changes while no one is depending on it. Up to you though.

We use FontSource as a dyn trait, and an associated type doesn't work in that context, so it would require some heftier re-architecting.

Realistically when I think about the wasm case, it's genuinely unclear what the error conditions would be? It's easy to imagine the case where a 'file' is missing, but barring that I'm genuinely unsure of the conditions under which a call to this API might fail? Happy for any insight on this..

Yep again a fair point. I would be interested to hear from @yanone about what kind of failure modes we'd be likely to see from the WASM use case, but otherwise yeah I'm now satisfied this is sufficient for an initial release, and would be relatively unlikely to need revisiting

RickyDaMa · 2026-06-25T10:23:52Z

+}
+
+/// A directory on disk implements [`FontSource`] directly.
+impl FontSource for &Path {


We could be really extra/helpful here, and easily update this impl to

impl<T: AsRef<Path>> FontSource for T { // -- snip -- }

Which would cover PathBuf and a bunch of string types too

we already have one blanket impl for the closure case, and adding a second breaks coherence, because 'in theory' you could have the same T that implements both AsRef<Path> and Fn(&Path) -> thing.

Urgh that's annoying. Personally I'd prefer impl<T: AsRef<Path>> FontSource for T over the closure case.

I don't know how often people are going to want to use a closure as their single source of logic for loading a font - it seems like the kind of thing you'd gravitate towards if you're writing example/test code and don't need any state - and even for this crate's own tests you sought additional state/functionality.

If people just want to implement a font loader in a single function they can effectively do that without this impl by making a ZST that implements FontSource. I'd want to optimise the ergonomics for the common case, which I think will still be path-likes even after we support sans-io.

bear in mind that the use of &Path as the impl type here is basically an internal plumbing detail, it isn't surfaced as public API; the public api all takes impl AsRef<Path>.

And although it remains tbd, I do think there's a case that the closure support is genuinely useful API, if (for instance) the caller just has a hashmap of path -> contents?

Ah yes okay I see what you mean, I lost sight of how little the public API touches a FontSource - it's basically only if you want something custom. Sorry! This is fine as-is then

Introduce a FontSource trait that abstracts file access so UFOs can be loaded from any source (directories, zip archives, in-memory stores), not just the filesystem. DirSource provides the filesystem implementation. Font::load_from_source is the new public entry point. Font::load creates a DirSource and delegates to it. All internal loading functions now work through FontSource rather than direct filesystem calls. DataStore/ImageStore use FontSource::as_path() to decide between lazy loading (filesystem-backed, matching the original behavior) and eager loading (non-filesystem sources).

RickyDaMa

Thanks for graciously putting up with all my feedback/questions 🙏

cmyr · 2026-06-29T16:05:48Z

Thanks for the review! will leave this open until @yanone has had a chance to actually try using it, but will merge if there's no problem there.

yanone · 2026-07-02T15:18:16Z

Thank you.

The FontSource trait direction works well for me — it's a cleaner abstraction than the callback-based approach I took in #401, and having the standard Font::load rewired through it is a nice bonus.

For my font-editor use case, though, three things are still missing before this can replace my fork: a write/sink counterpart to FontSource (I need to save UFOs back out without a filesystem, mirroring load_from_source); resolution of .fea include() directives during load — not for compilation (fea-rs handles that), but so norad materializes the included source files into Font::feature_files and they round-trip cleanly on save; and non-filesystem writing for designspace files (reading already works via load_from_reader, but there's no string/sink-based write path). I've implemented all three in #401 and would be happy to port them onto the FontSource foundation here as follow-ups if you're open to it; if now isn't the right time, I'm also fine running a perpetual fork. I need the full functionality either way.

RickyDaMa · 2026-07-02T15:36:21Z

Yeah to me those all seem like reasonable extensions that are compatible with the work on this PR, so we'd be fine to merge this as-is and build more onto it later. It seems like the design is good enough to support the additional requested features down the line.

This was referenced May 14, 2026

Add ufoz support #407

Open

Add tests for no-fs loading #408

Closed

RickyDaMa reviewed May 18, 2026

View reviewed changes

Comment thread src/datastore.rs Outdated

Comment thread src/datastore.rs Outdated

Comment thread src/font_source.rs Outdated

Comment thread src/font_source.rs

Comment thread src/fontinfo.rs Outdated

Comment thread src/error.rs Outdated

cmyr force-pushed the a-general-loading-api branch from 5378f30 to 2dee764 Compare June 24, 2026 18:14

cmyr mentioned this pull request Jun 24, 2026

Add no-fs feature for file-free loading #401

Closed

RickyDaMa reviewed Jun 25, 2026

View reviewed changes

cmyr force-pushed the a-general-loading-api branch from 2dee764 to cc1e973 Compare June 25, 2026 16:55

cmyr added 2 commits June 25, 2026 13:03

Add tests for no-fs loading

2223e37

cmyr force-pushed the a-general-loading-api branch 2 times, most recently from 5b5ee69 to f4737b9 Compare June 25, 2026 17:29

Address review comments

cc6c587

cmyr force-pushed the a-general-loading-api branch from f4737b9 to cc6c587 Compare June 25, 2026 18:34

RickyDaMa approved these changes Jun 26, 2026

View reviewed changes

Uh oh!

Conversation

cmyr commented May 14, 2026

Uh oh!

RickyDaMa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanone commented May 19, 2026

Uh oh!

cmyr commented Jun 24, 2026

Uh oh!

cmyr commented Jun 24, 2026

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

Uh oh!

RickyDaMa left a comment

Choose a reason for hiding this comment

Uh oh!

cmyr commented Jun 29, 2026

Uh oh!

yanone commented Jul 2, 2026

Uh oh!

RickyDaMa commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants