[poppler] Annot Improving

[Iñigo Martínez]

2007/11/6, Jeff Muizelaar <jeff at infidigm.net>:
> On Sun, Nov 04, 2007 at 06:32:26PM +0100, Iñigo Martínez wrote:
> > After some time very busy, I got a little time to work again, so I
> > have started splitting the huge annotation patch. The idea is to
> > implemente everything again slowly.
> >
> > These first changes are about the implementation of the AnnotBorder.
> >
> > Everything is changed using the official specification.
> >
> > 1. There are two types of borders. The old one with Border flag and
> > the new one with the BS flag. The border have only styles in the new
> > border type, BS.
> > 2. I have implemented both using inheritance.
> > 3. Colors aren't part of the Border and BS border types, so I have
> > taken them out and boxed in another class.
> >
> > I have created a new patch, but i'm new to git, so i'm not sure if I
> > have created it correctly.
>
> It looks like it was created correctly.
>
> >
> > (I have some tests done in the glib side, but they need the core to be
> > more complete than it's actually).
> >
> > In the old implementation I did in the summer, I was missing the
> > saving feature, I need to implement the setters and I don't understand
> > very well that side, so if anyone could help me...
>
> Have you had a look at the saving code by Julien?
> http://lists.freedesktop.org/archives/poppler/2007-October/003052.html
>
> Some review follow below.
>
> -Jeff
>
> >  void Annot::generateFieldAppearance(Dict *field, Dict *annot, Dict *acroForm) {
> > @@ -410,7 +490,7 @@ void Annot::generateFieldAppearance(Dict *field, Dict *annot, Dict *acroForm) {
> >
> >    // draw the border
> >    if (mkDict) {
> > -    w = borderStyle->getWidth();
> > +    w = border->getWidth();
> >      if (w > 0) {
> >        mkDict->lookup("BC", &obj1);
> >        if (!(obj1.isArray() && obj1.arrayGetLength() > 0)) {
> > @@ -424,80 +504,84 @@ void Annot::generateFieldAppearance(Dict *field, Dict *annot, Dict *acroForm) {
> >          hasCaption = mkDict->lookup("CA", &obj2)->isString();
> >          obj2.free();
> >          if (ftObj.isName("Btn") && (ff & fieldFlagRadio) && !hasCaption) {
> > -          r = 0.5 * (dx < dy ? dx : dy);
> > -          switch (borderStyle->getType()) {
> > -            case annotBorderDashed:
> > -              appearBuf->append("[");
> > -              borderStyle->getDash(&dash, &dashLength);
> > -              for (i = 0; i < dashLength; ++i) {
> > -                appearBuf->appendf(" {0:.2f}", dash[i]);
> > -              }
> > -              appearBuf->append("] 0 d\n");
> > -              // fall through to the solid case
> > -            case annotBorderSolid:
> > -            case annotBorderUnderlined:
> > -              appearBuf->appendf("{0:.2f} w\n", w);
> > -              setColor(obj1.getArray(), gFalse, 0);
> > -              drawCircle(0.5 * dx, 0.5 * dy, r - 0.5 * w, gFalse);
> > -              break;
> > -            case annotBorderBeveled:
> > -            case annotBorderInset:
> > -              appearBuf->appendf("{0:.2f} w\n", 0.5 * w);
> > -              setColor(obj1.getArray(), gFalse, 0);
> > -              drawCircle(0.5 * dx, 0.5 * dy, r - 0.25 * w, gFalse);
> > -              setColor(obj1.getArray(), gFalse,
> > -                  borderStyle->getType() == annotBorderBeveled ? 1 : -1);
> > -              drawCircleTopLeft(0.5 * dx, 0.5 * dy, r - 0.75 * w);
> > -              setColor(obj1.getArray(), gFalse,
> > -                  borderStyle->getType() == annotBorderBeveled ? -1 : 1);
> > -              drawCircleBottomRight(0.5 * dx, 0.5 * dy, r - 0.75 * w);
> > -              break;
>
> Won't the new code not draw a border unless a BS is specified? Whereas
> the old code defaulted to a solid border?
>
> Also, shouldn't the AnnotBorderArray class just have a getStyle()
> method that returns borderSolid? That way we avoid having to do the
> dynamic_cast. Currently there are no dynamic_casts in the core poppler
> code. It'd be nice if we could keep it that way, unless there's good
> reason to change.
>

Ups, I near missed this review.

Yes, the old code draws a border when any border is available (with
both Border and BS). I didn't know how to support Border (array)
exactly, I mean how it should be painted. Taking in account that Solid
is the default for BS, I can suppose that the border array should be
painted with that style. Looking deeper the specification, I have
found this one too:

Section 8.4, Page 611
If neither the Border nor the BS entry is present, the border is drawn
as a solid line with a width of 1 point.

Yes, we can assume that border array should be painted with that
style. I will change it this evening, and it does solve the second
problem as both border types should have the getStyle() method. Anyway
I will try to avoid dynamic_casts.

The first time I changed it, I saw only BS type border styles, so I
supposed that it had to be painted only in the case of being a BS
border. I tried to maintain the old code and I didn't implemented any
code to support border arrays, my idea was to implement them sometime
in the future, once the Annot API was more stable.

> > +          AnnotBorderBS *borderBS = dynamic_cast<AnnotBorderBS *> (border);
> > +          if (borderBS) {
> > +            r = 0.5 * (dx < dy ? dx : dy);
> > +            switch (borderBS->getStyle()) {
> > +              case AnnotBorderBS::borderDashed:
> > +                appearBuf->append("[");
> > +                dashLength = border->getDashLength();
> > +                dash = border->getDash();
> > +                for (i = 0; i < dashLength; ++i)
> > +                    appearBuf->appendf(" {0:.2f}", dash[i]);
> > +                appearBuf->append("] 0 d\n");
> > +                // fall through to the solid case
> > +              case AnnotBorderBS::borderSolid:
> > +              case AnnotBorderBS::borderUnderlined:
> > +                appearBuf->appendf("{0:.2f} w\n", w);
> > +                setColor(obj1.getArray(), gFalse, 0);
> > +                drawCircle(0.5 * dx, 0.5 * dy, r - 0.5 * w, gFalse);
> > +                break;
> > +              case AnnotBorderBS::borderBeveled:
> > +              case AnnotBorderBS::borderInset:
> > +                appearBuf->appendf("{0:.2f} w\n", 0.5 * w);
> > +                setColor(obj1.getArray(), gFalse, 0);
> > +                drawCircle(0.5 * dx, 0.5 * dy, r - 0.25 * w, gFalse);
> > +                setColor(obj1.getArray(), gFalse, borderBS->getStyle()
> > +                    == AnnotBorderBS::borderBeveled ? 1 : -1);
> > +                drawCircleTopLeft(0.5 * dx, 0.5 * dy, r - 0.75 * w);
> > +                setColor(obj1.getArray(), gFalse, borderBS->getStyle()
> > +                    == AnnotBorderBS::borderBeveled ? -1 : 1);
> > +                drawCircleBottomRight(0.5 * dx, 0.5 * dy, r - 0.75 * w);
> > +                break;
> > +            }
> >            }