<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://reviewboard.kde.org/r/5079/">http://reviewboard.kde.org/r/5079/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2010, 8:15 p.m., <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hi,
i haven't looked at the code yet, but in its current form the patch doesn't compile, as you try to include the non-existent "dbhighlighter.h" in octavehighlighter.h and sagehighlighter.h, and "highlightdatabase.h" in octavebackend.cpp.</pre>
</blockquote>
<p>On August 23rd, 2010, 6:17 p.m., <b>Miha Cancula</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 fixed the wrong includes, and I think the correct patch is up now.</pre>
</blockquote>
<p>On August 23rd, 2010, 8:06 p.m., <b>Alexander Rieder</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hi,
(i haven't tried the new patch, is it any different except for the includes?)
some initial comments:
- Octave Highlighter: operators, keywords variables are members but don't have m_ prefix. On purpose?
- Sage Highlighter needs newline at end of file
- use kDebug instead of qDebug (in DBHighlighter.cpp)
- personally i'd like it better if you'd use a QList<HighlightingRule> as it was before instead of a QHash for the regexps, as you only need to iterate over them,
and no lookup based on keys. (or is there any special reason for using a QHash?)
- indentations in sagehighlighter for keywords are a bit weird
- apidocs should be added (but those were already missing before)
Other than that, the patch looks good, and the performance is definately a lot better. I really like the fact that the API is completely independent of the underlying implementation(as in no QHash specific code in the headers).
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">- No purpose, I also noticed that one of them isn't even used. I'll change that.
- No problem, will add one
- Of course, missed that one
- Probably true, there's no reason for it, and easy to change since it's not exposed en the header. The QString one need a hash, so I just make the other one the same.
- Will take a look.
- About apidocs, I'll try to add them for the existing functions as well (primarily addPairs and highlightPairs).</pre>
<br />
<p>- Miha</p>
<br />
<p>On August 22nd, 2010, 9:06 p.m., Miha Cancula wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE-Edu and Alexander Rieder.</div>
<div>By Miha Cancula.</div>
<p style="color: grey;"><i>Updated 2010-08-22 21:06:23</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;">As discussed on email, I implemented a different approach to highlighting in Cantor. I'd like some feedback before committing it.
I introduced additional API in DefaultHighlighter and moved most of the logic in it, so the individual backend-specific highlighters only specify conditions (either QString or QRegExp) and matching text formats. The code looks much cleaner this way.
As Alexander and Oleksiy already determined, breaking the text into words and looking for these words is faster than iterating over a huge list of regexes and looking for each of them in the text. That's why functions, variables and keywords are implemented this way. OTOH, thing like comments and strings are easier done using Regexes, so this functionality is still there.
The implementation uses a QHash<QString, QTextCharFormat> and a QHash<QRegExp, QTextCharFormat>. If anyone knows of a way to make it faster, please say so.
I also updated highlighters for Octave, Maxima and Sage to use the word-based API as much as possible. Most of their code was also removed, because it's now in DefaultHighlighter. I left R alone because Oleksiy's work is not yet in trunk. </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;">I tested Maxima and Sage and they seem to be faster than before for large blocks. I used to have problems with non-smooth scrolling in Octave due to the large number of functions, but now it feels normal. I didn't notice any regressions (yet).
It all works both on trunk and 4.4. </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>/trunk/KDE/kdeedu/cantor/src/backends/maxima/maximahighlighter.h <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/backends/maxima/maximahighlighter.cpp <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/backends/octave/octavehighlighter.h <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/backends/octave/octavehighlighter.cpp <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/backends/sage/sagehighlighter.h <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/backends/sage/sagehighlighter.cpp <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/lib/defaulthighlighter.h <span style="color: grey">(1166707)</span></li>
<li>/trunk/KDE/kdeedu/cantor/src/lib/defaulthighlighter.cpp <span style="color: grey">(1166707)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/5079/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>