[poppler] Patch to add simple TOC interface

[Albert Astals Cid]

El Dimecres, 21 de març de 2012, a les 10:30:47, Adam Reichold va escriure:
> Hello,

Hi

> I attached a patch to add a simple TOC interface to the qt4 frontend as
> you proposed. This being my first try to contribute to poppler, I am very
> unsure of whether I correctly adhered to coding style and patch creation
> practices. I also had no idea of where to put the new structure
> "SimpleTocNode" I declared. (It currently just "hangs out" in
> "Poppler::Document" being declared just above the using method. This is
> surely not optimal.) So I really would appreciate helpful advice on how to
> make this fit for inclusion.

The code is wrong in two parts.

First, most of your new functions is shared with the old ones (they are 
basically the same but instead of inserting into the QDomElement insert into 
your structure). This is not acceptable since code duplication is bad. Please 
reuse the existing code.

Second, you are adding a structure with it's members public. This is a no go. 
Please see how the rest of the public classes have a d pointer and follow the 
same pattern.

Cheers,
  Albert

> Concerning functionality, I tried to stick to the current ways of doing
> things as close as possible. So this really just replaces "QDomDocument"
> with a custom data structure "SimpleTocNode." I also modified the qt4 demo
> to use the simple TOC interface, but hid the modifications behind an
> #ifdef. (I think one could replace the "QString" entries with the actual
> poppler data structures like "Poppler::LinkDestination" and so on, but I
> am trying not to get ahead of myself.)
> 
> Hope this this your liking. Best regards, Adam.
> 
> Am 21.03.2012, 00:17 Uhr, schrieb Albert Astals Cid <aacid at kde.org>:
> > El Dimecres, 21 de març de 2012, a les 00:03:03, Adam Reichold va
> > 
> > escriure:
> >> Hello,
> >> 
> >> Am 20.03.2012, 23:49 Uhr, schrieb Albert Astals Cid <aacid at kde.org>:
> >> > El Dimarts, 20 de març de 2012, a les 14:50:59, Adam Reichold va
> >> > 
> >> > escriure:
> >> >> Hello,
> >> > 
> >> > Hi
> >> > 
> >> >> I am currently maintaining a small PDF viewer called qpdfview that is
> >> >> using the poppler library through the qt4 frontend. I have three
> >> >> question
> >> >> that came up during development:
> >> >> 
> >> >> - The ArthurOutputDev seems incomplete. Are there plans on this?
> >> > 
> >> > There's noone working on it, patches are welcome.
> >> 
> >> I feared something like that. :-)
> >> 
> >> >> Can
> >> >> someone point me to documentation about this?
> >> > 
> >> > There's no documentation.
> >> > 
> >> >> (I would like to use it for
> >> >> printing in a platform independent way for which I currently fall
> >> 
> >> back
> >> 
> >> >> to
> >> >> drawing images to the printer. (I have seen a similar workaround in
> >> >> Okular
> >> >> when built on Windows.))
> >> >> 
> >> >> - The Poppler::Page::search method is deprecated but I could not find
> >> >> any
> >> >> documentation of what the plans for this are. Again, can someone
> >> 
> >> point
> >> 
> >> >> out
> >> >> relevant documentation to me?
> >> > 
> >> > Use the non deprecated version of Poppler::Page::search?
> >> 
> >> Oh. I assumed the version of search with the separate parameters for the
> >> rectangle was just a convenience overload.
> > 
> > You asumed wrong, the deprecation marker is clearly only on one of them
> > 
> > :-)
> > :
> >> Is it a long story why the first (easier?) one was deprecated?
> > 
> > Because a QRect is uses qreal which are floats on some arches which
> > produces
> > comparison problems with poppler internals that use doubles.
> > 
> >> >> - The Poppler::Document::toc method currently returns a QDomDocument
> >> >> pointer. Is there any other way to get the TOC using this frontend.
> >> > 
> >> > No
> >> > 
> >> >> I
> >> >> would like to avoid using QDomDocument as this alone would add
> >> 
> >> libQtXml
> >> 
> >> >> as
> >> >> a dependency to the program. Are there any opinions on replacing this
> >> >> by a
> >> >> simple custom tree model of the TOC?
> >> > 
> >> > What's the problem of linking with libQtXml?
> >> 
> >> I don't want to say that there is anything inherently wrong about it. It
> >> just feels a little excessive to link that whole library for that one
> >> feature. I feel like some of my users will care about adding
> >> dependencies.
> > 
> > Sincerely I don't think your users will care about that, they care about
> > features, not if your program is written in C++, C, Java or whatnot, and
> > if
> > they care, you should be totally right to ignore them.
> > 
> >> I also thought that extracting the information from the QDomDocument is
> >> a
> >> bit cumbersome considering the rather well-known structure of the TOC.
> >> But
> >> of course, I can always write some wrapper around that if really feel
> >> the
> >> need to. So this is probably just an opinion.
> > 
> > If you send a patch and it's not a lot of code we might consider it for
> > inclusion. Feature for poppler 0.20 are frozen on thursday next week.
> > 
> > Albert
> > 
> >> > Albert
> >> > 
> >> >> Thank you for your help.
> >> >> 
> >> >> Best regards, Adam.
> >> >> _______________________________________________
> >> >> poppler mailing list
> >> >> poppler at lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/poppler
> >> > 
> >> > _______________________________________________
> >> > poppler mailing list
> >> > poppler at lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/poppler
> >> 
> >> Thanks again. Best regards, Adam.
> >> _______________________________________________
> >> poppler mailing list
> >> poppler at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/poppler
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler