> 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 > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
