<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/109393/">http://git.reviewboard.kde.org/r/109393/</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;">Thanks for the effort. That's my partial review. I started with simple notes related to coding style and what not but then went into the essential topics, and these are the most important. If you'll agree I'd try to help to make this export tool complementary to the tools designed for the OOXML import filters (what mostly means using stream-readers/writers: QXmlStreamReader/QXmlStreamWriter)</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/109393/diff/3/?file=119635#file119635line36" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">36</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// The purpose of this class is to provide a parser for the</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;">Doxygen comments would be better, everywhere.</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/109393/diff/3/?file=119635#file119635line41" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">41</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">OdfParser</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;">How about Ko prefix?</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/109393/diff/3/?file=119635#file119635line55" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QHash</span><span class="o"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="o">&</span><span class="n">metadata</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;">For any new API let's recommend IN-OUT parameters passed via pointers (Qt API guildeline adopted by some of KDE). This comment is also about consistency - you passed odfStore here via pointer.</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/109393/diff/3/?file=119635#file119635line66" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">66</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QHash</span><span class="o"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="o">&</span><span class="n">manifest</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;">same note as for line 55</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/109393/diff/3/?file=119636#file119636line44" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QHash</span><span class="o"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="o">&</span><span class="n">metadata</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;">I wonder if the design is the best at all. The OOXML reader classes recursively check element names and perform translation for supported elements. No intermediate hash structures are filled, XML-stream-reading is performed as in pull-parser. They are confenient at first look, yes, but that's not the best approach performance-wise, and even wouldn't lead to noticeably smaller code.
When we convert FROM ODF, we can similarly translate on-demand without basically importing everything into memory.
This way we would be able to import data very selectively and skip ignored data.
I know the data for manifest, metadata and settings may be quite small, but styles would be already quite big. Moreover QHash<QString, QString> doesn't seem to model what styles are (there are various levels of styles in ODP for example). What container would you use for them?</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/109393/diff/3/?file=119636#file119636line87" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdfParser.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">87</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">kDebug</span><span class="p">()</span> <span class="o"><<</span> <span class="s">"Error occurred while parsing meta.xml "</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;">KDElibs4's kDebug() does not need extra spaces</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/109393/diff/3/?file=119637#file119637line46" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdtTraverser.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">46</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">ODFTRAVERSE_EXPORT</span> <span class="n">OdtTraverser</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;">Not sure but how about OdtContentIterator?
Compare http://en.wikipedia.org/wiki/XML#Pull_parsing</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/109393/diff/3/?file=119638#file119638line236" style="color: black; font-weight: bold; text-decoration: underline;">filters/libodftraverse/OdtTraverser.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">236</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_backend</span><span class="o">-></span><span class="n">beginTagSpan</span><span class="p">(</span><span class="n">element</span><span class="p">,</span> <span class="n">m_context</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;">begin*(), end() are all like SAX events.
Compared to pull-parser, this API forces to use three separate blocks:
1. beginTag*() - the backend implementation would want to create some data structures (DT1) here
2. handleInsideElementsTag() can call the same beginTag*() so there can be recursion, if so we have to protect the DT1 on a stack (with pull parsers we did not have this problem)
3. endTag*() needs to somehow find the data structure and actually bin it to the rest of overall processing result, be it an in-memory strcture or simply an output stream.
Unless I completely misunderstood, I feel we need to take a step back.
I'd like to see as much as possible helper code for ODF reading _shared_ but in the same time the API would be on top of stream-reader, and for stream-writing.
Please look at the key sentences from [http://en.wikipedia.org/wiki/XML#Pull_parsing]: "The recursive-descent approach tends to lend itself to keeping data as typed local variables in the code doing the parsing, while SAX, for instance, typically requires a parser to manually maintain intermediate data within a stack of elements which are parent elements of the element being parsed. Pull-parsing code can be more straightforward to understand and maintain than SAX parsing code."
At this point, caffeinated at night, I am interrupting the review, hoping we can find best possible solutions ;)</pre>
</div>
<br />
<p>- Jarosław</p>
<br />
<p>On March 13th, 2013, 7:24 p.m. UTC, Inge Wallin 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 Inge Wallin.</div>
<p style="color: grey;"><i>Updated March 13, 2013, 7:24 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 creates a new library in filters/odftraverse. The purpose of this library is to create classes to make it easier to create export filters for ODF files. What you do to use this library is that you inherit a base class for backends to this parser / traverser and in the many callbacks you write the output that is relevant for your output format.
To show how it can be used I have also created a very simple proof of concept filter that exports to text format, something Calligra actually lacked before.
The current implementation traverses only ODT files and there are still a number of NYI functions that I want to finish before the actual merge. But I thought I'd get some opinions early. In other words, I expect at least one, maybe two iterations before this branch can be merged.</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;">Tested with a lengthy text file.</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>filters/CMakeLists.txt <span style="color: grey">(8bcd640)</span></li>
<li>filters/libodftraverse/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdfParser.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdfParser.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdtTraverser.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdtTraverser.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdtTraverserBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/libodftraverse/OdtTraverserBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/words/ascii/AsciiExport.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/words/ascii/AsciiExport.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/words/ascii/CMakeLists.txt <span style="color: grey">(d36de47)</span></li>
<li>filters/words/ascii/OdtTraverserAsciiBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/words/ascii/OdtTraverserAsciiBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/words/ascii/TODO <span style="color: grey">(ceb1a24)</span></li>
<li>filters/words/ascii/words_ascii_export.desktop <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109393/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>