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

Thorsten Zachmann t.zachmann at zagge.de
Sat Aug 3 05:46:40 BST 2013


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



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

    In calligra we usually use m_ as prefix for member variables. Is there a good reason why you did not do it like that?



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

    Maybe put it inline in the class as the != operator.



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

    Shouldn't that be isDefined?



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

    How about using Q_ASSERT?



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

    In calligra we put the return value in front of the function and not in the line above.



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

    In calligra we put the opening bracket of the function into the next line.



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

    If this should stay it should be put inside #if DEBUG
    #endif



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

    If this should stay it should be put inside #if DEBUG
    #endif



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

    Please use Q_ASSERT



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

    Please use Q_ASSERT



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

    This typedef seems to be unused.



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

    Should those methods not use the QChar version of the function



filters/libmso/shapes.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27101>

    Can we use CamelCase for the created classes and function method as we do in Calligra? 



filters/libmso/shapes.cpp
<http://git.reviewboard.kde.org/r/111773/#comment27102>

    This uses a totally different format. How is decided when this format or the other one is used?



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

    What is the difference to page-number?



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

    Is this always called with a cell? Otherwise  the test should be back.


- Thorsten Zachmann


On Aug. 2, 2013, 11:59 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. 2, 2013, 11:59 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/20130803/7701edb9/attachment.htm>


More information about the calligra-devel mailing list