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

Improve plist handling #62

Open
3 tasks
cmyr opened this issue Nov 30, 2020 · 3 comments
Open
3 tasks

Improve plist handling #62

cmyr opened this issue Nov 30, 2020 · 3 comments
Labels
enhancement New feature or request

Comments

@cmyr
Copy link
Member

cmyr commented Nov 30, 2020

This has been discussed but feels worth having a tracking issue.

There are a few different things going on here. I'll quote @madig on zulip:

@cmyr thinking about better (embedded) plist handling, what we need is

something that reads an entire file like fontinfo.plist, which has a rigidly defined key-value (type) schema, basically what serde excels at
something that handles kerning.plist and groups.plist which both have a defined structure but nothing else, like a simple special case of the fontinfo case?
something that handles lib.plist, which is a standalone, free-form plist file
something that handles embedded plists in .glif and layercontents.plist files
number 1 needs something that associates keys with types, number 2 is a basic parse-into-hashmap scenario and 3 and 4 need just be dictionaries that can take arbitrary plist key types plus values

  • fontinfo.plist: The problem with fontinfo.plist is that it's huge, and has tons of optional keys, and so using serde to create a struct ends up both generating a lot of code and also creating this huge struct that must also be impacting binary size. It would be nice to replace this with some sort of typed dictionary, kind of like druid's Env.
  • lib.plist: this probably doesn't need to change, and should just be a plist object. We might also want to expose API that lets people try and deserialize this to some concrete struct, if we want?
  • embedded plists: this is mostly a matter of passing some portion of the xml off to a plist parser to generate a plist object, and then doing that in reverse when we serialize. It might also be nice to let the user provide some custom serdeable type here, but it will be hard to make that play nice in the type system.

This all feels very doable; I'm going to start by testing out the tweaks to fontinfo, and seeing how that feels.

@cmyr cmyr added the enhancement New feature or request label Nov 30, 2020
cmyr added a commit that referenced this issue Dec 1, 2020
cmyr added a commit that referenced this issue Dec 4, 2020
@madig
Copy link
Collaborator

madig commented Dec 26, 2020

Issue unified-font-object/ufo-spec#128 tells me that fontinfo should really be treated as a grab bag of stuff with prescribed value types and validators for certain keys, like an enforcement and validation structure sitting on top of a bare plist.

Not sure how to handle getting data in and out of it though, i.e. imagine being a UFO compiler and querying e.g. familyName and italicAngle. If the enforcement structure provides e.g. get_family_name and get_italic_angle, you can bake the return type into it. If you're doing something where you're iterating over all keys and values, you'll probably need to do plist's as_string, as_real dance.

@madig
Copy link
Collaborator

madig commented Jan 11, 2022

An additional point is that serde errors apparently aren't chained, so you'll get e.g. the one shown in #245 (comment). I don't know if you can make serde give more context, but it's something that should be tackled if we handle fontinfo differently.

@madig
Copy link
Collaborator

madig commented Jan 12, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants