Complete artifact types#273
Conversation
|
Maybe we could again add a small test case that uses a subtype? |
|
|
|
Both done. I was unsure whether the rectangle need to be transformed in any way. |
|
Yes it needs to be transformed, should be the same as here: |
|
I'm afraid that Anyways, this PR does not block Typst 0.14 |
|
Oh, maybe it doesn't need any transform, since the page root transform and other transforms are already applied to the surface? |
|
I'm pretty sure it doesn't need to be transformed, but since it uses the same |
43e29f3 to
e25ee37
Compare
Done in 3fb3d28 |
LaurenzV
left a comment
There was a problem hiding this comment.
Overall LGTM, but we will have to do some more research on the whole bbox thing and then figure out how to best document how the bbox should be determined.
Unfortunately I don't have Acrobat Pro, so if someone else has it might be useful to test how Acrobat interprets the bbox attributes, especially if there are CTM operations in the main page stream.
| >> | ||
| stream | ||
| /Artifact << | ||
| /BBox [0 80 200 130] |
There was a problem hiding this comment.
Is this really the correct bbox? If I understood correctly, we need to assume the default user space (i.e. where in PDF Coordinates it will actually end up being). Since the text is written at the top but the PDF coordinate system is y-down, shouldn't the y be something like 600 or 700 instead?
There was a problem hiding this comment.
I couldn't find a way to verify this using Acrobat or other tools, but I agree.
| /// structure elements, this property list entry is specified with regards | ||
| /// to the page transform, not the [`Surface`](crate::surface::Surface) |
There was a problem hiding this comment.
Are you sure about this? Does that mean you think it should only take the y-flip into account? I don't see why this would be the case, given the description in the spec.
There was a problem hiding this comment.
No, you're right. I guess both the comment here and for the artifact could briefly state that the Rect is specified within the page transform.
| /// The bounding box of a tag that encloses its visible content. | ||
| /// If the content spans multiple pages, this should be omitted. | ||
| /// | ||
| /// Unlike the property list entry `BBox` on [artifacts](crate::tagging::Artifact), |
There was a problem hiding this comment.
I'm sorry, I don't know how I got there. I've read through the spec again and that indeed doesn't make any sense.
Regarding the implementation, we could get the page transform from the root_transform of the Surface::root_builder in Surface::start_tagged and pass it through ContentBuilder::start_marked_content_with_properties.
Another idea I had, though that would be a larger change, would be to have a SurfaceKind with a Page variant that contains the page dimensions.
In the future we could also add a mutable borrow of the page annotations, so annotations could be added through the Surface which would come in handy when constructing a TagTree.
There was a problem hiding this comment.
No worries! The only problem is that I've tried my best to abstract away the y-flip transform needed for PDF so that the user can always just assume a y-down coordinate system, it would be unfortunate if the user somehow has to take it into account here. Therefore, I think the best thing to do is to simply document that the bbox needs to cover all shapes in user space, but still allow the user to assume that user space is a y-down coorindate system. Then, internally we apply the y-flip transform so that it's converted to y-up, but this is hidden away from the user. WDYT?
There was a problem hiding this comment.
I agree, the Y-flip should definitely stay hidden away, that's what I meant to say :)
That would require having access the page height to compute the transform right?
The implementation ideas were just two options I could think of to achieve that.
There was a problem hiding this comment.
Yep, but I’m pretty sure this should already exist somewhere in the code? I think we have something similar for annotation rects.
There was a problem hiding this comment.
Ah, but the difference is that those are written directly into the content stream… so yeah if there is an easy way to wire the page height through, that’s what we should do. I don’t currently remember if the automatic page size might cause problems here…
There was a problem hiding this comment.
Is there a way to specify an automatic page size? Seems to me like the page size must be specified when creating a page 🤔
The surface also already uses the page height from the page settings when creating the root content builder, so that would match the behavior
There was a problem hiding this comment.
Then I must remember wrong! I’m sure there was this functionality in the past but I might have removed it already. 🤔
This commit adds the PDF 1.7 artifact type `Background`, the PDF 1.7 pagination artifact subtype `Watermark`, and the PDF 2.0 pagination artifact subtypes `PageNum`, `LineNum`, `Redaction`, `Bates`. Finally, the variant `PaginationOther` allows writing a pagination artifact without a subtype. Because these new variants are not compatible with PDF 1.4, where the mechanism was introduced, a compatibility layer has been introduced.
|
LGTM, thanks! |
| tag.write_properties(sc, properties); | ||
| // Page height extracted from transform and passed to function to allow | ||
| // its dependants to flip the y-axis, mirroring Krilla conventions. | ||
| let page_height = self.root_transform.ty(); |
There was a problem hiding this comment.
Is this really correct? This function will be called on the last sub_builder, which doesn't necessarily have the page root transform right?
I think to be correct we should use the root_transform of the root_builder inside Surface::start_tagged.
Also instead of extracting the page_height we could just pass through the entire transform.


This commit adds the PDF 1.7 artifact type
Background, the PDF 1.7 pagination artifact subtypeWatermark, and the PDF 2.0 pagination artifact subtypesPageNum,LineNum,Redaction,Bates. Finally, the variantPaginationOtherallows writing a pagination artifact without a subtype.Because these new variants are not compatible with PDF 1.4, where the mechanism was introduced, a compatibility layer has been introduced.