Albert Astals Cid <[email protected]> writes: > El Dimecres, 27 de novembre de 2013, a les 08:51:42, Carlos Garcia Campos va > escriure: >> Albert Astals Cid <[email protected]> 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.
Yes, I'm sorry, I wrote these patches quickly because I didn't have much time. > 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. Great, thanks for testing! > BTW you can't commit the first patch since the qt frontends use that getType > function you removed because noone was using. Oops, I'll fix it then, this time I'll have to actually look at the code :-P > 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. :-D > 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 >> > [email protected] >> > http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ > poppler mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/poppler -- Carlos Garcia Campos PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
pgpPWFMPiuxIJ.pgp
Description: PGP signature
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
