Review Request 125590: Patch for kimageformats that handles xcfgz / xcfbz2 (compressed GIMP) images (read-only).

[Alex Merry]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125590/#review87357
-----------------------------------------------------------


Thanks for this! The code generally looks pretty good, but see the comments below.

Your commit message should include the following on a separate line:

    BUG: 126072

We could potentially add the entry you suggest in your "testing" box to the mime database as part of kimageformats, although I'm not sure how that would interact with entries from The Gimp.

I would appreciate some unit tests - have a look at what I've done for other formats in the autotests directory. Given that all this plugin does is uncompress and defer to an existing plugin, it only needs to be very simple.


CMakeLists.txt (lines 43 - 44)
<https://git.reviewboard.kde.org/r/125590/#comment59988>

    You should use set_package_properties to say what KF5Archive is required for (see the OpenEXR one just above, for example, although you could also add URL and DESCRIPTION properties since it will be found via a Config module rather than a Find module).



src/imageformats/xcf_compressed.cpp (lines 1 - 8)
<https://git.reviewboard.kde.org/r/125590/#comment59992>

    I know many of the existing files in kimageformats don't follow this, but it would be better to use the longer license header found at https://techbase.kde.org/Policies/Licensing_Policy#LGPL_Header
    
    The (C) is also kind of pointless (although does no harm).



src/imageformats/xcf_compressed.cpp (lines 18 - 21)
<https://git.reviewboard.kde.org/r/125590/#comment59989>

    It would be better to use catagorised debugging (qCDebug). Then it can be switched at runtime.



src/imageformats/xcf_compressed.cpp (lines 79 - 99)
<https://git.reviewboard.kde.org/r/125590/#comment59990>

    Could you not pass the KCompressionDevice directly to QImage::load(), rather than using a temporary file?



src/imageformats/xcf_compressed.cpp (line 126)
<https://git.reviewboard.kde.org/r/125590/#comment59991>

    Oops... copy-paste error :-)



src/imageformats/xcf_compressed_p.h (lines 1 - 8)
<https://git.reviewboard.kde.org/r/125590/#comment59993>

    Same comment as for license header in cpp file


- Alex Merry


On Oct. 25, 2015, 3:05 a.m., Tudor PRISTAVU wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125590/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2015, 3:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Alex Merry.
> 
> 
> Bugs: 126072
>     https://bugs.kde.org/show_bug.cgi?id=126072
> 
> 
> Repository: kimageformats
> 
> 
> Description
> -------
> 
> Add support for GIMP's compressed xcf files (xcfgz / xcfbz2).
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 518c9b25d9e93286494984ed360ab88f30106346 
>   src/imageformats/CMakeLists.txt 8cf8d54bd0186521a5a71fdacae5ed3196e83b05 
>   src/imageformats/xcf_compressed.cpp PRE-CREATION 
>   src/imageformats/xcf_compressed.desktop PRE-CREATION 
>   src/imageformats/xcf_compressed.json PRE-CREATION 
>   src/imageformats/xcf_compressed_p.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125590/diff/
> 
> 
> Testing
> -------
> 
> Before testing, please make sure that you have image/x-compressed-xcf mime type and the system recognizes .xcfgz / .xcfbz2 files. It seems that various distributions have various problems with that.
> 
> I think the easiest (CLI) way to solve this is:
> 
> cat > /usr/share/mime/packages/xcfz.xml << EOF
> <?xml version="1.0"?>
> <mime-info xmlns='http://www.freedesktop.org/standards/shared-mime-info'>
>   <mime-type type="image/x-compressed-xcf">
>     <comment>Compressed xcf</comment>
>     <glob pattern="*.xcfgz"/>
>     <glob pattern="*.xcfbz2"/>
>   </mime-type>
> </mime-info>
> EOF
> 
> update-mime-database /usr/share/mime
> 
> 
> Thanks,
> 
> Tudor PRISTAVU
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151025/f6b3c6b9/attachment-0001.html>