typst / pdf-writer

A step-by-step PDF writer.
Apache License 2.0
481 stars 26 forks source link

API & documentation cleanup #27

Open tingerrr opened 12 months ago

tingerrr commented 12 months ago

In wake of #26 the API would experience a breaking change, this unavoidable if we want to support interactive forms (#25).

If we break the API, we may as well make the best of it and bring some things into unity, there are small differences in the API that I'd like to clean up to provide an overall cleaner and more consistent API to the end user. While also allowing the average contributor to write less code.

Things I had in mind:

I would like to take this issue to discuss changes to the API before the next update, so we can minimize the amount of breaking updates we have by thinking ahead.

tingerrr commented 12 months ago

cc: @laurmaedje @reknih @LaurenzV since you guys are, as it seems, most involved with pdf-writer.

tingerrr commented 11 months ago

During work on #25, I stumbled upon a problem that is relevant for this.

To be able to convert a field to a widget annotation, we must allow a way to construct an annotation from an already created Dict, this is not that hard, the actual problem lies in the current module structure, opening this up means it's possible from any other module inside pdf-writer.

I propose that the module structure should sort-of copy the structure of the PDF 1.7 spec. Related structs like these could then access each other's internals without exposing them to the whole crate:

pub mod interactive {
  pub mod annots {
    pub struct Annotation {
      pub(super) dict: Dict
    }
  }

  pub mod forms {
    pub struct Field {
      pub(super) dict: Dict
    }

    impl Field {
      pub fn merge_annot(self) -> Annotation {
        Annotation { dict: self.dict }
      }
    }
  }
}
laurmaedje commented 11 months ago

While the module structure maybe isn't ideal, I don't see the problem of a pub(crate) field. As long as it's not part of the public API.

laurmaedje commented 11 months ago

The proposed docs changes sound good. I'm also fine with having a few useful macros for generating methods as long as they are declarative macros and not proc-macros. I think these two changes would be a good start and we can then see about the module structure going from there (as this should already shorten the files considerably).

methods which are only available for some subtypes for a writer are hard to find without checking the docs, prefixes like in https://github.com/typst/pdf-writer/pull/23 could help find the right ones quicker

Sounds fine, in principle. Are there many occurances of this beyond annotations?

tingerrr commented 11 months ago

I'm also fine with having a few useful macros for generating methods as long as they are declarative macros and not proc-macros.

That should work just fine, decl macros should suffice here.

Sounds fine, in principle. Are there many occurances of this beyond annotations?

In functions.rs the common_func_methods macro is used to achieve the opposite where the common functions are duplicated on all the subtypes. Which could also be done on #25 before it is merged. But this would mean that, for the sake of consistency, we'd create a type for each annotation subtype. Which could be automated with macros too.

The question is how much type safety we want for a format that is not very type safe in practice. We pass i32 to functions writing lengths and don't validate if they are positive at all, we could simply only allow writing u16 there and cast those to i32, but we don't currently.

I'm personally all for a statically safer API, but we also shouldn't flood API with a million subtypes.

laurmaedje commented 11 months ago

I don't have a strong opinion either way, but I do think that consistency's important here. So either annotations or functions should change.

Type safety is nice, but in the end there's just too many ways to write an invalid PDF ...