Repository navigation
feat(rectangle): add width and height helpers - #317
Conversation
|
@PrinsFrank ready for review. |
There was a problem hiding this comment.
Hey, thanks for the PR and the effort you've put into this!
I think the implementation is a bit too focussed on a specific use case, but we can get this to be generic! It looks like your end goal is to get the crop box of a page, and if it doesn't exist to get the mediabox.
In the end, the code here mostly boils down to three things:
- Traverse parents for inheritable keys
- Resolve values for references by retrieving the referenced object and resolving their type.
- Retrieving width/height for Rectangles
All three are now implemented with a specific use case in mind, but are patterns that could be reused when we implement them according to spec. I think both would be great additions to this library, solving these problems generically, and all three would qualify for their own PRs.
Thanks again for the PR, and seeing the way you implemented this uncovered some great insights!
What do you think?
|
I refactored the PR to make those parts generic rather than page-specific:
Page-level code now only consumes dictionary-level APIs for the I also moved the dimension tests to align with your testing feedback. If you prefer, these changes could be split into smaller PRs. Since this PR was already open and under review, I focused the changes on addressing your comments with the smallest coherent end-to-end update. If you prefer, I can split follow-up refinements into separate PRs after this one. |
|
I also changed the PR description after the review. |
PrinsFrank
left a comment
There was a problem hiding this comment.
Hi,
I only have a few hours per evening to work on this library. Looking at this PR, I think it warrants some rearchitecturing of the Dictionary class and how to traverse parent/child dictionaries. I won't be able to answer that question tonight.
Just to get some context: How many files do you have were you need to traverse Dictionaries for Cropboxes/MediaBoxes to get a definitive answer?
d50f553 to
d9172c1
Compare
Good question, but after your implementation, rearchitecturing |
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
d9172c1 to
a4a101d
Compare
PrinsFrank
left a comment
There was a problem hiding this comment.
Thank you for the contribution! I'll release a new version including this change soon!
Summary
Adds helper methods to compute rectangle width and height directly from rectangle coordinates.
Usage