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