Review Request 111773: Add api for writing ODF that is generated from the ODF RNG file
Jos van den Oever
jos at vandenoever.info
Sun Aug 4 19:48:17 BST 2013
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > 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:
Thanks for the thorough look. You caught one bug and quite some style issues.
wrt to the product remark, you seem more knowledgable on the subject and i dont want to learn more cmake folklore.
Pushing as a fixup.
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > devtools/rng2cpp/rng2cpp.cpp, line 959
> > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045line959>
> >
> > isEmpty()
A constant may be "".
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > devtools/rng2cpp/rng2cpp.cpp, line 962
> > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045line962>
> >
> > better isEmpty() than isNull(), unless that difference is important here
why is it better?
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > devtools/CMakeLists.txt, line 10
> > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176043#file176043line10>
> >
> > 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.
If you think this is important, you can do so, but I do not see the benefit. The dependencies are set correctly.
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > libs/odf/writeodf/odfwriter.h, lines 67-70
> > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058line67>
> >
> > 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!
on 32-bit qlonglong would be automatically used.
> On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > libs/odf/writeodf/odfwriter.h, line 96
> > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058line96>
> >
> > so T is always just passed by value? what kind of Ts are expected here?
T is a pointer for both uses.
void addCompleteElement(const char* cstr);
void addCompleteElement(QIODevice* dev);
- Jos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111773/#review37065
-----------------------------------------------------------
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/bee751b4/attachment.htm>
More information about the calligra-devel
mailing list