Review Request: Fix Mobi format problems

[Inge Wallin]

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


Ok, I have a number of comments.  This is a good start but not quiet ready for merge yet. See below.


filters/words/mobi/CMakeLists.txt
<http://git.reviewboard.kde.org/r/107526/#comment17463>

    If you're really sure you won't need it anymore (which you should be at this stage) you should remove it rather than comment it out.



filters/words/mobi/MobiFile.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17464>

    Argh!  Magic numbers!
    
    Please add some form of comment what these numbers are about.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17465>

    Without actually knowing what this number is about, it feels that it would be good to have a defined constant for it instead of just using the number directly.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17466>

    Our coding standar is to:
     - Have an empty line between methods
     - have the initializer colon on the next line.
    
    Also look for other places where this happens.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17467>

    I think compressedTextSize would be more correct.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17468>

    Coding standard says "spaces around binary operators".



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17469>

    If you comment out a larger piece of code it's better to do it with:
    #if 0
    #endif
    than using comments on every line. It's very easy to miss one line either when commenting or uncommenting it back.
    
    That said, if you're done with the code and won't need it anymore (as like when you're registering a review request) then it's better to remove it altogether.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17470>

    Remove code not needed anymore.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17471>

    calligra-suite.org is deprecated.  Use calligra.org instead.



filters/words/mobi/MobiHeaderGenerator.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17472>

    Hmm, what strange date format is this?  I know yyyy-MM-dd (the ISO format) and dd/MM/YYYY (the US format).  I'd prefer the ISO format but in any case this is non-standard (or perhaps not in Iran?).



filters/words/mobi/OdtMobiHtmlConverter.cpp
<http://git.reviewboard.kde.org/r/107526/#comment17473>

    Just a note in case somebody else comments on this...
    
    This file contains a number of places where code is commented away. In this particular file I would prefer if it's actually *not* removed since I want to refactor the parsing of ODT and then bring it back into one parser. That would be simplified by having it all in place.


- Inge Wallin


On Nov. 30, 2012, 7:34 p.m., mojtaba shahi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107526/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2012, 7:34 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This fixes are becuse that Kindle device couldn't open our Mobi format.
> The first problem was about html text content that i used for mobi, it was not standard, sor for have a standard Mobi html i create OdtMobiHtmlConverter
> a copy of OdtHtmlConverter. I changed it t have a one, for that i neede inline attributes in elements no CSS, and i think we can merge it to OdtHtmlConverter.
> 
> Now with these fixes we support images, bookmarks (not very well), And Kindle can open our mobi correct without any problem.
> 
> 
> Diffs
> -----
> 
>   filters/words/mobi/CMakeLists.txt 88633a7 
>   filters/words/mobi/MobiFile.cpp 29c3551 
>   filters/words/mobi/MobiHeaderGenerator.h 7864620 
>   filters/words/mobi/MobiHeaderGenerator.cpp 378a01a 
>   filters/words/mobi/OdtMobiHtmlConverter.h PRE-CREATION 
>   filters/words/mobi/OdtMobiHtmlConverter.cpp PRE-CREATION 
>   filters/words/mobi/PalmDocCompression.h 2c7ed71 
>   filters/words/mobi/PalmDocCompression.cpp 4df0067 
>   filters/words/mobi/exportmobi.cpp 455b526 
> 
> Diff: http://git.reviewboard.kde.org/r/107526/diff/
> 
> 
> Testing
> -------
> 
> I have tested ebooks in Kidle Desktop Previewer for windows.
> And also tested on Kidle device too, no problem :)
> There is a mobi that i have converted in attach file, it is about mobi format but is not completed yet. :) 
> 
> 
> Thanks,
> 
> mojtaba shahi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20121202/7fd56016/attachment.htm>