[poppler] Annot Improving (VIII)

[Albert Astals Cid]

A Divendres 11 Gener 2008, Iñigo Martínez va escriure:
> Here goes an update to the last patch fixing the comments. Anyway, I'm
> not very proud of the new code. I don't like the code that parses the
> quadrilateral points, it should be generic but as it depends on
> annotation fields I have implemented it in AnnotLink. Everything isn't
> bad thought, this way it checks for errors in the quadrilaterals points
> and it uses the annotation rect instead the quadrilaterals when they are
> wrong (as specified in the reference).
>
> Any comment is welcome.

0003-Changed-AnnotQuadrilateral-parsing-inside-AnnotLink.patch has lots of 
spacing changes, are they really needed? 

Besides that if noone objects i think we can commit them if you want.

Albert

>
> PD: I have been some days thinking on the better solution (for that
> little piece of code yes!), any idea on a better implementation for
> quadrilaterals parsing is welcome too.
>
> El dom, 30-12-2007 a las 23:54 +0100, Iñigo Martínez escribió:
> > El dom, 30-12-2007 a las 17:43 -0500, Jeff Muizelaar escribió:
> > > On Sun, Dec 30, 2007 at 11:19:22PM +0100, Iñigo Martínez wrote:
> > > > El dom, 30-12-2007 a las 16:56 -0500, Jeff Muizelaar escribió:
> > > > > On Sun, Dec 30, 2007 at 09:32:05PM +0100, Iñigo Martínez wrote:
> > > > > > Hi:
> > > > > >
> > > > > > Here goes two new patches. The first one improves AnnotLink
> > > > > > support, anyway it lacks a lot of features. The best option
> > > > > > should be to merge the actual Link implementation with the
> > > > > > AnnotLink one.
> > > > > >
> > > > > > The second patch adds support for AnnotFreeText.
> > > > > >
> > > > > > I have tested both (I have figured out the old problems, and now
> > > > > > I can test them correctly, anyway I have a few glib test on the
> > > > > > way) but I would prefer a review.
> > > > >
> > > > > I had a quick look and things looked pretty good. My comments
> > > > > follow below.
> > > > >
> > > > > -Jeff
> > > >
> > > > Thank you very much Jeff. I have done the changes to every if wrong
> > > > typed. I wrote them incorrectly in the last patches too.
> > > >
> > > > I have another question. I think I should move the AnnotQuadPoint
> > > > parsing from AnnotFreeTex code to the class implementation itself.
> > > > Something like (and yes, I will change quadrilateralsLength, I didn't
> > > > liked it neither when I writed it):
> > >
> > > Yeah, this change looks like a good idea.
> > >
> > > > I have some doubts with AnnotCalloutLine and AnnotCalloutMultiline.
> > > > Think that they use inheritance so they could need dynamic cast at
> > > > some point in the future.
> > >
> > > Can you explain this issue in more detail?
> >
> > It was recommended not to use dynamic cast as it wasn't used in any
> > other place in the poppler code. This case uses inheritance and usually
> > to cast between base and child classes a dynamic cast is needed. I'm
> > thinking that I should avoid it. Maybe using independent classes instead
> > of using inheritance ?
> >
> > > -Jeff