Bug 105692

The behavior was added by Fabio D'Urso - Fabio, are you still around (or does anybody know)? It would be great to learn about your intents regarding the commits below:

Commit 7684c325, "AnnotText: Always force 24x24 size with custom stamps, not only on first rendering"

Commit c6296cd8, "Do not trust the rect of AnnotLine, AnnotPolygon and AnnotInk when drawing"

Commit 3d4985f1, "Do not trust the rect of AnnotTextMarkup when drawing"

Created attachment 138316 [details] [review]
Apply template method design pattern to Annot::draw

Annot::draw becomes a template method. Goal is to make the code more schematic and understandable, decrease redundancy and gain places where we can hook in future changes. I loosely followed the GoF description.

Note: This patch doesn't change any public behavior. It just prepares the ground for other things I plan to do. If you like it, you can use it standalone, no matter what happens to the bug here in #105692. I'll attach separate patches later that actually deal with the bug.

Created attachment 138340 [details] [review]
Apply template method design pattern to Annot::draw (Rev 2)

Rebase on master (5e7aef9). And, no need to have public template primitives, make them private/protected.

Btw., is this the correct way to handle continuous rebasing or other minor changes for a patch?

Created attachment 138341 [details] [review]
Always respect Annot Rect for AnnotText icons

(2nd part of patch series - based on https://bugs.freedesktop.org/attachment.cgi?id=138340)

Instead of forcing 24x24, always use the size of Annot Rect when drawing icons in user space (even on first rendering - to rephrase Fabios commit message from 7684c325). It fixes the most important part of #105692, imho.

For our hardcoded icons we only need 24x24 while we're in appearance XObject coordinate system. For user space, Gfx::drawAnnot does the transformation (translate + scale) to whatever size and position is in Annot Rect.

Note: For the same reason the AP BBox can start at (0,0). We don't need an offset. I changed the AP BBox to 0, 0, 24, 24. That's simpler, and other embedded appearance streams I've seen from other PDF writers do it that way, too.

Is this ready to get reviewed or are we still waiting on Leonard's answer on the question you made on the mailing list?

Patch 1 "Apply template method design pattern to Annot::draw" is ready.

For Patches 2..x (some will follow) I'd say wait a bit.

There seem to exist alternate solutions. One is to always respect the original Rect, as it's done by the 2nd patch. Another option would be to temporary reset Rect to whatever poppler likes, but then propagate the new size to clients in QT Annotatin::boundary() and glib poppler_annot_get_rectangle(). Leonard will tell which options are PDF compliant. And we have to judge what's best from clients perspective. Hopefully guys from evince or other projects will comment here too.

So you removed

-void AnnotLink::draw(Gfx *gfx, GBool printing) {
-  if (!isVisible (printing))
-    return;
-
-  annotLocker();
-  // draw the appearance stream
-  Object obj = appearance.fetch(gfx->getXRef());
-  gfx->drawAnnot(&obj, border, color,
-		 rect->x1, rect->y1, rect->x2, rect->y2, getRotation());
-}


meaning it'll use Annot::draw which ignores the border.

That seems like a bug? Or am i reading it wrong?

> meaning it'll use Annot::draw which ignores the border.
> 
> That seems like a bug? Or am i reading it wrong?

Indeed, good catch! I'll provide an override AnnotLink::drawAppearance, which can care for the border.

Created attachment 138638 [details] [review]
Apply template method design pattern to Annot::draw (Rev 3)

Don't forget border when drawing AnnotLink. Rebase on master 0a79dc3.

Can you please rebase it again?

Sorry ^_^

Created attachment 138645 [details] [review]
Apply template method design pattern to Annot::draw (Rev 4)

Continuous rebasing ¯\_(ツ)_/¯ I like that AnnotAppearanceBuilder...

(In reply to Tobias Deiminger from comment #11)
> Created attachment 138645 [details] [review] [review]
> Apply template method design pattern to Annot::draw (Rev 4)
> 
> Continuous rebasing ¯\_(ツ)_/¯ I like that AnnotAppearanceBuilder...

I'm a bit conflicted with this patch.

I see it adds some structure, that is always nice, on the other hand it doesn't really save that many lines as shown by the base implementations (that is what we end up reusing) are basically one line each.

Also the new structure makes you add the fontResourceDict which is a bit harder to manage than before since now it spans two methods instead of one.

On the other hand now we don't create the fonts dict on every draw that is a good thing too (not that it's a hot path but all speedups are good)

Let me sleep over it :D

Comments either from you or from anyone else that add pros for this patch are also welcome :)

(In reply to Albert Astals Cid from comment #12)
> Comments either from you or from anyone else that add pros for this patch
> are also welcome :)
The size fix (not submitted yet) for the remaining types like AnnotLine and AnnotPolygonfrom will probably look something like this:
 void Annot::getRect(double *x1, double *y1, double *x2, double *y2) const {
-  *x1 = rect->x1;
-  *y1 = rect->y1;
-  *x2 = rect->x2;
-  *y2 = rect->y2;
+  PDFRectangle drawRect = getDrawRect();
+  *x1 = drawRect.x1;
+  *y1 = drawRect.y1;
+  *x2 = drawRect.x2;
+  *y2 = drawRect.y2;
 }

I.e. always report the actually drawn size instead of the size stored in /Rect, because it can differ in corner cases and the drawn size is what's more important to API clients. That measure is only possible if there exists that getDrawRect method from attachment 138645 [details] [review]. That would be a (however still fictional) pro from my point of view.

(In reply to Tobias Deiminger from comment #13)
> (In reply to Albert Astals Cid from comment #12)
> > Comments either from you or from anyone else that add pros for this patch
> > are also welcome :)
> The size fix (not submitted yet) for the remaining types like AnnotLine and
> AnnotPolygonfrom will probably look something like this:
>  void Annot::getRect(double *x1, double *y1, double *x2, double *y2) const {
> -  *x1 = rect->x1;
> -  *y1 = rect->y1;
> -  *x2 = rect->x2;
> -  *y2 = rect->y2;
> +  PDFRectangle drawRect = getDrawRect();
> +  *x1 = drawRect.x1;
> +  *y1 = drawRect.y1;
> +  *x2 = drawRect.x2;
> +  *y2 = drawRect.y2;
>  }
> 
> I.e. always report the actually drawn size instead of the size stored in
> /Rect, because it can differ in corner cases and the drawn size is what's
> more important to API clients. That measure is only possible if there exists
> that getDrawRect method from attachment 138645 [details] [review] [review]. That
> would be a (however still fictional) pro from my point of view.

Thinking about this, i really think if we're going to go that route, it's better to kill appearBBox as a member, just use it to when we're creating the new appearance and then adjust the rect member.

I don't like having the getrect function not really returning the rect member, i've looked at the code and seems we really use rect for drawing and geometrical stuff (i.e. inRect) so it should be fine updating it in my opinion.

(In reply to Albert Astals Cid from comment #14)
> Thinking about this, i really think if we're going to go that route, it's
> better to kill appearBBox as a member, just use it to when we're creating
> the new appearance and then adjust the rect member.
Yeah, I thought about that already. I hesitated a bit, maybe you can help. It's this: We can't use Annot::setRect to adjust the rect member, because this (a) also creates an adjusted annotObj["Rect"] Object, and (b) calls invalidateAppearance.
(a) would cause the size change to be saved into the document which I think would be an unexpected side effect
(b) would invalidate our just created appearance, so we never get an valid appearance

If we instead bypass Annot::setRect on appearance creation and update Annot::rect directly, it could work. But I have not yet checked for side effects of Annot::rect != Annot::annotObj["Rect"]. If there's no side effect then we should be able to do what you suggested and discard both the Annot::appearBBox member and the Annot::getDrawRect method and as a bonus things are simplified a bit.

> I don't like having the getrect function not really returning the rect
> member, i've looked at the code and seems we really use rect for drawing and
> geometrical stuff (i.e. inRect) so it should be fine updating it in my
> opinion.
In places such as inRect we would have to use getDrawRect() instead of Annot::rect too, to be consistent. But if your suggestion above works, we don't have to bother anyway.

Created attachment 138666 [details] [review]
Alternate all-in-one patch

This patch tries what was discussed in comment 14 and comment 15:
- Get rid of Annot::appearBBox member, local AnnotAppearanceBBox'es are enough.
- Get rid of Annot::getDrawRect again, no longer needed because Annot::rect always equals draw size.
- Keep the template pattern approach (just my own decision atm)
- Fix AnnotLine, AnnotTextMarkup, AnnotPolygon, AnnotInk by adjusting Annot::rect (but not annotObj["Rect"]!) to generated appearance. See also [0].
- Fix AnnotText by scaling default appearance (24x24) to size of annotObj["Rect"].

I did some first tests with Okular and it worked as expected. Albert, what do you think, can we proceed this way?

Based on master a8e93f4.

[0] If I got it right, Acrobat/Reader handles the case the same. See https://lists.freedesktop.org/archives/poppler/2012-April/008956.html ("[..] generate a new bounding box that incorporates the entire line as that is what one would expect [...]")

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/379.