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

Implement full user name to file name conversion #251

Merged
merged 5 commits into from
Feb 7, 2022
Merged

Conversation

madig
Copy link
Collaborator

@madig madig commented Jan 19, 2022

Closes #85
Closes #252

Alright, this took me a week to implement.

  1. Name::is_valid must check chars() instead of bytes(), because the spec talks about "Any character with a Unicode value in the ranges [...]". E.g. the name "hello💝" encoded as bytes happens to include a banned control character but is a valid name.
  2. user_name_to_file_name now...
    • takes a &Name, which guarantees some properties relevant for file name generation already
    • ensures a file name does not end with a period or space, as that's disallowed on Windows
    • ensures a file name is not using a Windows-reserved stem like "con", but unlike the official spec example does not replace more than necessary ("con.txt" must be turned into "_con.txt" but "hello.con.txt" is fine)
    • ensures a file name is unique among a case-insensitive set of existing file names. Or it panics after 99 attempts.
    • tries to not do something stupid if some form of usize over- or underflow occurs, e.g. by one of name, prefix or suffix being pathologically long. I'm not too confident about it though... I should probably fuzz it?
    • returns a PathBuf, because that's what's needed down the code chain. This allows me to simplify Layer::new and streamline its callers.
  3. LayerSet and Layer are now responsible for providing user_name_to_file_name with a set of lowercased strings of existing file names for clash checking.
  4. Layer::new now expects callers to do the Name instantiation work up front.

I think clashes are very unlikely, so this does feel a bit excessive... but I also can't rule them out completely. At least, I only try one strategy to resolve clashes instead of the two in the UFO spec example.

There is a discussion at unified-font-object/ufo-spec#164 about a different and better file naming scheme, but so far without a definitive conclusion.

I need to look more closely at Font::save() to see how to implement erroring out when a file should be unique but the filesystem thinks differently. Should Glyph::save_with_options() error out if the destination exists already?

Also, random observation: do we need to change or regenerate file names when removing/renaming glyphs? Doesn't seem like it.

@linebender linebender deleted a comment from github-actions bot Jan 20, 2022
@linebender linebender deleted a comment from github-actions bot Jan 21, 2022
@linebender linebender deleted a comment from github-actions bot Jan 21, 2022
@madig madig marked this pull request as ready for review January 21, 2022 16:25
@linebender linebender deleted a comment from github-actions bot Jan 21, 2022
@linebender linebender deleted a comment from github-actions bot Jan 21, 2022
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is certainly something, huh.

some thoughts inline: 10^15 is definitely way more checks than could possibly make sense, but whatever.

More importantly, I think constructing the set of paths each time is a mistake; we should just maintain a set of normalized paths, similarly to how we maintain a set of names.

src/layer.rs Show resolved Hide resolved
src/name.rs Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@madig
Copy link
Collaborator Author

madig commented Jan 24, 2022

I ended up tackling #253 in the last commit.

@linebender linebender deleted a comment from github-actions bot Jan 24, 2022
@linebender linebender deleted a comment from github-actions bot Jan 24, 2022
@madig
Copy link
Collaborator Author

madig commented Jan 24, 2022

Needs to be rebased on top of #254 if that goes through :)

Base automatically changed from fontinfo-check-postscript-pairs to master January 25, 2022 15:51
@madig madig changed the base branch from master to fix-layer-renaming January 25, 2022 19:15
@linebender linebender deleted a comment from github-actions bot Jan 25, 2022
Base automatically changed from fix-layer-renaming to master January 26, 2022 12:31
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay a few more little comments inline but I think this is close. Thanks!

src/layer.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@madig
Copy link
Collaborator Author

madig commented Jan 27, 2022

I noticed that inserting something to escape a reserved name could push the string over 255 characters. Maybe I need to do the escaping before.

@linebender linebender deleted a comment from github-actions bot Jan 27, 2022
@linebender linebender deleted a comment from github-actions bot Jan 27, 2022
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing ebb1fd5 against 3444ad3

target old size new size difference
target/release/examples/load_save 1.9 MB 1.93 MB 33.96 KB (1.74%)
target/debug/examples/load_save 8.6 MB 8.64 MB 42.88 KB (0.49%)

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@madig madig merged commit bc568b3 into master Feb 7, 2022
@madig madig deleted the full-filenames branch February 7, 2022 21:09
@madig
Copy link
Collaborator Author

madig commented Feb 7, 2022

One day I must fuzz this 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants