D18883: Add PDF thumbnailer

[Méven Car]

meven added a comment.


  In D18883#424888 , @aacid wrote:
  
  > 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"
  
  
  kio-extras thumbnail is a an ioslave so I guess we can add it simply.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: meven, bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, 
kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D18883: Add PDF thumbnailer

[Nathaniel Graham]

ngraham added a comment.


  Yeah, seems like this got bikeshedded to death but I think it would still be 
quite worthwhile to have.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: meven, bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, 
kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D18883: Add PDF thumbnailer

[Méven Car]

meven added a comment.


  ping
  
  Is it still standing ?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: meven, bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, 
kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D18883: Add PDF thumbnailer

[Albert Astals Cid]

aacid added a comment.


  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"

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, 
alexde, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, 
alexde, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Stefan Brüns]

bruns added a comment.


  In D18883#414517 , @broulik wrote:
  
  > 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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: bruns, dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, 
alexde, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik added a comment.


  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.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Albert Astals Cid]

aacid added a comment.


  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?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik added a comment.


  Ping

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> dhaumann wrote in pdfcreator.cpp:62
> Does this also give nice results with high dpi displays?

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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> pdfcreator.cpp:62
> +// Compute a dpi that results in a thumbnail roughly the size we 
> requested
> +const qreal fakeDpiX = width / page->pageSizeF().width() * 72.0;
> +const qreal fakeDpiY = height / page->pageSizeF().height() * 72.0;

Does this also give nice results with high dpi displays?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: dhaumann, ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, 
feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Pino Toscano]

pino added a comment.


  In D18883#408893 , @ngraham wrote:
  
  > 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?).

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik updated this revision to Diff 51292.
broulik added a comment.


  - Render image at desired size already
  - Remove unused include

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18883?vs=51290=51292

REVISION DETAIL
  https://phabricator.kde.org/D18883

AFFECTED FILES
  thumbnail/CMakeLists.txt
  thumbnail/pdfcreator.cpp
  thumbnail/pdfcreator.h
  thumbnail/pdfthumbnail.desktop

To: broulik, dfaure, aacid, jtamate
Cc: ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D18883: Add PDF thumbnailer

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: ngraham, pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D18883: Add PDF thumbnailer

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> pdfcreator.cpp:23
> +
> +#include 
> +#include 

the QFile include is no more needed now

> broulik wrote in pdfcreator.cpp:45-46
> 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.

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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> pino wrote in pdfcreator.cpp:45-46
> please honor the requested width and height

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.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik updated this revision to Diff 51290.
broulik added a comment.


  - Remove mimetypes left from testing
  - Don't encode filename (missed that there's one other than the `QByteArray` 
one)

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18883?vs=51281=51290

REVISION DETAIL
  https://phabricator.kde.org/D18883

AFFECTED FILES
  thumbnail/CMakeLists.txt
  thumbnail/pdfcreator.cpp
  thumbnail/pdfcreator.h
  thumbnail/pdfthumbnail.desktop

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> pdfcreator.cpp:45-46
> +{
> +Q_UNUSED(width);
> +Q_UNUSED(height);
> +

please honor the requested width and height

> pdfcreator.cpp:49
> +QScopedPointer document;
> +document.reset(Document::load(QFile::encodeName(path)));
> +

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

> pdfthumbnail.desktop:6
> +X-KDE-ServiceTypes=ThumbCreator
> +MimeType=application/pdf;application/x-dvi;application/postscript;
> +

definitely neither DVI nor PS...

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Luigi Toscano]

ltoscano added a comment.


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

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

To: broulik, dfaure, aacid, jtamate
Cc: ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D18883: Add PDF thumbnailer

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: dfaure, aacid, jtamate.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
broulik requested review of this revision.

REVISION 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).

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D18883

AFFECTED FILES
  thumbnail/CMakeLists.txt
  thumbnail/pdfcreator.cpp
  thumbnail/pdfcreator.h
  thumbnail/pdfthumbnail.desktop

To: broulik, dfaure, aacid, jtamate
Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov