Review Request: Add support for EPUB 2.0.1

[Thorsten Zachmann]

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



filters/words/epub/exportepub2.h
<http://git.reviewboard.kde.org/r/105820/#comment13051>

    Please remove the space at the start of the line



filters/words/epub/exportepub2.h
<http://git.reviewboard.kde.org/r/105820/#comment13052>

    Please move the implementation to the cpp file.



filters/words/epub/exportepub2.h
<http://git.reviewboard.kde.org/r/105820/#comment13053>

    If this is not be used it should be removed for now.



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13054>

    There should be no blank after the (



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13055>

    odfStore should be deleted before the return to avoid a memory leak



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13056>

    I think storing the images used during reading the content/styles into a structure and copying after reading that would be much better. Also I think it would be a good idea to convert image formats to e.g. png that are not supported by epub. e.g. svm or wmf...
    



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13057>

    The pointer should not be deleted here as it is already done in the convert method.



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13059>

    Instead of using QDom... classes KoXml... classes should be used.



filters/words/epub/exportepub2.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13060>

    Replace QDom... classes by KoXml... classes.



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13061>

    Replace QDom... by KoXml... classes.



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13063>

    Are you sure you are parsing all styles (auto and named styles)?



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13062>

    You parse different properties here then for the styles in content.xml is this correct?
    Maybe the styles parsing should be done in a function that handles the styles for both files so that it is reused.



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13064>

    There seems to be no need to use new here. The objects should be created on the stack as it is faster.



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13065>

    Replace QDom... classes by KoXml... ones



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13066>

    I think passing the KoXmlWriter as reference makes more sense as it is always needed.



filters/words/epub/htmlconvert.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13067>

    Please remove as it seems to be a left over.



filters/words/epub/libepub/EpubFile.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13068>

    I think you don't need to close when open failed.



filters/words/epub/libepub/EpubFile.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13069>

    This can be simplified to:
    
    delete epubStore;
    return status;



filters/words/epub/libepub/EpubFile.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13070>

    There is no need to create the writer in the heap. Please create it on the stack then it is also not leaked.
    
    That is true for all places where this is done.



filters/words/epub/libepub/EpubFile.cpp
<http://git.reviewboard.kde.org/r/105820/#comment13071>

    don't delete epubStore here as it is already done in the caller.


- Thorsten Zachmann


On Aug. 1, 2012, 9:53 p.m., Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105820/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2012, 9:53 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This patch adds support for EPUB 2.0.1, the standard format for ebooks. The current standard is version 3.0 but that is a much more extensive standard and we have to start somewhere.
> 
> Features:
>  - Text with extensive formatting
>  - Tables, even tables in tables
>  - Pictures
>  - Breaking into different html files (chapters) by having a break-before on a paragraph style.
> 
> The support is of course not perfect yet but the current state seems to be useful and it looks like a good point to get it to master so that more people can help out. 
> 
> 
> Diffs
> -----
> 
>   filters/words/CMakeLists.txt a07793a 
>   filters/words/epub/CMakeLists.txt PRE-CREATION 
>   filters/words/epub/TODO PRE-CREATION 
>   filters/words/epub/exportepub2.h PRE-CREATION 
>   filters/words/epub/exportepub2.cpp PRE-CREATION 
>   filters/words/epub/htmlconvert.h PRE-CREATION 
>   filters/words/epub/htmlconvert.cpp PRE-CREATION 
>   filters/words/epub/libepub/EpubFile.h PRE-CREATION 
>   filters/words/epub/libepub/EpubFile.cpp PRE-CREATION 
>   filters/words/epub/words_epub2_export.desktop PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105820/diff/
> 
> 
> Testing
> -------
> 
> We have a number of test files and the attached file contains all the features that are supported. We have also used the EPUB validator a lot. That can be found at http://validator.idpf.org/. 
> 
> 
> Thanks,
> 
> Inge Wallin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120802/560f6e34/attachment.htm>