<table><tr><td style="">ahmadsamir added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D26285">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D26285#inline-148271">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1342</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Duplication :-)</p>

<p style="padding: 0; margin: 8px;">Dangerous, if the actual char* is modified one day.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Personally, I needed to see the actual regex spelled out like that to understand the rest of the code in this function, this:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">QLatin1String("^(") + QLatin1Strig(s_entitySubRx) + QLatin1String(");")</pre></div>

<p style="padding: 0; margin: 8px;">doesn't work as well for me, and especially so with finalizeVisualText(), where ENTITY_SUBRX is many lines away, and there are two capture groups used.</p>

<p style="padding: 0; margin: 8px;">I thought of doing away with ENTITY_SUBRX and just construct the regex's in toVisualText() and  finalizeVisualText(), but the two must use the same pattern, except for the beginning, ^ and & respectively.</p>

<p style="padding: 0; margin: 8px;">I can try lessening the danger by tweaking the comment.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D26285#inline-148274">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1572</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Please try to always declare the variables where they are used. The old code said "QString ent = ...", why does the new code look more like C, declaring variables outside the loop?<br />
This is in no way more efficient, construction isn't very different from assignment.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I wouldn't know about C, having never written any code with it.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">This is in no way more efficient, construction isn't very different from assignment.</p></blockquote>

<p style="padding: 0; margin: 8px;">That is the key I was missing (which means I should do some research, before "assuming" stuff).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D26285#inline-148273">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1597</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">So it's not the "original" value anymore. Why the renaming of plain to text, and of text to original? It makes the diff harder to review (but if you convince me that it makes the code easier to read, that's fine). I'm just not sure that "original" for something that is modified in many places (line 1592 and here) makes sense as a name.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I concede that changing var names makes reading the diff harder, and I should, if needed, have done it in a separate patch.</p>

<p style="padding: 0; margin: 8px;">My, weird, logic is that "original" is the original string that is being modified, v.s. text which is a temporary place to store the parts of original after processing them to prevent the match looping on one singular place in the string. Simply original didn't translate to constant in my head. (And "plain" didn't make sense, it's not plain when it may have "&blah4;" appended to it).</p>

<p style="padding: 0; margin: 8px;">Anyway, I'd rather change them back than argue. :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R249 KI18n</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D26285">https://phabricator.kde.org/D26285</a></div></div><br /><div><strong>To: </strong>ahmadsamir, Frameworks, ilic, dfaure, mlaurent, aacid<br /><strong>Cc: </strong>kde-frameworks-devel, ltoscano, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>