Review Request: Comic Book Thumbnailer

Ingo Klöcker kloecker at kde.org
Fri Nov 6 21:50:08 GMT 2009


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


Sorry. I should have done a thorough review before. The usage of QScopedPointer is okay, but I found a few more things that could be changed to improve the code.

Apart from the remarks below I suggest to make extractArchiveImage() and extractRARImage() return the cover image instead of a bool. This way you do not need a member variable for the cover image. Moreover, this change will make the code more readable because the reader of the code will not be surprised by extractArchiveImage() and extractRARImage() changing a member variable. It's good practice to avoid methods with side effects (i.e. methods that change member variables) as much as possible.


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

    Not that important (because this header file will not be included by any other cpp files than the cpp file containing the implementation), but a forward declaration of KArchiveDirectory, i.e.
    
    class KArchiveDirectory;
    
    should suffice.



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

    A forward declaration of QEventLoop (as for KArchiveDirectory) should suffice.



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

    const QString& path, const QString& type
    
    Even better: Make type an enum.
    



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

    Initialization of m_comicCover and m_process is not really necessary because they are now QScopedPointer.



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

    This is unnecessary.



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

    There's no need for prepending the method name with "ComicCreator::".



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

    ditto



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

    ditto



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

    I would simply
    
    return false;
    
    here. Or do you want to get the debug message also in this case?



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

    Here you could
    
    return true;
    
    It's probably a bit better readable since the reader doesn't wonder whether result could be false at this point.



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

    This could be done in one loop. If entry is an image then add it to entryMap. This way you will automatically get rid of the non-images because you do not put them into the entryMap.



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

    I suggest to scratch this line and declare the variable further down where cArchiveDir is used for the first time.



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

    The (0) is unnecessary.



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

    After this call you should check whether entries is empty. Otherwise, entries[0] below will cause an assertion.



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

    Remove "ComicCreator::".



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

    const QString unrar = ...
    



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

    After this call you should check whether entries is empty. Otherwise, entries[0] below will cause an assertion.



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

    This check makes no sense since m_comicCover will certainly not be pointing to 0.



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

    This line should be moved directly below
    coverFile.close();
    so that the temp directory is not left over.



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

    I wonder what happens if the rar file contains a file with a filename that cannot be encoded in the locally used encoding. Since we have to parse the standard output of rar there's probably not much we can do about this.


- Ingo


On 2009-11-06 15:30:18, Harsh J wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
> 
> (Updated 2009-11-06 15:30:18)
> 
> 
> 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 1044774 
>   /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