[poppler] Patch to add simple TOC interface

Albert Astals Cid aacid at kde.org
Mon Mar 26 11:31:04 PDT 2012 

El Dimecres, 21 de març de 2012, a les 23:26:06, Albert Astals Cid va 
escriure:
> 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.

You have three days until 0.20 feature freeze.

Albert

> 
> 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
> 
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler

More information about the poppler mailing list