Add api for writing ODF that is generated from the ODF RNG file

Review Request #111773 - Created July 28, 2013 and submitted

Information
Jos van den Oever
calligra
libs-writeodf-vandenoever-2
Reviewers
calligra
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
Opened several ppt and xls files.
Checked ppt conversion for two files and checked that XML was equivalent.

Issues

  • 6
  • 47
  • 9
  • 62
Description From Last Updated
In calligra we usually use m_ as prefix for member variables. Is there a good reason why you did not ... Thorsten Zachmann Thorsten Zachmann
Shouldn't that be isDefined? Thorsten Zachmann Thorsten Zachmann
Please use Q_ASSERT Thorsten Zachmann Thorsten Zachmann
Please use Q_ASSERT Thorsten Zachmann Thorsten Zachmann
Can we use CamelCase for the created classes and function method as we do in Calligra? Thorsten Zachmann Thorsten Zachmann
This uses a totally different format. How is decided when this format or the other one is used? Thorsten Zachmann Thorsten Zachmann
Thorsten Zachmann
Inge Wallin
Thorsten Zachmann
Inge Wallin
Jos van den Oever
Review request changed

Status: Closed (submitted)

Friedrich W. H. Kossebau
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:
  1. 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.
devtools/CMakeLists.txt (Diff revision 5)
 
 
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.
  1. If you think this is important, you can do so, but I do not see the benefit. The dependencies are set correctly.
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
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 (Diff revision 5)
 
 
qPrintable(...)
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
const QDomElement& e ?
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
const QDomElement &e ?

Also all methods below
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
const QString& url
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
qPrintable(err)
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
isEmpty()
  1. A constant may be "".
    
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
better isEmpty() than isNull(), unless that difference is important here
  1. why is it better?
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
r.args.chop(2) is easier to read
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
isEmpty()
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
better isEmpty() here
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
r.args.chop(2);
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
isEmpty()
devtools/rng2cpp/rng2cpp.cpp (Diff revision 5)
 
 
 
 
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 (Diff revision 5)
 
 
prefix2.clear();
instead, for consistency with rest of Calligra code where this optimized solution is used.

filters/sheets/excel/import/excelimporttoods.cc (Diff revision 5)
 
 
 
 
 
{} 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 (Diff revision 5)
 
 
 
Calligra code style: {} also around single statements
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
make ch const, so reuse later is safer
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
 
{}
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
 
if {
   ...
}
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
 
{}
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
 
Please add {} 
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
Please add {}
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
Why not "str += ch;"?
Saves new lookup of text[i]
filters/stage/powerpoint/PptToOdp.cpp (Diff revision 5)
 
 
{}
libs/odf/writeodf/helpers.h (Diff revision 5)
 
 
Copyright header missing!
libs/odf/writeodf/odfwriter.h (Diff revision 5)
 
 
Copyright header missing!
libs/odf/writeodf/odfwriter.h (Diff revision 5)
 
 
 
 
drop the QtCore/, will help the port to Qt5
libs/odf/writeodf/odfwriter.h (Diff revision 5)
 
 
 
 
 
 
 
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 (Diff revision 5)
 
 
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 (Diff revision 5)
 
 
 
 
 
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!
  1. on 32-bit qlonglong would be automatically used.
libs/odf/writeodf/odfwriter.h (Diff revision 5)
 
 
please add a comment why lists are just written as "". Does not make sense to me
libs/odf/writeodf/odfwriter.h (Diff revision 5)
 
 
so T is always just passed by value? what kind of Ts are expected here?
  1. T is a pointer for both uses.
        void addCompleteElement(const char* cstr);
        void addCompleteElement(QIODevice* dev);
Loading...