[Poppler-bugs] [Bug 83642] add a displayAnnot to Page class

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=83642

--- Comment #7 from Albert Astals Cid <aacid at kde.org> ---
> --- Comment #6 from Tobias Deiminger <haxtibal at posteo.de> ---
> Whitespace changes are removed now. See rev. 2 of
> https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a. Sorry,
> wanted to cleanup interleaved tabs and thought one could ignore changes
> during review. But obviously the "?w=1" trick doesn't work for gists. Btw.,
> if you prefer we can shift the patch over to bugs.freedesktop.org anytime.

Could you try to split this into a series of patches so they are smaller and
easier to review since each patch has a clear focus/idea behind it? Seems like
there's various stuff going on there (specially all the qt5 code moving seems
like could be easily split).

> You'll probably find more to dislike. I do, just had no better ideas yet.
> Some reasoning in advance:
> The changes around Qt5SplashOutputDev/ArthurOutputDev [0] are a bit heavy
> and unfinished (dislike!). But I wanted to share as much "factory code" as
> possible between Poppler::Page and Poppler::Annotation, thus factored out
> common stuff. The changes regarding Annot::draw [1] are a bit heavy and
> unfinished (again dislike). But I had to find a common place amongst all
> annotation types where one can hook in to get the real size of the
> dynamically generated appearance *1). Plus the template pattern approach
> hopefully helps poppler newbies like me to understand things better, as it
> makes common steps in Annot::draw schematic and explicit.

makeAppearance(obj, drawRect, getXRef()); is a bit ackward API, I think it
would be better if you return obj instead of passing it as output parameter.

> 
> Regarding API... QImage Poppler::Annotation::renderToImage(double hDPI,
> double vDPI) was the simplest API I could imagine. Functionality explicitly
> cares about one single annotation and no other objects, thus the method is
> bound to Annotation, instead of Page. We don't need to bother clients with
> position/size arguments, poppler knows it all, and better. We probably
> don't need callbacks. Maybe we want to allow fine tuning some rendering
> settings? E.g. an enableTransparentBackground option could make sense to
> chose either page color or transparency as background. 

What would be the use case for wanting the page color as background? 

> Also a further
> method
> Poppler::Annotation::renderToArthur(...) could be useful and would be
> consistent to Poppler::Page::renderToArthur.

Guess so yeah, but i wouldnt' really worry about that for now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20180304/035f83ea/attachment.html>