Bug 102513 - [PATCH] Allow to control whether renderToImage shows annotations
Summary: [PATCH] Allow to control whether renderToImage shows annotations
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: qt frontend (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-01 20:30 UTC by oliver.sander
Modified: 2017-09-04 19:52 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Patch that allows to control whether annotations are shown (3.96 KB, patch)
2017-09-01 20:30 UTC, oliver.sander
Details | Splinter Review
Patch that allows to control whether annotations are shown (4.30 KB, patch)
2017-09-04 14:49 UTC, oliver.sander
Details | Splinter Review

Description oliver.sander 2017-09-01 20:30:57 UTC
Created attachment 133932 [details] [review]
Patch that allows to control whether annotations are shown

I'd like to control whether the renderToImage and renderToPainter methods show annotations or not. To this end, this patch introduces a new value 'HideAnnotations' to the Document::RenderHint enum, and makes the said methods mind it.

I have no idea whether this is a good way to get the new option into the interface, but it seemed the least invasive one to me.  renderToPainter has a dedicated PainterFlags parameter, but renderToImage doesn't see that one. Is the new RenderHint value okay?
Comment 1 Albert Astals Cid 2017-09-01 22:36:29 UTC
I'd prefer if in the showAnnotations case you pass null as the callback, that way we save a function for nothing.
Comment 2 oliver.sander 2017-09-02 18:35:35 UTC
That makes sense.  However, I've been wondering: if we touch the interface for this, shouldn't we make it more future-proof?  Maybe somebody will want more fine-grained control in the future.  We could add more options, but we could also simply have the call-back as a new parameter for renderToImage and renderToPainter.  What do you think?
Comment 3 Albert Astals Cid 2017-09-02 20:20:06 UTC
Before adding such a weird API, i'd like to listen why anyone would need it :D
Comment 4 oliver.sander 2017-09-03 04:40:53 UTC
I don't personally need more than the HideAnnotations flag.  But I could imagine that somebody would want to selectively hide certain annotation types, for example.
Comment 5 Albert Astals Cid 2017-09-03 19:55:08 UTC
Writing a library API without a use case usually ends up in broken API, let's not write something noone has asked to have :)
Comment 6 oliver.sander 2017-09-04 14:49:17 UTC
Alright.  New patch is attached.
Comment 7 oliver.sander 2017-09-04 14:49:39 UTC
Created attachment 133961 [details] [review]
Patch that allows to control whether annotations are shown
Comment 8 Albert Astals Cid 2017-09-04 19:52:21 UTC
Pushed


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.