D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

This revision was automatically updated to reflect the committed changes.
Closed by commit R373:2da545defff9: Add blender thumbnailer (authored by 
chinmoyr).

REPOSITORY
  R373 Image Thumbnailers

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19696?vs=53971&id=54065

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

AFFECTED FILES
  CMakeLists.txt
  blend/CMakeLists.txt
  blend/blendercreator.cpp
  blend/blendercreator.h
  blend/blenderthumbnail.desktop

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

D19696: Add blender thumbnailer

[Stefan Brüns]

bruns accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R373 Image Thumbnailers

BRANCH
  master

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

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

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 53971.
chinmoyr added a comment.


  Removed unused headers.

REPOSITORY
  R373 Image Thumbnailers

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19696?vs=53848&id=53971

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  blend/CMakeLists.txt
  blend/blendercreator.cpp
  blend/blendercreator.h
  blend/blenderthumbnail.desktop

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

D19696: Add blender thumbnailer

[Stefan Brüns]

bruns added a comment.


  Save the two nitpicks, looks good to me now.
  
  Anyone else any comments?

INLINE COMMENTS

> blendercreator.cpp:24
> +#include 
> +#include 
> +

Really required?

> blendercreator.cpp:29
> +#include 
> +#include 
> +#include 

Unused ...

REPOSITORY
  R373 Image Thumbnailers

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

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

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 53848.
chinmoyr marked 3 inline comments as done.
chinmoyr added a comment.


  Issues fixed.

REPOSITORY
  R373 Image Thumbnailers

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19696?vs=53820&id=53848

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  blend/CMakeLists.txt
  blend/blendercreator.cpp
  blend/blendercreator.h
  blend/blenderthumbnail.desktop

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

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr added inline comments.

INLINE COMMENTS

> bruns wrote in blendercreator.cpp:134
> So does the code I linked to. No need to cook your own.

Ah, sorry. I misunderstood what was written in doc. It really does swap them.

REPOSITORY
  R373 Image Thumbnailers

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

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

D19696: Add blender thumbnailer

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> blendercreator.cpp:67
> +QPointer gzFile;
> +if(file.peek(2).startsWith("\x1F\x8B")) { // gzip magic (each gzip 
> member starts with ID1(0x1f) and ID2(0x8b))
> +file.close();

This requires a comment, stating gzip compression is optional.

> blendercreator.cpp:85
> +
> +// Example header: "BLENDER-257"
> +

Still wrong, "BLENDER-v257", missing 'v'/'V'

> blendercreator.cpp:105
> +if(isLittleEndian) {
> +blendStream.setByteOrder(QDataStream::LittleEndian);
> +} else {

Not relevant, as you only read using `readRawData()`

> blendercreator.cpp:110
> +
> +auto toInt32 = [isLittleEndian](const QByteArray &bytes) -> qint32 {
> +return isLittleEndian ? qFromLittleEndian(bytes.constData())

Coding style, no trailing return type.

> chinmoyr wrote in blendercreator.cpp:134
> I am trying to swap bgr to rgb here.

So does the code I linked to. No need to cook your own.

REPOSITORY
  R373 Image Thumbnailers

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

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

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 53820.
chinmoyr marked 4 inline comments as done.
chinmoyr added a comment.


  Moved to kdegraphics-thumbnailer
  Got rid of operator>>() and fixed other issues.

REPOSITORY
  R373 Image Thumbnailers

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19696?vs=53699&id=53820

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  blend/CMakeLists.txt
  blend/blendercreator.cpp
  blend/blendercreator.h
  blend/blenderthumbnail.desktop

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

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr added inline comments.

INLINE COMMENTS

> bruns wrote in blendercreator.cpp:57
> I think this is already done by the calling code - the thumbnailer gets only 
> called when the mimetype matches.

This part is required because blender can save files with gzip compression. 
Examples here :   https://download.blender.org/demo/test/Demo_274.zip. For some 
of them properties dialog shows "Contents: application/gzip".

> bruns wrote in blendercreator.cpp:101
> Does this mean 'size of header fields 1 to 5'?

yes

> bruns wrote in blendercreator.cpp:134
> https://doc.qt.io/qt-5/qimage.html#rgbSwapped-1

I am trying to swap bgr to rgb here.

REPOSITORY
  R373 Image Thumbnailers

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

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

D19696: Add blender thumbnailer

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> blendercreator.cpp:134
> +blendStream.readRawData(imgBuffer, length);
> +abgr_to_argb(imgBuffer, length);
> +QImage thumbnail((unsigned char*)imgBuffer, x, y, QImage::Format_ARGB32);

https://doc.qt.io/qt-5/qimage.html#rgbSwapped-1

REPOSITORY
  R320 KIO Extras

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

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

D19696: Add blender thumbnailer

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> blendercreator.cpp:57
> +QPointer gzFile;
> +if(file.peek(2).startsWith("\x1F\x8B")) { // gzip magic (each gzip 
> member starts with ID1(0x1f) and ID2(0x8b))
> +file.close();

I think this is already done by the calling code - the thumbnailer gets only 
called when the mimetype matches.

> blendercreator.cpp:62
> +blendStream.setDevice(gzFile);
> +}
> +}

`} else {` ...?

> blendercreator.cpp:73
> +
> +// Example header: "BLENDER-257"
> +

This is missing the 'v' / 'V'

> blendercreator.cpp:98
> +}
> +const qint32 REND = isLittleEndian ? 1145980242 /*python 
> int.from_bytes(b'REND', byteorder='little')*/ : 1380273732;
> +const qint32 TEST = isLittleEndian ? 1414743380 : 1413829460;

When you write these as hex, it becomes more obvious one is "REND" while the 
other is "DNER". Although ...

> blendercreator.cpp:101
> +const bool is64Bit = head[7] == '-';
> +const int fileBlockHeadSize = is64Bit ? 24 : 20; // sum(1 through 6)
> +const int remainingHeader = fileBlockHeadSize - 8; // sum(3 through 5)

Does this mean 'size of header fields 1 to 5'?

> blendercreator.cpp:105
> +while (true) {
> +blendStream >>  code >> length;
> +if (code == REND) {

... you can use `readRawData(data, 4)` here and just compare with 'REND'.

Or even always read the whole header, saving you the 
`skipRawData(remainingHeader)`, and gets rid of `remainingHeader`.

Actually, using `operator>>` on raw data, i.e. anything not written using 
QDataStream is dangerous. There is **no** guarantee an int is serialized as 
just 4 bytes.

REPOSITORY
  R320 KIO Extras

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

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

D19696: Add blender thumbnailer

[Nathaniel Graham]

ngraham added a comment.


  What would make sense to me is to have a single repo/package that holds all 
thumbnailers (`kde-thumbnailers`?). Having them in multiple repos/packages 
doesn't make sense because there's no real reason to want to only exclude 
certain thumbnailers in the packaging, and it increases the likelihood that 
users and distros will accidentally exclude desired thumbnailers.
  
  +10 for this particular feature, regardless of where it ends up living. :)

REPOSITORY
  R320 KIO Extras

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

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

D19696: Add blender thumbnailer

[Luigi Toscano]

ltoscano added a comment.


  In D19696#429476 , @broulik wrote:
  
  > Since this one is a bit more specific, perhaps it should indeed go into 
kdegraphics-thumbnailers, don't really mind. I think I have asked this often 
before but is kdegraphics-thumbnailers a thing that is installed out of the box 
in most distros?
  
  
  Our organizational decision shouldn't be affected by downstream consumers if 
they don't affect them in a bad way (as in this case).
  
  Distributions can surely install kdegraphics-thumbnailers as they do with 
kio-extras, if we tell them so.

REPOSITORY
  R320 KIO Extras

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

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

D19696: Add blender thumbnailer

[Kai Uwe Broulik]

broulik added a comment.


  Since this one is a bit more specific, perhaps it should indeed go into 
kdegraphics-thumbnailers, don't really mind. I think I have asked this often 
before but is kdegraphics-thumbnailers a thing that is installed out of the box 
in most distros?

REPOSITORY
  R320 KIO Extras

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

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

D19696: Add blender thumbnailer

[Luigi Toscano]

ltoscano added a comment.


  I have my usual question: shouldn't this go into kdegraphics-thumbnailers? 
kio-extras shouldn't be a "dump everything which is not somewhere else".

REPOSITORY
  R320 KIO Extras

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

To: chinmoyr, #frameworks
Cc: ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr edited the summary of this revision.

REPOSITORY
  R320 KIO Extras

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

To: chinmoyr, #frameworks
Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D19696: Add blender thumbnailer

[Chinmoy Ranjan Pradhan]

chinmoyr created this revision.
chinmoyr added a reviewer: Frameworks.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
chinmoyr requested review of this revision.

REVISION SUMMARY
  This extracts thumbnail from .blend files.
  
  Before: Default icon.
  After:
  F6685286: Blender thumbnail 

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

AFFECTED FILES
  thumbnail/CMakeLists.txt
  thumbnail/blendercreator.cpp
  thumbnail/blendercreator.h
  thumbnail/blenderthumbnail.desktop

To: chinmoyr, #frameworks
Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov