search

linebender / norad

Rust crate for working with Unified Font Object files
Apache License 2.0
43 stars 12 forks source link

Deserialize designspace lib fields #311

Closed cmyr closed 1 year ago

cmyr commented 1 year ago

This was about as painful as expected, and requires us to manually glue together the xml data model with the plist model, using serde.

To briefly summarize the problem: XML data, by definition, does not have any defined semantics. Semantics are added on top of XML on a per-case basis, by indicating the document type inline (https://en.wikipedia.org/wiki/Document_type_declaration).

The plist crate does implement derive for plist types, but they are based on the assumption that type information is being provided by some deserializer (e.g. for a specific file format). This is the right thing for the plist crate to do. Unfortunately quick_xml cannot provide this type information, since quick_xml does not know that we are deserializing a plist.

To make this all work, I have added a bunch of manual deserialize code. This code will only work when deserializing from XML; if in the future we wish to serialize to other formats, we will need to revise it.

Some notes:

RickyDaMa commented 1 year ago

Wow this is insane, thanks for implementing this! I'll do a more thorough review when I have time (Friday or next week). I'll probably end up implementing serialisation as part of #309

cmyr commented 1 year ago

yea, serialization will likely be annoying too, but I'm not sure exactly how annoying. I guess we'll find out? 😬

cmyr commented 1 year ago

Huh interesting, I honestly kind of appreciate the compactness of your version, maybe we should take that instead? I'm not even sure there's much performance difference to be honest. Want to PR it, maybe including the tests from this branch?

cmyr commented 1 year ago

Although thinking about it a bit more, you are going to need more logic to handle date/data/integer, at least.

edit:

Actually now that I think about it I'm not sure your version is working as expected. Maybe try moving over the one big catch-all test I have, and see what happens?

RickyDaMa commented 1 year ago

Yeah mine wasn't working fully, I stopped investing too much time in it and reached out in the Google Chat, at which point you went crazy 😁 The main hitch I'm having is nested dictionaries, I'm getting cryptic serde errors no matter what I try it feels like. If you think it's worth trying to bring "up to spec" I can keep working on it.

Ultimately it's a trade-off of brevity/clarity for efficiency

cmyr commented 1 year ago

Thinking about this more I suspect that your approach just may not be tractable, because XML alone doesn't provide enough structural information to identify the appropriate types? I'm not positive, but if we're happy enough with my version then I'm okay with merging this as-is.

cmyr commented 1 year ago

Okay I've talked myself into thinking this is the right approach, will merge and then you can use this to figure out how to save? Happy to help if you get stuck.