[poppler] Patch to add simple TOC interface

Adam Reichold adamreichold at myopera.com
Mon Mar 26 12:32:29 PDT 2012 

Am 26.03.2012, 20:31 Uhr, schrieb Albert Astals Cid <aacid at kde.org>:

> 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
>

I know but thanks for the reminder anyway. I though about it and while I  
think I understand the second part. I am not so sure how to go about the  
first problem without starting with a QDomDocument and wrapping that up in  
a SimpleTocNode which sort of defeats the whole purpose. The only other  
option I see would be to replace that interface but that is obviously not  
really an option. So I retract that proposal until I have a really good  
idea how to do this in a way that's better than just writing a wrapper  
around QDomDocument inside my application.

Sorry I you feel like you lost time looking at the code and thanks for the  
comments.

Regards, Adam.

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

More information about the poppler mailing list