Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying whitespace on save #148

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Allow specifying whitespace on save #148

merged 1 commit into from
Aug 5, 2021

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Jul 29, 2021

This lets the user optionally customize the whitespace used during
serialization.

The implementation is slightly annoying. the quick_xml and rust-plist
crates specify whitespace in different ways; quick_xml takes a (u8,
usize) pair (the byte and its count) and rust-plist takes a
Cow<'static str>. Additionally, in our own glyph serialization code we
need to write whitespace directly when appending the 'lib' section.

Anyway: you create a WriteOptions object by passing it a string, and
then we figure out what the appropriate (u8, usize) pair is. The reason
we want to think about this is because eventually writing out a UFO
should be parallelized, and that could mean cloning WriteOptions once
for each glyph; we would like to make sure this isn't allocating.

So this seems to work, even if some of the internals are a bit gross.


this is based off of #147, which should go in first.

@cmyr cmyr force-pushed the update-plist branch 3 times, most recently from 36599c5 to 7249946 Compare August 4, 2021 19:25
Base automatically changed from update-plist to master August 4, 2021 20:17
@github-actions
Copy link

github-actions bot commented Aug 4, 2021

🗜 Bloat check ⚖️

Comparing 7c97df8 against f1a82f2

target old size new size difference
target/release/examples/load_save 1.76 MB 1.75 MB -15.52 KB (-0.86%)
target/debug/examples/load_save 8.17 MB 8.17 MB -776 Bytes (-0.01%)

@linebender linebender deleted a comment from github-actions bot Aug 4, 2021
@linebender linebender deleted a comment from github-actions bot Aug 4, 2021
@linebender linebender deleted a comment from github-actions bot Aug 4, 2021
@chrissimpkins
Copy link
Collaborator

@cmyr are you actively working on XML declaration single quote support? If not, I will have a shot at it based on the changes in this PR

This lets the user optionally customize the whitespace used during
serialization.

The implementation is slightly annoying. the `quick_xml` and `rust-plist`
crates specify whitespace in different ways; `quick_xml` takes a (u8,
usize) pair (the byte and its count) and `rust-plist` takes a
`Cow<'static str>`. Additionally, in our own glyph serialization code we
need to write whitespace directly when appending the 'lib' section.

Anyway: you *create* a `WriteOptions` object by passing it a string, and
then we figure out what the appropriate (u8, usize) pair is. The reason
we want to think about this is because eventually writing out a UFO
should be parallelized, and that could mean cloning `WriteOptions` once
for each glyph; we would like to make sure this isn't allocating.

So this seems to work, even if some of the internals are a bit gross.
@cmyr
Copy link
Member Author

cmyr commented Aug 5, 2021

@chrissimpkins I took a peek haven't implemented anything, but let's merge this first, so we can use WriteOptions.

I also think that it's worth being slightly fancier and doing the replacement before the initial write, it just feels more principled, and will be more portable (the write-then-rewrite approach may be slower on older storage media, or on other platforms.)

@cmyr
Copy link
Member Author

cmyr commented Aug 5, 2021

@chrissimpkins to elaborate on what I would propose:

  • add a quote_char method to WriteOptions
  • When serializing the .glifs, (which we serialize manually) write the header to the stream ourselves, using the specified character
  • When serializing plists, we don't control writing the header, so we'll have to rewrite. There are options here. All plist writing is now handled through a method in write.rs.
    • One option is to write to disk, and then use the unix/windows specific methods to go and overwrite the header in place, before closing the file handle: see std::os::unix::fs::FileExt and std::os::windows::fs::FileExt; you'll need slightly different impls on either platform.
    • the other option would be to have a custom Write type, that would wrap or replace the BufWriter; this would intercept the header and replace it, if needed, otherwise forwarding the bytes to the inner writer.

Thinking about this, I think the first option is totally reasonable. The second option is probably marginally more 'efficient', but I don't think it matters; the main thing we want to avoid is closing and reopening all the file handles.

edit: broke this out into an issue, #156

@cmyr cmyr merged commit fce1673 into master Aug 5, 2021
@cmyr cmyr deleted the custom-whitespace branch August 5, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants