Review Request: Comic Book Thumbnailer

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

Review request for Dolphin and kdelibs.


Summary
-------

Okular in kdegraphics supports viewing Comic Book Reader files such as .cbr, .cbt 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

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

(Updated 2009-10-27 09:56:04.893412)


Review request for Dolphin and kdelibs.


Changes
-------

Okular and .cbt.


Summary (updated)
-------

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

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

(Updated 2009-10-27 16:03:21.632858)


Review request for Dolphin and kdelibs.


Changes
-------

Fixed a minor overlook of the application/x-tar type (uncompressed tar archives). And also a double free occurance.


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 (updated)
-----

  /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

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

(Updated 2009-10-27 16:08:52.348860)


Review request for Dolphin and kdelibs.


Changes
-------

Previous diff broke. My bad, forgot to commit.


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 (updated)
-----

  /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

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

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


Thanks a lot for the patch! I've had a look at the code and have added some comments. It would be very helpful if somebody else could also have a look at the usage of the local event loop...

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

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

(Updated 2009-10-31 17:10:40.278392)


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

> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 94
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line94>
> >
> >     I'd suggest to add a m_comicCover = 0 after deleting it.

Done.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 125
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line125>
> >
> >     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.

Done.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 179
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line179>
> >
> >     \ not required

Removed.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 194
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line194>
> >
> >     please use const QString& instead of QString

Done.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 113
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line113>
> >
> >     \'s not required

Removed.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 170
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line170>
> >
> >     The 'if'... code + the 2 returns might be replaced by "return m_comicCover != 0"

Done :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 141
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line141>
> >
> >     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.

As above, re-wrote to use KArchive itself.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 169
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line169>
> >
> >     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?

Removed the line, a mistake on my part to call free upon it! :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 218
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line218>
> >
> >     I'd suggest to allocate KTempDir on the stack instead of doing a 'new'. There are memory leaks in the lines 225 and 233.

Changed as suggested :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 307
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line307>
> >
> >     please use const QString& and const QStringList&

Ok, made a note of this :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 243
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line243>
> >
> >     - 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

- Changed. I basically kept it that way so that it looked similar to getArchiveFileList, but guess its better this way.
- Done :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 131
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line131>
> >
> >     Memory leak: cZip is not deleted... I did not check the size of KZip, but maybe it can be allocated on stack.

Re-wrote this whole 'type' check to use the base KArchive class. Allocating without "new" doesn't seem to work, since using the directory() it returns leads to a crash. Deleted new cArchive variable after use.


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 252
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line252>
> >
> >     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.

Renamed the method and its usage as per suggestion :)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 99
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line99>
> >
> >     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?

I was testing which approach was better (case sensitive or non), and had created this logic branch. Removed the branching now, since non-sensitive is best for cover files (Tested so far with over 1000 files - all showed up covers and not any other image using this approach).


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 331
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line331>
> >
> >     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...

Ok, is there an alternative to this approach?

This code segment (the whole startProcess() one) was borrowed from Okular's comicbook-generators, by Tobias Koenig <tokoe@...>. Have also added that credit in each source file.

Thank you for the comprehensive review, Peter. Learned a lot! :)

New diff (r4) attached for changes. Awaiting a second review, for still possible leaks and the local event loop :)

P.s. (Also removed DrawFrame since it didn't look so good, now flag is set to None)


> On 2009-10-31 16:40:04, Peter Penz wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 56
> > <http://reviewboard.kde.org/r/1983/diff/3/?file=13439#file13439line56>
> >
> >     I'd suggest to add a d'elete m_comicCover' to prevent a potential memory leak.

I initially put the delete statement there, but it didn't seem to work. The memory usage ran high (over 200+ MB per kio_thumbnail process) when I enabled previews for around 300 files in a test folder. Doing it in the ComicCreator::create() function itself worked better, and memory usage was limited.

If I should *still* add the delete here and not in ComicCreator::create(), let me know, 'cause maybe it varies for others?


- Harsh


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


On 2009-10-31 17:10:40, Harsh J wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
>
> (Updated 2009-10-31 17:10:40)
>
>
> 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
>
>

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

(Updated 2009-10-31 21:37:21.995063)


Review request for Dolphin and kdelibs.


Changes
-------

Updated sources to contain changes suggested by Peter Penz's review.

(Unrel: Imported the source code to kdereview (after fixing the changes in playground), since I got my svn account confirmed.)


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 (updated)
-----

  /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1043145
  /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

[Ingo Klöcker]

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


Please use shared pointers instead of C-pointers whereever it makes sense. Now that Qt finally provides QSharedPointer (new in Qt 4.5) and QScopedPointer (new in Qt 4.6; see http://doc.qt.nokia.com/4.6-snapshot/qscopedpointer.html) there's really no reason anymore for risking memory leaks by using C-pointers which need to be deleted explicitly.


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

    Use QScopedPointer<QImage> instead of a C-pointer.



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

    Initialize all non-object members here, i.e. all those C-pointer variables.



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

    Use QScopedPointer<KArchive> instead of a C-pointer because then you won't have to think about deleting cArchive everywhere where it might be necessary.



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

    Here you leak cArchive.



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

    Here you also leak cArchive.



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

    And yet another leak of cArchive.


- Ingo


On 2009-10-31 21:37:21, Harsh J wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
>
> (Updated 2009-10-31 21:37:21)
>
>
> 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 1043145
>   /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
>
>

> On 2009-10-31 22:01:28, Ingo Klöcker wrote:
> > Please use shared pointers instead of C-pointers whereever it makes sense. Now that Qt finally provides QSharedPointer (new in Qt 4.5) and QScopedPointer (new in Qt 4.6; see http://doc.qt.nokia.com/4.6-snapshot/qscopedpointer.html) there's really no reason anymore for risking memory leaks by using C-pointers which need to be deleted explicitly.

A few queries before I try changing the variables to QScopedPointer.

* To initialize a QImage, I'd have to do m_comicCover.reset(new QImage()) in the code, yes?
* Must I change all the current C-pointer types (QEventLoop, KArchiveDirectory etc.) to this type?

Would be helpful if I can understand when to do this and when not to..


- Harsh


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


On 2009-10-31 21:37:21, Harsh J wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
>
> (Updated 2009-10-31 21:37:21)
>
>
> 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 1043145
>   /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
>
>

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

(Updated 2009-11-02 17:38:59.535410)


Review request for Dolphin and kdelibs.


Changes
-------

Applied review requested changes by Ingo Klöcker.

* Changed cArchive and m_comicCover C-pointers to QScopedPointer type.
* (Unrel: Added credits to Tobias Koenig for his process-execution code.)


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 (updated)
-----

  /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1043899
  /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

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

(Updated 2009-11-06 15:30:18.648102)


Review request for Dolphin and kdelibs.


Changes
-------

Made m_process use QScopedPointer class as well, after reading more upon it. Tested again with cbr/cbz/cbt files and it works smooth.


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 (updated)
-----

  /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

> On 2009-10-31 22:01:28, Ingo Klöcker wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 157
> > <http://reviewboard.kde.org/r/1983/diff/4/?file=13590#file13590line157>
> >
> >     And yet another leak of cArchive.

I've made the requested changes. Is the code alright now, that I commit it to the trunk under kdebase/runtime/kioslave/thumbnail for 4.4?


- Harsh


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


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

> On 2009-10-31 22:01:28, Ingo Klöcker wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.cpp, line 157
> > <http://reviewboard.kde.org/r/1983/diff/4/?file=13590#file13590line157>
> >
> >     And yet another leak of cArchive.
>
>  wrote:
>     I've made the requested changes. Is the code alright now, that I commit it to the trunk under kdebase/runtime/kioslave/thumbnail for 4.4?

>From my point of view the code is OK now and the patch can be committed. However I'd suggest to wait for Ingo's final OK.


- Peter


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


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

[Ingo Klöcker]

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

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

(Updated 2009-11-07 16:29:57.978624)


Review request for Dolphin and kdelibs.


Changes
-------

Applied more requested changes :)


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 (updated)
-----

  /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

> On 2009-11-06 21:50:25, Ingo Klöcker wrote:
> > 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.

Agreed, and changed! =)


> On 2009-11-06 21:50:25, Ingo Klöcker wrote:
> > /trunk/KDE/kdebase/runtime/kioslave/thumbnail/comiccreator.h, line 37
> > <http://reviewboard.kde.org/r/1983/diff/6/?file=13828#file13828line37>
> >
> >     A forward declaration of QEventLoop (as for KArchiveDirectory) should suffice.


- Harsh


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


On 2009-11-07 16:29:57, Harsh J wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
>
> (Updated 2009-11-07 16:29:57)
>
>
> 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
>
>

[Ingo Klöcker]

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


Looks good now. Only one mandatory change and one suggested change to go...


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

    You have to check whether entries is empty. Otherwise, entries[0] will cause an assertion.



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

    You could use QImage::load() instead of QImage::fromData(), i.e.
   
    QImage cover;
    cover.load(cUnrarTempDir.name() + entries[0]);
   


- Ingo


On 2009-11-07 16:29:57, Harsh J wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1983/
> -----------------------------------------------------------
>
> (Updated 2009-11-07 16:29:57)
>
>
> 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
>
>

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

(Updated 2009-11-08 07:42:14.374882)


Review request for Dolphin and kdelibs.


Changes
-------

* Fixed the overlooked empty-list check for the archive method.
* Made QImage load the file directly.


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 (updated)
-----

  /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1046203
  /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