Review Request: Comic Book Thumbnailer

Peter Penz peter.penz at gmx.at
Sat Oct 31 16:39:49 GMT 2009 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1983/#review2875
-----------------------------------------------------------



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2257>

    I'd suggest to add a d'elete m_comicCover' to prevent a potential memory leak.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2260>

    I'd suggest to add a m_comicCover = 0 after deleting it.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2254>

    I'd change the signature from 'const bool' to 'bool' (the parameter is passed per value). Hmm, in the code filterImages() is always called with 'false' - why did you use true as default or leave away this code path at all?



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2252>

    \'s not required



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2253>

    I'd suggest to initialize cArchiveDir with 0 immediately. I know you do it in the else-path already, but it prevents having an "uninitialized value" problem when somebody later might do some adjustments in the code.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2255>

    Memory leak: cZip is not deleted... I did not check the size of KZip, but maybe it can be allocated on stack.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2256>

    Memory leak: cTar is not freed. I did not check the size of the KTar class, but maybe it can be allocated on the stack.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2261>

    Hmm, this looks very dangerous in combination with the cast in line 160. Where is the allocation done? And where are the other entries deleted?



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2262>

    The 'if'... code + the 2 returns might be replaced by "return m_comicCover != 0"



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2263>

    \ not required



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2264>

    please use const QString& instead of QString



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2265>

    I'd suggest to allocate KTempDir on the stack instead of doing a 'new'. There are memory leaks in the lines 225 and 233.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2259>

    - I'd change to interface to use a QStringList as return value instead of using an output parameter.
    - please use const QString& instead of const QString



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2266>

    I'd suggest to rename this method: isUnrarAvailable() implies that only a check is done whether the Unrar is available, but the main intention is to fill the string. I'd suggest to use:
    'QString unrarPath() const' and to return an empty string if there was no success.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2258>

    please use const QString& and const QStringList&



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp
<http://reviewboard.kde.org/r/1983/#comment2267>

    I'm not a fan of local event loops, as the can be quite dangerous. But I think in the context of the thumbnailer it should be OK. But maybe somebody else can comment on this too...


- Peter


On 2009-10-27 16:08:52, Harsh J wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
> 
> (Updated 2009-10-27 16:08:52)
> 
> 
> Review request for Dolphin and kdelibs.
> 
> 
> Summary
> -------
> 
> Okular in kdegraphics supports viewing Comic Book Reader files such as .cbr, and .cbz. However, Dolphin does not preview them just like it previews PDF or other documents.
> 
> Since these comic book files are merely archives of various types, I decided to write a ThumbCreator module that performs the job of previewing these files. The ComicCreator class lists the files, sorts the images among them and then extracts the comic's cover image (usually the first) and hands it over to the thumbnail service.
> 
> For .cbz and .cbt I've used the KArchive subclasses - KZip and KTar to extract.
> (Bug 204195 : Okular doesn't support .cbt as of now - will work on this next)
> 
> For .cbr (the RAR type file) I've used the 'unrar'/'unrar-nonfree' executable (by Eugene Roshal, RARLabs) to extract. This is due to the fact that the free unrar utility does not support most of the new RAR version files.
> 
> This is my first contribution to the KDE Project and I've tried to conform to all Policies:
> 
>  * Code reports no issue with Krazy2All checker.
>  * Code structure, whitespace, etc. is as per the policies of KDE.
>  * License is the new GPL 2 or higher license (as per KDE e.V.)
>  * Followed the existing CMakeLists.txt file format.
> 
> I'm yet to receive my (applied) svn account.
> 
> Have attached screen-shots of it in action.
> 
> Awaiting your feedback :)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1040930 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comicbookthumbnail.desktop PRE-CREATION 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.h PRE-CREATION 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1983/diff
> 
> 
> Testing
> -------
> 
> * Compiles without any issues.
> 
> * Thumbnailing works on comic book files of .cbz, .cbt and .cbr types.
>   (Maximum File Size limit needs to be raised a little via Dolphin in some cases for the thumb service to work.)
> 
> * Tested thumbnailing ~300 files (mix of .CBZ, .CBT and .CBR) in a single folder at large preview size.
> 
> * Tested for memory leaks. For 300~ files, each of the spawned kio_thumbnail process takes only ~30 MB Memory
>   resource at maximum (+10 MB Shared, at max). Have seen 3 spawn at the most, 2 usually.
> 
> * Tested with both Dolphin and the Preview Plasmoid.
> 
> 
> Screenshots
> -----------
> 
> Dolphin - Large Comic Previews
>   http://reviewboard.kde.org/r/1983/s/240/
> Dolphin - Normal or Small Comic Previews
>   http://reviewboard.kde.org/r/1983/s/241/
> Dolphin - File Properties Preview
>   http://reviewboard.kde.org/r/1983/s/242/
> 
> 
> Thanks,
> 
> Harsh
> 
>

More information about the kde-core-devel mailing list