<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111773/">http://git.reviewboard.kde.org/r/111773/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have looked through this rather big patch, and I must say that in many places where these writers are used it results in easier to read code. I also like that it's possible to mix the old manual and this new automatic style of writing ODF. So in general I'm in favour of this patch and the ideas behind it.
But I think that if we want to have this in libs/odf we need more documentation. I miss a general overview of the classes and how they are supposed to be used together. I also miss other types of API docs even if it's difficult to say exactly where it should be put since we are talking about automatically generated code here. Finally, the code should follow the coding standards of Calligra. See comments below.
I also have a question: Will this be able to handle extensions to ODF, like the special tags that we write in some of our filters to create better compatibility with MS Office documents?
In conclusion I think it's very promising but not ready for merge yet.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line1" style="color: black; font-weight: bold; text-decoration: underline;">devtools/rng2cpp/rng2cpp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#include <QFile></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Missing license information.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line8" style="color: black; font-weight: bold; text-decoration: underline;">devtools/rng2cpp/rng2cpp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">8</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">//#define FULL</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think some small documentation of FULL and SUB wouldn't hurt. I try to look at the code below (void test()...) and I don't understand the purpose.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line40" style="color: black; font-weight: bold; text-decoration: underline;">devtools/rng2cpp/rng2cpp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">class</span> <span class="nc">Datatype</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Our coding standards say that the start brace for classes and functions should be in column 1.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174558#file174558line95" style="color: black; font-weight: bold; text-decoration: underline;">devtools/rng2cpp/rng2cpp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">95</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">class</span> <span class="nc">Element</span> <span class="o">:</span> <span class="k">public</span> <span class="n">RNGItem</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In general I think this style of coding without any empty lines and very little comments is very hard to read.
I think at least one empty line between classes and one between functions. Whitespace is your friend when it comes to structuring the source code. Comments too, btw.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174559#file174559line2" style="color: black; font-weight: bold; text-decoration: underline;">filters/libmso/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">${</span><span class="nv">KOODF_INCLUDES</span><span class="o">}</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thank you :)</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174560#file174560line132" style="color: black; font-weight: bold; text-decoration: underline;">filters/libmso/shapes.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void ODrawToOdf::processRectangle(const OfficeArtSpContainer& o, Writer& out)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">130</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">out</span><span class="p">.</span><span class="n">xml</span><span class="p">.</span><span class="n">startElement</span><span class="p">(</span><span class="s">"draw:enhanced-geometry"</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">121</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">draw_enhanced_geometry</span> <span class="nf">eg</span><span class="p">(</span><span class="n">rect</span><span class="p">.</span><span class="n">add_draw_enhanced_geometry</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This may be a personal thing but wouldn't it be easier if this was written:
draw_enhanced_geometry eg = rect.add_draw_enhanced_geometry();
?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174563#file174563line1309" style="color: black; font-weight: bold; text-decoration: underline;">filters/sheets/excel/import/excelimporttoods.cc</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QString currencyValue(const QString &value)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1269</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n"><span class="hl">xmlWriter</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">addAttribute</span></span><span class="p"><span class="hl">(</span></span><span class="s"><span class="hl">"</span>office<span class="hl">:</span>boolean<span class="hl">-</span>value<span class="hl">"</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n">value</span><span class="p">.</span><span class="n">asBoolean</span><span class="p">()</span> <span class="o">?</span> <span class="s">"true"</span> <span class="o">:</span> <span class="s">"false"</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1210</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n"><span class="hl">c</span></span><span class="p"><span class="hl">.</span></span><span class="n"><span class="hl">set_</span>office<span class="hl">_</span>boolean<span class="hl">_</span>value</span><span class="p"><span class="hl">(</span></span><span class="n">value</span><span class="p">.</span><span class="n">asBoolean</span><span class="p">()</span> <span class="o">?</span> <span class="s">"true"</span> <span class="o">:</span> <span class="s">"false"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This seems to be a good place to ask this question, but it's not specific to these lines:
Is it possible to add convenience functions to write a bool (for instance) so we don't have to use this ugly way:
<boolexpr> ? "true" : "false"
?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111773/diff/1/?file=174565#file174565line58" style="color: black; font-weight: bold; text-decoration: underline;">filters/stage/powerpoint/PptToOdp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">class</span> <span class="nc">PptToOdp</span><span class="o">::</span><span class="n">List</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Comments, man! Comments!
What is this list for? The name "List" strikes me as a little too generic for a non-template class.</pre>
</div>
<br />
<p>- Inge</p>
<br />
<p>On July 28th, 2013, 9:47 p.m. UTC, Jos van den Oever wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra.</div>
<div>By Jos van den Oever.</div>
<p style="color: grey;"><i>Updated July 28, 2013, 9:47 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Opened several ppt and xls files.
Checked ppt conversion for two files and checked that XML was equivalent.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>devtools/CMakeLists.txt <span style="color: grey">(15008fb)</span></li>
<li>devtools/rng2cpp/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>devtools/rng2cpp/rng2cpp.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libmso/CMakeLists.txt <span style="color: grey">(6bc145f)</span></li>
<li>filters/libmso/shapes.cpp <span style="color: grey">(073e061)</span></li>
<li>filters/libmso/shapes2.cpp <span style="color: grey">(0f0b906)</span></li>
<li>filters/sheets/excel/import/CMakeLists.txt <span style="color: grey">(2466218)</span></li>
<li>filters/sheets/excel/import/excelimporttoods.cc <span style="color: grey">(de788d4)</span></li>
<li>filters/stage/powerpoint/PptToOdp.h <span style="color: grey">(8d85c1f)</span></li>
<li>filters/stage/powerpoint/PptToOdp.cpp <span style="color: grey">(9258564)</span></li>
<li>libs/kotext/KoInlineNote.cpp <span style="color: grey">(6faa9a9)</span></li>
<li>libs/odf/CMakeLists.txt <span style="color: grey">(a2e3695)</span></li>
<li>libs/odf/writeodf/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/odf/writeodf/helpers.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/odf/writeodf/odfwriter.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111773/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>