Skip to content
This repository has been archived by the owner on May 20, 2022. It is now read-only.

New table access API. #55

Merged
merged 15 commits into from Nov 5, 2021
Merged

New table access API. #55

merged 15 commits into from Nov 5, 2021

Conversation

cmyr
Copy link
Collaborator

@cmyr cmyr commented Oct 13, 2021

This includes the commit from #54; if we don't want that PR (or want some alternative form) I'll rework this accordingly.

This is the impl of a new API for accessing tables.

I haven't fixed the various other projects & crates yet, because I thought it might be helpful to open this for discussion while I plod through getting CI to pass.

Highlights

  • there's a new TableSet type that coordinates storing and loading tables.
  • tables are lazily loaded on first access, and we use interior mutability so that table getters don't require &mut self.
  • all tables are stored behind shared pointers, so you never need to hold on to a borrow of the TableSet or Font.
  • tables have transparent copy-on-write semantics: after a table has been retrieved from a font, the first time it is mutated we do a deep clone of the source table. (this is maybe a bit controversial, it's a pattern I haven't seen before and is a bit 'magical')
  • this means that you cannot mutate a table "in place"; you need to retrieve it, modify it, and then insert it back into the font.
  • access to known tables is provided via methods on TableSet, with the signature,
    fn GPOS(&self) -> Result<Option<CowPtr<GPOS>>, DeserializationError> { .. }
  • for unknown tables there is still a get method

There are a few rough edges.

  • It's annoying that we need to always return Result<Option<T>>. I've been daydreaming about having two forms of Font, one which has been preloaded and one which hasn't; methods on the former would be infallible, and maybe we could also do things like guarantee that all of the required tables are present? I don't want to worry about this too much right now, but wanted to at least mention it.

  • I'm also not totally sure about the process of mutating tables and mutating fonts; I can imagine a different API where there Font is read-only, and if you want to mutate you have to use an explicit FontBuilder method, and then there would be a to_builder() method on Font for constructing that; you would then add or remove tables as desired before calling a build() method that would give you a new font. This would be slightly more cumbersome but would also be clearer with regards to the fact that if you get a table and then mutate it you need to put it back in the font for anything to change?

Anyway, overall I think this should be quite a bit easier to use.

Simon if you'd like to chat about this at some point let me know!

@simoncozens
Copy link
Owner

Thanks very much for this. It certainly looks a lot tidier. I'm going to work through it, and convert some of the fonttools-cli binaries to the new interface as a way of helping me get a feel for it. The variable font instancer is definitely cleaner, so I'm sure they will be too. (The fact that we have utilities using the library with a variety of degrees of complexity helps us to understand the impact of this kind of change.)

@cmyr
Copy link
Collaborator Author

cmyr commented Oct 13, 2021

I was working through the binaries but it sounds like a good idea to have you take a look at that, I think it will be a good gauge of how successful this API is.

@simoncozens
Copy link
Owner

Thanks very much for this. It is clearly much better!

@simoncozens simoncozens merged commit 7cf7874 into simoncozens:main Nov 5, 2021
@cmyr cmyr deleted the table-api branch November 5, 2021 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants