Add PDF thumbnailer
Needs ReviewPublic

Authored by broulik on Feb 9 2019, 6:09 PM.

Details

Summary

This adds a PDF thumbnailer using libpoppler.
There is one in kdegraphics-thumbnailers but that one (as far as I could understand the old (fork+exec etc) and convoluted code) relies on an external gs binary to generate the thumbnails and is pretty slow.
This one is just 40 lines of code and would get people PDF thumbnails out of the box.

Test Plan

Disabled gsthumbs from kdegraphics-thumbnailer, still got neat little PDF thumbnails
EPS files can be generated using KImageFormats (cf D18882) so the only bits missing from kdegraphics-thumbnailers (which isn't installed by default on most distros) is DVI and ghostscript files (and RAW files).

Diff Detail

Repository
R320 KIO Extras
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Feb 9 2019, 6:09 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptFeb 9 2019, 6:09 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.Feb 9 2019, 6:09 PM

Why can't this go to kdegraphics-thumbnailer? Kio-extras shouldn't be, IMHO, a "drop everything which does not fit elsewhere"

pino added a subscriber: pino.Feb 9 2019, 8:10 PM
pino added inline comments.
thumbnail/pdfcreator.cpp
46–47

please honor the requested width and height

50

Document::load() takes a QString, so do not encode the filename (which will be encoded twice)

thumbnail/pdfthumbnail.desktop
7

definitely neither DVI nor PS...

broulik updated this revision to Diff 51290.Feb 9 2019, 9:54 PM
  • Remove mimetypes left from testing
  • Don't encode filename (missed that there's one other than the QByteArray one)
broulik added inline comments.Feb 9 2019, 9:56 PM
thumbnail/pdfcreator.cpp
46–47

I can't. The renderToImage can only be told a resolution or part of the page to render, to render downscaled into a certain box.

The ThumbnailJob downscales the image when it exceeds the requested size, so doing any manual downscaling (other than already getting the correct size which we can't) here is superfluous.

pino added inline comments.Feb 9 2019, 10:00 PM
thumbnail/pdfcreator.cpp
24

the QFile include is no more needed now

46–47

Sure you can: see what okular does, for example, as it requests pixmaps of precise sizes.

Conceptually I think it makes sense to have all the thumbnailers in their own package so it's easy to remove them all if for people who really don't want them. That package (kdegraphics-thumbnailers) is shipped by default by at least Kubuntu, openSUSE, Manjaro and Neon, so it's not like moving them all over there would result in a user experience regression for many (any?) people.

I don't have a problem with ultimately putting everything into that separate package, but perhaps it makes sense to do that all at once in a deliberate manner rather than piecemeal. In which case we would land this, then move all the thumbnailers in kio-extras into kdegraphics-thumbnailers.

broulik updated this revision to Diff 51292.Feb 9 2019, 10:46 PM
  • Render image at desired size already
  • Remove unused include
pino added a comment.Feb 9 2019, 11:02 PM

Conceptually I think it makes sense to have all the thumbnailers in their own package so it's easy to remove them all if for people who really don't want them.

... resulting in a single package with lots of dependencies. Or the distributions will split the package (how?), getting compliants from upstream (yay?).

dhaumann added inline comments.
thumbnail/pdfcreator.cpp
62

Does this also give nice results with high dpi displays?

broulik added inline comments.Feb 10 2019, 11:18 AM
thumbnail/pdfcreator.cpp
62

KIO Thumbnailer doesn't support high dpi at all at this point.

The code can't be much simpler than this, there's not much places it can go wrong, i guess the only other thing to mention is, poppler will crash with some bad data, so i don't know if this is on it's own process or not, but if it's not it may bring down whatever app uses it, though given the gs issues lately it's probably not worse?

kio_thumbnail is a separate process (like all kioslaves) so a crash doesn't bring down the user-visible application.

However, you will get a Dr Konqi each time it encounters that file as it will try to generate a thumbnail for it every time you open the folder.

bruns added a subscriber: bruns.Feb 18 2019, 4:21 PM

However, you will get a Dr Konqi each time it encounters that file as it will try to generate a thumbnail for it every time you open the folder.

Shouldn't this be covered somehow by
https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-0.8.0.html#FAILURES

Perhaps but it's only recently become an issue since kioslaves now support KCrash. Previously such thumbnailer crashes just went unnoticed.

aacid added a comment.Mar 4 2019, 11:26 PM

Also do we have a way to limit thumbnailers to run say for X seconds? There's some files in which poppler will run "forever"