Add blender thumbnailer
ClosedPublic

Authored by chinmoyr on Mar 12 2019, 1:01 AM.

Details

Summary

This extracts thumbnail from .blend files.
FEATURE: 246050

Before: Default icon.
After:

Diff Detail

Repository
R373 Image Thumbnailers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9636
Build 9654: arc lint + arc unit
chinmoyr created this revision.Mar 12 2019, 1:01 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptMar 12 2019, 1:01 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Mar 12 2019, 1:01 AM
chinmoyr edited the summary of this revision. (Show Details)Mar 12 2019, 1:06 AM

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

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?

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.

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

bruns requested changes to this revision.Mar 12 2019, 4:12 PM
bruns added a subscriber: bruns.
bruns added inline comments.
thumbnail/blendercreator.cpp
57 ↗(On Diff #53699)

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

62 ↗(On Diff #53699)

} else { ...?

73 ↗(On Diff #53699)

This is missing the 'v' / 'V'

98 ↗(On Diff #53699)

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

101 ↗(On Diff #53699)

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

105 ↗(On Diff #53699)

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

This revision now requires changes to proceed.Mar 12 2019, 4:12 PM
bruns added inline comments.Mar 12 2019, 11:33 PM
thumbnail/blendercreator.cpp
134 ↗(On Diff #53699)
chinmoyr updated this revision to Diff 53820.EditedMar 13 2019, 6:49 PM
chinmoyr marked 4 inline comments as done.

Moved to kdegraphics-thumbnailer.
Got rid of operator>>(). Now reading the entire file-block header and extracting the length from raw bytes.
Used QByteArrays instead of static char arrays.
Fixed some minor issues.

chinmoyr added inline comments.Mar 13 2019, 6:50 PM
thumbnail/blendercreator.cpp
57 ↗(On Diff #53699)

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

101 ↗(On Diff #53699)

yes

134 ↗(On Diff #53699)

I am trying to swap bgr to rgb here.

bruns added inline comments.Mar 13 2019, 8:15 PM
blend/blendercreator.cpp
68

This requires a comment, stating gzip compression is optional.

86

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

106

Not relevant, as you only read using readRawData()

111

Coding style, no trailing return type.

thumbnail/blendercreator.cpp
134 ↗(On Diff #53699)

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

chinmoyr added inline comments.Mar 14 2019, 2:32 AM
thumbnail/blendercreator.cpp
134 ↗(On Diff #53699)

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

chinmoyr updated this revision to Diff 53848.Mar 14 2019, 2:34 AM
chinmoyr marked 3 inline comments as done.

Issues fixed.

Save the two nitpicks, looks good to me now.

Anyone else any comments?

blend/blendercreator.cpp
25

Really required?

30

Unused ...

chinmoyr updated this revision to Diff 53971.Mar 15 2019, 5:45 PM

Removed unused headers.

bruns accepted this revision.Mar 16 2019, 2:44 AM
This revision is now accepted and ready to land.Mar 16 2019, 2:44 AM
This revision was automatically updated to reflect the committed changes.