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

Glyph: get Vecs out of Options? #92

Closed
madig opened this issue Mar 24, 2021 · 6 comments · Fixed by #93
Closed

Glyph: get Vecs out of Options? #92

madig opened this issue Mar 24, 2021 · 6 comments · Fixed by #93

Comments

@madig
Copy link
Collaborator

madig commented Mar 24, 2021

Currently, contours, components, anchors, codepoints and guidelines are Vecs hidden behind an Option, which seems to be because they allow for non-existence of attributes in a glif file. This leads to code like

fn move_glyph(glyph: &mut Glyph, delta: f32) {
    let Some(outline) = &mut glyph.outline {
        // ...
    }
}

which I don't find especially, uh, ergonomic? Same goes for anchors, etc. I'd rather have a

pub struct Glyph {
    pub name: GlyphName,
    pub format: GlifVersion,
    pub advance: Option<Advance>,
    pub codepoints: Vec<char>,
    pub note: Option<String>,
    pub guidelines: Vec<Guideline>,
    pub anchors: Vec<Anchor>,
    pub contours: Vec<Contour>,
    pub components: Vec<Component>,
    pub image: Option<Image>,
    pub lib: Option<Plist>,
}

Since empty Vecs can just not be serialized (and don't alloc until they have one member), I don't think making them optional holds semantic value? Same for lib maybe?

Related to #27.

@cmyr
Copy link
Member

cmyr commented Mar 24, 2021

Yes, it's gross. my intention was to losslessly adhere to the UFO spec, and represent all fields that were optional as Option<_> on the rust code. I'm not sure if this matters nor not? The way that it might matter is for round-tripping, if we wanted to preserve empty collections or something.

I defer to you, here; if you think it makes sense to use an empty vec to represent a missing field, and not serialize empty vecs, that sounds good to me.

@madig
Copy link
Collaborator Author

madig commented Mar 24, 2021

FWIW, ufoLib2 and defcon use empty lists for the above.

@cmyr
Copy link
Member

cmyr commented Mar 24, 2021

Happy to see this change, it will be a nice ergonomics improvement.

@madig
Copy link
Collaborator Author

madig commented Mar 25, 2021

I just noticed: in the same vein, advance width and height are kept behind Option. The spec says https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#advance, i.e. both could be regular fields defaulting to 0. There has been some thinking about changing 0 though: unified-font-object/ufo-spec#95 -- but @anthrotype decided to not go ahead with it?

@cmyr
Copy link
Member

cmyr commented Mar 26, 2021

this also sounds good, the only reason these various things are awkward is because i did not feel knowledgeable enough to take any liberties.

@madig
Copy link
Collaborator Author

madig commented Mar 26, 2021

Good thing you now have extreme prejudice on your team.

@madig madig mentioned this issue Mar 27, 2021
@madig madig closed this as completed in #93 Mar 30, 2021
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 a pull request may close this issue.

2 participants