[poppler] Annot borders

[Albert Astals Cid]

El Dimecres, 27 de novembre de 2013, a les 08:51:42, Carlos Garcia Campos va 
escriure:
> Albert Astals Cid <aacid at kde.org> writes:
> > El Dimarts, 26 de novembre de 2013, a les 21:11:11, Carlos Garcia Campos
> > va
> > 
> > escriure:
> >> I've noticed some small issues in the way we handle the border of
> >> annots compared to what the spec says and what acroread does. Attached
> >> are 4 patches:
> >> 
> >> [PATCH 1/4] annots: Remove unused AnnotBorderType from AnnotBorder
> >> 
> >> This is simply a cleanup, we have a base class AnnotBorder that allows
> >> to create instances of it with the unknown type. It doesn't make much
> >> sense and we are not using it at all, so I've removed the
> >> AnnotBorderType, and made the constructor protected.
> >> 
> >> [PATCH 2/4] annots: Add helper function Annot::setLineStyleForBorder
> >> 
> >> The code to set the line width and dash was duplicated in several draw()
> >> methods, so I've moved it to a helper function. This patch also changes
> >> the way we are using it. We were always ignoring the border when the
> >> border with was 0, but it doesn't seem to be always correct. The PDF
> >> spec says that in both Border and BS entries when the width is 0, no
> >> border should be drawn at all. But some annotations like lines,
> >> geometry, polygons, etc. don't use the border entry to actually draw a
> >> border, but to set the line with and dash for the stroke operations. For
> >> example, we were not drawing lines for annots with no border or with a
> >> border width = 0, but acroread does, because in this case the border
> >> entry is used to set the line with and dash pattern, and the PDF spec
> >> also says that a line width of 0 should be drawn as the thinnest line
> >> that
> >> can be rendered at device resolution: 1 device pixel wide.
> >> So, for FreeText annotations where we actually draw a border, we only
> >> call Annot::setLineStyleForBorder if we have a border and the width is
> >> greater than 0, and in all other cases we always call it when we have a
> >> border. Of course, this only affects annotations not having an AP entry.
> >> 
> >> [PATCH 3/4] annots: Use a default border for annots that can have a BS
> >> entry
> >> 
> >> According to the PDF spec if neither the Border nor the BS entry is
> >> present, the border shall be drawn as a solid line with a width of 1
> >> point. But again, acroread seems to only apply this rule for annotations
> >> that can have a BS entry. This patch moves the parsing of the BS entry
> >> From the base Annot class to the specific annot classes that can have a
> >> BS entry, and it creates a default border object when neither Border nor
> >> Bs is present. It also removes the border passed to Gfx::drawAnnot() in
> >> AnnotFileAttachment::draw and AnnotSound::draw because acroread ignores
> >> the Border entry also for annots that can't have a BS entry.
> >> This ensures that we always draw line, geometry, polygon, etc, even if
> >> there's no border specified.
> >> 
> >> [PATCH 4/4] annots: Make Annot::setBorder receive an AnnotBorder object
> >> 
> >> Currently we can only set AnnotBorderArray objects to annots. This might
> >> have no effect if the annots already has a BS entry, for example,
> >> because the BS takes precedence over Border. This patches changes
> >> Annot::setBorder to receive an AnnotBorder object, so that you can
> >> either pass an AnnotBorderArray or an  AnnotBorderBS. Frontends should
> >> always use BS when updating an annotation that can have BS entries. The
> >> patch also completes the implementation of writeToObject method for
> >> array borders and adds an impementation for BS borders too.
> >> 
> >> I've passed my tests with no regressions,
> > 
> > Are there improvements in rendering of some files?
> 
> Nothing changed in my tests, because most of the PDF documents either
> use explicit borders or don't use 0 border widths. I noticed this
> problem when reviewing the patches to add support for adding square and
> circle annotations in the glib frontend. Germán told me he needed to add
> a border or nothing was drawn, and that sounded wrong to me. So, reading
> the spec, our code and playing with acroread I ended up writing these
> patches. See bug https://bugs.freedesktop.org/show_bug.cgi?id=70983.
> 
> There might be an improvement if there were any file with a /Border in a
> FileAttachment annotation, for example, since we were drawing a border
> in such case, while these patches don't.
> 
> >> but I can't test the qt
> >> frontend (that uses setBorder(), for example), so it would be great if
> >> someone could test these patches to make sure they don't introduce any
> >> regression in qt (it should build in any case).
> > 
> > Why would it introduce any regression?
> 
> Because qt is the only frontend using Annot::setBorder() and I've
> changed that. I don't mean regressions of rendering existing files, but
> annotating files. I don't even know how Annot::setBorder() is used in
> qt, I just grepped and saw it was used.

You could have looked, it's like 4 lines. I'm not a huge specialist of the 
annot code, but looks like it should still work.

Anyway I did run Okular against a patched poppler and a unpatched one and 
annotations that hit that path render the same to my naked eyes.

BTW you can't commit the first patch since the qt frontends use that getType 
function you removed because noone was using.

Next time we meet i'll give you a memory stick so you can backup some of your 
stuff there and you can install Qt in the freed space.

Cheers,
  Albert

> 
> > Cheers,
> > 
> >   Albert
> >> 
> >> Leonard, please, feel free to correct me if I'm wrong in any of my
> >> interpretations of the PDF spec.
> >> 
> >> Regards,
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler