Add sanity check for outlines, add point pen

[madig]

As detailed on https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point.

I'm not quite sure about off-curves follows by a move, so I opened unified-font-object/ufo-spec#148.

Also added a GlyphPointPen for programmatically drawing glyphs, like https://fonttools.readthedocs.io/en/latest/pens/pointPen.html.

[cmyr]

This looks reasonable to me, a few comments inline!

[madig]

There are a bunch of eprintln in use in the parser code. Shouldn't they be the usual ErrorKinds instead?

[cmyr]

Honestly this all looks great to me! A few questions and comments inline, but no major concerns.

[cmyr]

Had an opportunity to give this a bit more thought; some comments inline.

I had an idea for an adjustment to the API, which I am going to call the 'meta-builder pattern'. The basic idea is that some methods on the builder take as arguments closures that are passed another builder that can be used to build the component in question, like a guideline or an anchor. In use this would look something like,

let mut pen = Pen::new("test", GlifVersion::V2);
pen.unicode("x")
    .guideline(|builder| builder.identifier("accent-2").line(Line::Horizontal(840.0)))?
    .outline(|builder| builder.add_point((173.0, 422.0), PointType::Line, false, None, None)
        .add_point((193.0, 300.0), PointType::Line, false, None, None)
        .end_path()
        .add_component("hallo", None, "some-ident"))?;

The signature for guideline would then look something like,

fn guideline(&mut self, build_fn: impl FnMut(&mut GuidelineBuilder)) -> Result<&mut Self, Error> {
    let mut builder = GuidelineBuilder::new();
    build_fn(&mut builder);
    let guildeline = builder.finish()?;
    // insert guideline
    Ok(self)
}

It's very possible this doesn't make sense; in particular there's a sort of funny mismatch between the old API we use for parsing and this new Pen stuff. We could definitely consider rethinking that; one option would be that GlifParser goes away, and all those parse methods become just free functions; instead of mutating the pen inside the GlifParser they would just return the appropriate type, so we'd have things like,

fn parse_outline(reader: &mut Reader<[u8]>, buf: &mut Vec<u8>) -> Result<Outline, Error>;
fn parse_contour(reader: &mut Reader<[u8]>, buf: &mut Vec<u8>) -> Result<Contour, Error>;
// etc

Let me know what you think? This is all just brainstorming, but it feels like you've been fighting a bit to cram the pen into the existing parsing, and maybe it makes sense for it to live outside of that?

[madig]

I like the sub-builder approach and will try to come up with something! I think it may work well enough in GlifParser? I want the best API first, then I can make it work with GlifParser somehow.

What do you mean by funny mismatch? How would free standing parse functions work together?

[madig]

So, I tried the sub-builder approach. It's ok for constructing things directly but clashes with GlifParser. E.g. to have an outline builder, closures are awkward because you ideally do everything at once in the closure, but outline parsing is strewn across 4 functions. I think this needs more thinking and more use-cases to flesh out. Might revisit when I know more.

[madig]

It was suggested to me to call the pen GlyphBuilder instead to avoid confusion with fontTools' Pen concept.

[cmyr]

Did a quick pass, and this seems reasonable. There are definitely some ways I might consider doing this differently, as discussed, but it's hard for me to be helpful without actually trying to play around with the implementation itself, more. Happy to discuss in any case, though!

[cmyr]

as with Identifier, my preference here would be to make illegal values impossible to construct. This could be follow-up work, though.

[madig]

That was my thinking. At which level though? making a special Degrees newtype?

[cmyr]

Happy to get this in as a checkpoint, on the understanding that we're going to keep iterating. :)