Conversation
RickyDaMa
left a comment
RickyDaMa
left a comment
There was a problem hiding this comment.
I think there are significant improvements that we could implement while everything is in flux. Sorry I don't have the time to offer to help beyond these little reviews 🙏
| #[cfg(feature = "ufoz")] | ||
| if metadata.is_file() { |
There was a problem hiding this comment.
Why are we only checking it's a file, not looking at the extension?
If we were ever to add JSON UFO loading, we're going to clash with this
There was a problem hiding this comment.
my thinking is basically: if it's a directory, try to load it as a ufo. If it's a file, try to load it as a ufoz. I believe in the wild I've seen zipped ufos with the .zip extension, and I'm not sure what doing the extension check really buys us?
There was a problem hiding this comment.
Compatibility with e.g. JSONified UFOs or any other single-file UFO formats we may want to support in future. Granted they're not enshrined in the spec the same as I believe ufoz is, but JSONified UFOs are phenomenally useful to work with in build systems as it massively reduces I/O usage while still keeping human readable files for debugging.
There was a problem hiding this comment.
that makes sense, and i'm not opposed to adding json support if it would be genuinely helpful (and it might be worth even proposing as an extension to the spec, if you're finding it has real-world benefits. In any case I propose we punt on this until support for that materializes, and then we can do something fancier here like actually checking for the zip's magic bytes.
| impl ZipSource { | ||
| /// Open a zip archive at the given path and load all file entries into memory. | ||
| pub fn open(path: &Path) -> Result<Self, FontLoadError> { |
There was a problem hiding this comment.
Would it be possible to just decompress what we need on the fly, instead of everything at once ahead of time?
If we store the archive in ZipSource instead of the decompressed entries. I guess the issue is that FontSource doesn't let you mutate self currently, but that could be changed or worked around I guess?
There was a problem hiding this comment.
Yea, so this is possible but we'd either need the trait to take &mut self, or we'd need interior mutability.
Ultimately it comes down to the tradeoffs: doing it all up front increases peak memory usage but is also more efficient, since we don't need to seek around the zip contents and do redundant deflating?
One issue I would point out here though is that the current design means we deflate everything even if we have a sparse DataRequest, and that does smell bad, but it's easy enough for us to thread DataRequest through to ZipSource::open and then avoid that work, so i'll make that change.
5378f30 to
2dee764
Compare
cc1e973 to
5b5ee69
Compare
f4737b9 to
cc6c587
Compare
If passed a file (not a directory) and with the 'ufoz' feature enabled, norad will attempt to treat the file as a zip'd ufo directory.
Add end-to-end integration tests for ufoz loading Verify that loading a UFO from a zip archive via Font::load produces a Font identical to loading the same UFO from a directory, covering the wrapper-dir, __MACOSX, data/images, DataRequest, and error paths.
|
@RickyDaMa if this looks good to you i'll get it in :) |
RickyDaMa
left a comment
RickyDaMa
left a comment
There was a problem hiding this comment.
One final thing that could be nice to do is to add this to our Cargo.toml:
[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]So that the docs are generated for all features, and feature-specific API is labelled with what feature has to be enabled.
It's not the cleanest example because the underlying code is generated, but you can see this in the docs for static-lang-word-lists' constants
- Thread DataRequest through to ZipSource::open, so we don't decompress things we don't need - improve find_zip_root
2 participants
If passed a file (not a directory) and with the 'ufoz' feature enabled, norad will attempt to treat the file as a zip'd ufo directory.