<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/100154/">http://git.reviewboard.kde.org/r/100154/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 19th, 2010, 10:58 p.m., <b>Jon Ander Peñalba</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 think that this solves the problem. I don't know if I've added all the symbols that cause trouble, but if not, it's really simple to update :)</pre>
</blockquote>
<p>On November 20th, 2010, 12:11 p.m., <b>Benjamin Poulain</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;">In my opinion, it is just the wrong way to solve the problem. You still have not addressed the case where you match "XX" in a string of "XXXXXXXXXXXXX".
In you last patch, you replace special symbols, but those can appear in an URL, you want to match them. If you want to escape all the control character in a meaningful way, your code will end up being bigger than the previous code.
The objective of you patch is "Text highlighting in the url bar simplified", the new code is neither simplified, neither correct.</pre>
</blockquote>
<p>On November 20th, 2010, 4:37 p.m., <b>Jon Ander Peñalba</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 think it's simpler and way more readable than our current solution.
I don't understand the problem of "XX" in "XXXXX". The result is "<b>XX</b><b>XX</b>X" and that's what's supposed to be.
I know that hard-coding all the symbols and removing them from the string is normally a bad solution, but in this case I think we have more to gain, the previous code is quite hard to follow.</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;">If I take a site like http://www.snowandrock.com/all/ski/fcp-category/home I might want to look it up in the history by typing "snow+rock" and this currently doesn't work with the regexp approach.
as for the </b><b> I don't really like it, if you type X then it'd result in <b>X</b><b>X</b><b>X</b><b>X</b><b>X</b>, even though it doesn't matter that much, that's a bit of overhead compared to <b>XXXXX</b>.
Weighing the pros and cons, I think the regexp-based solution is going to become as unreadable as the current implementation quite soon at this rate, and I fear it might never be as robust.
I agree that the current function used to do that can seem somewhat cryptic, and I take the blame for it, I should probably write a comment block there to explain what it does, since it's probably something that's bound to be changed and/or fine tuned in the future.
http://xkcd.com/208/
</pre>
<br />
<p>- Pierre</p>
<br />
<p>On November 19th, 2010, 10:56 p.m., Jon Ander Peñalba wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.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 rekonq.</div>
<div>By Jon Ander Peñalba.</div>
<p style="color: grey;"><i>Updated 2010-11-19 22:56:28</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;">I've simplified text highlighting. I think the behaviour hasn't changed, but a second opinion is welcome :)
I've removed the Qt::escape (and updated the test accordingly) because I find it useless, but if it needs to be there for any reason there's no problem in putting it back.</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;">The 'listitem_test' test passes.</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>src/tests/listitem_test.cpp <span style="color: grey">(fc0b62e)</span></li>
<li>src/urlbar/listitem.cpp <span style="color: grey">(a0462e7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100154/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>