Review Request 111773: Add api for writing ODF that is generated from the ODF RNG file

Sun Aug 4 18:57:00 BST 2013

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

Meh, too late, committed while I was giving it finally also a look.

Please still pick up the following comments and do a fix-up commit:

devtools/CMakeLists.txt
<http://git.reviewboard.kde.org/r/111773/#comment27394>

    Please turn rng2cpp into a product.
    Means you add a product definition for this tool and wrap the call add_subdirectory(rng2cpp) into the proper conditions.

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27392>

    Don't use toAscii/fromAscii, it's behaviour is globally influenced by QTextCodec::setCodecForCStrings(), so not predictable.

    Also the output should rather be converted using toLocal8Bit().

    Usually in such places qPrintable(yourStringVar) is used for a short version, see http://qt-project.org/doc/qt-4.8/qtglobal.html#qPrintable

    So here it would be
    Q_ASSERT_X(cond,"",(qPrintable(what))
    at least if all what can be implicitely converted to QString, which should be the case

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27393>

    qPrintable(...)

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27396>

    const QDomElement& e ?

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27397>

    const QDomElement &e ?

    Also all methods below

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27399>

    const QString& url

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27400>

    qPrintable(err)

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27425>

    isEmpty()

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27422>

    better isEmpty() than isNull(), unless that difference is important here

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27426>

    r.args.chop(2) is easier to read

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27424>

    isEmpty()

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27423>

    better isEmpty() here

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27427>

    r.args.chop(2);

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27428>

    isEmpty()

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27421>

    Seems not needed. A null QString is also an empty QString. And the result of getPrefix(...) seems only tested on isEmpty().

    If there is a real reason, please add a comment, as it's not obvious.

devtools/rng2cpp/rng2cpp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27420>

    prefix2.clear();
    instead, for consistency with rest of Calligra code where this optimized solution is used.

filters/sheets/excel/import/excelimporttoods.cc
<http://git.reviewboard.kde.org/r/111773/#comment27418>

    {} also around single statements
    Or what about
                        span.set_text_style_name(cell->format().font().subscript() ? subScriptStyle : superScriptStyle);

    At least to me makes it better to read that the condition is just about the parameter, not also the call

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27409>

    Calligra code style: {} also around single statements

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27415>

    make ch const, so reuse later is safer

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27414>

    {}

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27412>

    if {
       ...
    }

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27413>

    {}

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27410>

    Please add {} 

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27411>

    Please add {}

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27416>

    Why not "str += ch;"?
    Saves new lookup of text[i]

filters/stage/powerpoint/PptToOdp.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27417>

    {}

libs/odf/writeodf/helpers.h
<http://git.reviewboard.kde.org/r/111773/#comment27407>

    Copyright header missing!

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27406>

    Copyright header missing!

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27408>

    drop the QtCore/, will help the port to Qt5

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27401>

    Usually in Calligra code all member vars are listed last, at the end of the class declaration. Please follow that here as well.

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27402>

    So is it? Or why did you implement it? Please be more clear in this comment here.

    Why just not prevent the copy constructor, by listing it in the class as private, as usual?

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27405>

    This method can result in data-loss, no? 

    Passing quint64 value to QString::number(long) will not work on 32-bit systems, as long is only 32bit there. And possibly on Win64 as well IIRC. (Also long is signed).

    So the API of this method promises more then it can.

    Please fix!

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27404>

    please add a comment why lists are just written as "". Does not make sense to me

libs/odf/writeodf/odfwriter.h
<http://git.reviewboard.kde.org/r/111773/#comment27403>

    so T is always just passed by value? what kind of Ts are expected here?

- Friedrich W. H. Kossebau

On Aug. 4, 2013, 5:43 p.m., Jos van den Oever wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111773/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2013, 5:43 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This patch is also available in the branch libs-writeodf-vandenoever-2.
> 
> Two years ago I wrote an initial version of this patch and a detailed discussion on the mailing list [1] followed. The main objections to the patch have been dealt with (see below).
> Most of this new version was written at Akademy in Bilbao.
> 
> Very short summary of the patch:
>  This patch should help everybody, young and old, with coding C++ for writing ODF and make errors easier to catch.
>  The OpenDocument Format specification is published with a Relax NG file that specifies the XML format. This file can be used to check if ODF files are valid. It can also be used to generate a C++ API headers. This is what this patch does.
> 
> Example:
>  Instead of writing:
> ==
>   xmlWriter->startElement("text:p");
>   xmlWriter->addAttribute("text:style-name", "italic");
>   xmlWriter->startElement("text:p");
>   xmlWriter->addAttribute("text:style-name", "bold");
>   xmlWriter->addTextNode("Hello World!");
>   xmlWriter->endElement();
>   xmlWriter->endElement();
> ==
> you can write:
> ==
>   text_p p(xmlWriter);
>   p.set_text_style_name("italic");
>   text_span span(p.add_text_span());
>   span.set_text_style_name("italic");
>   span.addTextNode("Hello World!");
> ==
> 
> Some advantages:
>  - autocompletion when coding: faster coding
>  - tag and attribute names are not strings but class and function names: less errors
>  - nesting is checked by the compiler
>  - you write to elements (span, p), not xmlwriter: easier to read
>  - required attributes are part of the element constructor
> 
> Implementation considerations: 
>  - Calligra is large, so the generated code mixes well with the use of KoXmlWriter and porting can be done in small steps.
>  - class and function names are similar to the xml tags with ':' and '-' replaced by '_'.
>  - stack based: no heap allocations
>  - only header files: all code will inline and have low impact on runtime
>  - modular: one header file per namespace to reduce compile overhead
>  - code generator is Qt code, part of Calligra and runs as a build step
> 
> Not in this patch (and places where you can help in the future):
>  - generate enumerations based on Relax NG
>  - check data type for attributes (you can still write "hello" to an integer attribute)
>  - complete port of Calligra to the generated code
>  - improved speed by using static QString instead of const char*
> 
> Provided solutions to previously raised issues:
>  - modular headers to reduce compile overhead
>  - function "end()" to optionally close an element before it goes out of scope
>  - use structure of Relax NG file to reduce header sizes by inheritance from common groups
>  - provide most KoXmlWriter functionality safely through the element instances
>  - closing elements is now automatic at a slight runtime overhead
>  
> 
> [1] http://lists.kde.org/?t=130768700500002
> 
> 
> Diffs
> -----
> 
>   devtools/CMakeLists.txt 15008fb 
>   devtools/rng2cpp/CMakeLists.txt PRE-CREATION 
>   devtools/rng2cpp/rng2cpp.cpp PRE-CREATION 
>   filters/libmso/CMakeLists.txt 6bc145f 
>   filters/libmso/shapes.cpp 073e061 
>   filters/libmso/shapes2.cpp 0f0b906 
>   filters/sheets/excel/import/CMakeLists.txt 2466218 
>   filters/sheets/excel/import/excelimporttoods.cc de788d4 
>   filters/stage/powerpoint/PptToOdp.h 8d85c1f 
>   filters/stage/powerpoint/PptToOdp.cpp 425ac33 
>   libs/kotext/KoInlineNote.cpp 6faa9a9 
>   libs/odf/CMakeLists.txt 8549ace 
>   libs/odf/writeodf/CMakeLists.txt PRE-CREATION 
>   libs/odf/writeodf/README.txt PRE-CREATION 
>   libs/odf/writeodf/helpers.h PRE-CREATION 
>   libs/odf/writeodf/odfwriter.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/111773/diff/
> 
> 
> Testing
> -------
> 
> Opened several ppt and xls files.
> Checked ppt conversion for two files and checked that XML was equivalent.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

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