Review Request 111773: Add api for writing ODF that is generated from the ODF RNG file
Jos van den Oever
jos at vandenoever.info
Sat Aug 3 11:42:42 BST 2013
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> >
Thanks for reviewing this large patch.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > devtools/rng2cpp/rng2cpp.cpp, line 88
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line88>
> >
> > Shouldn't that be isDefined?
No, it corresponds to the <define/> element. Add docu.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > devtools/rng2cpp/rng2cpp.cpp, line 90
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line90>
> >
> > How about using Q_ASSERT?
Q_ASSERT does print a custom message. assert() here uses Q_ASSERT_X which does.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > devtools/rng2cpp/rng2cpp.cpp, lines 46-47
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line46>
> >
> > In calligra we usually use m_ as prefix for member variables. Is there a good reason why you did not do it like that?
In Calligra, I've seen quite some private classes that do not follow the m_ convention.
Since this code generator is just on cpp without a public header, I think the m_ convention is not obligatory.
Also, many members are public and personally, I find that it looks weird to call instance.m_member.
If I misunderstand the rule, I can change the code.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > filters/libmso/shapes.cpp, line 132
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174560#file174560line132>
> >
> > Can we use CamelCase for the created classes and function method as we do in Calligra?
We could, but I prefer not to. I realize that the convention in Calligra is CamelCase.
I thought it would be clearer to write text_p and add_text_p instead of textP and addTextP.
On IRC the option of using add<text::p> was discussed. That does not have as nice autocompletion though.
I propose to have a separate discussion and possibly change this in a follow-up patch.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > filters/libmso/shapes.cpp, lines 170-173
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174560#file174560line170>
> >
> > This uses a totally different format. How is decided when this format or the other one is used?
It is still possible, but discouraged, to use addAttribute, both on xmlwriter and on the tag instance.
In this particular case, the tag requires the attributes for svg:x1, svg:x2, svg:y1, svg:y2, so they must be passed to the constructor.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > filters/sheets/excel/import/excelimporttoods.cc, lines 926-929
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174563#file174563line926>
> >
> > What is the difference to page-number?
The Relax NG for this element goes like this:
<element>
<choice>
<name>text:page-count</name>
<name>text:paragraph-count</name>
<name>text:word-count</name>
<name>text:character-count</name>
<name>text:table-count</name>
<name>text:image-count</name>
<name>text:object-count</name>
</choice>
<ref name="common-field-num-format-attlist"/>
<text/>
</element>
The <element> tag has no name but a set of possible names. I did not add support for this case yet, but will do soon after the patch lands.
> On Aug. 3, 2013, 4:46 a.m., Thorsten Zachmann wrote:
> > filters/sheets/excel/import/excelimporttoods.cc, line 1274
> > <http://git.reviewboard.kde.org/r/111773/diff/1/?file=174563#file174563line1274>
> >
> > Is this always called with a cell? Otherwise the test should be back.
Yes, this file has quite some redundant tests like that.
- Jos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111773/#review36694
-----------------------------------------------------------
On Aug. 3, 2013, 10:41 a.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. 3, 2013, 10:41 a.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/957b402f/attachment.htm>
More information about the calligra-devel
mailing list