Review Request: 4.5 Feature: ePub Book Thumbnailer

[David Faure]

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


SAX isn't my preferred way of doing it, QXmlStreamReader leads to much simpler and readable code, with less need to keep state in member variables. However if you want to keep the SAX code, I don't have any objections.

On the other hand, the overuse of QScopedPointer is both very confusing, and wrong; some of this code obviously leads to double deletions, since the instances of KArchiveFile and KArchiveDirectory are owned by KZip and deleted by it. I don't see any need for QScopedPointer in this code, see detailed comments below.


/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.h
<http://reviewboard.kde.org/r/3271/#comment4139>

    A simple member variable "KZip mEPubFile;" would be better and simpler.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.h
<http://reviewboard.kde.org/r/3271/#comment4140>

    And this one should be a simple pointer, since the KArchiveDirectory* is owned by KZip.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.cpp
<http://reviewboard.kde.org/r/3271/#comment4141>

    ~ ? I guess you mean ! instead, for boolean negation, rather than bitwise negation.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.cpp
<http://reviewboard.kde.org/r/3271/#comment4142>

    Make that just OPFHandler opfHandler. There is no need to use QScopedPointer when you can just create the instance onf the stack.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.cpp
<http://reviewboard.kde.org/r/3271/#comment4143>

    Simple pointer, since owned by the archive.



/trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.cpp
<http://reviewboard.kde.org/r/3271/#comment4144>

    should be on the stack


- David


On 2010-03-12 19:08:23, Harsh J wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3271/
> -----------------------------------------------------------
> 
> (Updated 2010-03-12 19:08:23)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This feature addition to kdebase aims to provide thumbnails of ePub book covers.
> 
> Early-message: This is my first try at XML parsing (I've used the SAX approach), so do give me pointers/tips on doing it better wherever possible :-)
> 
> I've written code to:
>   * Unpack the ePub (essentially a proper-structured ZIP file).
>   * Open and parse the OPF file to find the 'cover page' entry (x/html), done by:
>     ** Keeping store of all available 'items' in the 'manifest' with their IDs and Locations.
>     ** Matching the first 'idref' ID entry of the book's 'spine' (practically the cover page of the book)
>        to one of those 'items' and thereby getting its Location.
>   * Parse this HTML to get the first img element's src, if it exists.
>   * Use the image file got, to generate the cover preview.
> 
> There is also a 'reference' tag that supports the special 'type' attribute for 'covers' (as per the new spec), but most books seem to lack that thing and the current approach works for both of those type of books. If this element also needs to be checked, do let me know.
> 
> References I used:
>   * The ePub format OPF spec: http://www.idpf.org/2007/opf/OPF_2.0_0.984_draft.html
>   * A simple ePub construction guide: http://www.hxa.name/articles/content/epub-guide_hxa7241_2007.html
>   * A few free ePub files (with proper covers) can be found on
>     ** http://books.google.com (look under classics, or ones with full free previews) and,
>     ** http://www.web-books.com/ (most are free I think)
> 
> Feature entry has also been added to the 4.5 Feature Plan (along with FictionBook docs, which is coming up shortly).
> 
> P.s. Shall I also commit this to /trunk/kdereview?
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/CMakeLists.txt 1102437 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.h PRE-CREATION 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubcreator.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/runtime/kioslave/thumbnail/epubthumbnail.desktop PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3271/diff
> 
> 
> Testing
> -------
> 
> Tested with about 4 ePub files only, 1 sourced from Google Books (public domain) and 3 from web-books.com.
> 
> I'm looking for other good cover-included free sources of ePub books, but yet to find any (do test with some sources you use). Many places add the cover image but don't link it within the cover page or anywhere else (even the OPF).
> 
> 
> Screenshots
> -----------
> 
> ePub Thumbnails in the wonderful Dolphin File Manager
>   http://reviewboard.kde.org/r/3271/s/328/
> 
> 
> Thanks,
> 
> Harsh
> 
>