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

API & documentation cleanup #27

Open
tingerrr opened this issue Oct 26, 2023 · 6 comments
Open

API & documentation cleanup #27

tingerrr opened this issue Oct 26, 2023 · 6 comments

Comments

@tingerrr
Copy link
Contributor

tingerrr commented Oct 26, 2023

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:

  • Module structure
    • it could be a bit deeper to avoid having very long files
  • Documentation
    • small inconsistencies break up the flow when reading
    • required or disallowed attributes are only mentioned here and there
    • the spec version which introduced an attribute is listed for some but not for others
    • Some things are linked directly, while flags are often times simply put into CODE_BLOCKS
  • API
    • lots of code duplication for very simple attributes
      • declarative macros for defining the most common attribute definitions would be a great addition (simple writers or single argument functions)
    • methods which are only available for some subtypes for a writer are hard to find without checking the docs, prefixes like in Feature: Form Field types (PDF 1.7 Section 12.7.4) #23 could help find the right ones quicker

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
Copy link
Contributor Author

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

@tingerrr
Copy link
Contributor Author

tingerrr commented Nov 4, 2023

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
Copy link
Member

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
Copy link
Member

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 #23 could help find the right ones quicker

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

@tingerrr
Copy link
Contributor Author

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
Copy link
Member

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 ...

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

No branches or pull requests

2 participants