<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127843/">https://git.reviewboard.kde.org/r/127843/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 7th, 2016, 9:40 a.m. UTC, <b>Dominik Haumann</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can see that the patch works, since Qt's QTextLayout functions are used for cursor navigation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I dislike the part that exposes the "currentTextLayout()", since it exposes more API (but ok, maybe we must), and it is not flexible. For instance, ktexteditor.git has a multicursor branch, and just getting the "current" text layout is not enough. Could you change it to textLayout(int line), along with a comment that the returned pointer is possibly a nullptr?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">PS: KateViewInternal.cpp also has a KateWrappingCursor. A proper solution would be to expose this class so that proper text navigation is available everywhere (e.g. KTextEditor::ViewCursor, just like KTextEditor::DocumentCursor).</p></pre>
</blockquote>
<p>On May 9th, 2016, 9:30 a.m. UTC, <b>Jan Grulich</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm trying to expose KateWrappingCursor class, but problem is that it requires access to KateViewInternal which we would still need to expose instead of currentTextLayout(). Any idea how to solve that?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hm, say we have KateViewCursor with the following constructor:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">KTextEditor</span><span style="color: #666666">:</span><span style="color: #AA22FF">:KateViewCursor</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">KTextEditor</span><span style="color: #666666">:</span><span style="color: #AA22FF">:View</span> <span style="color: #666666">*</span> <span style="color: #008000; font-weight: bold">view</span><span style="color: #666666">);</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then, can't we qobject_cast this view to KTextEditor::ViewPrivate ? Then we have access to our internal KateView and KateViewInternal.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We'd need a pimpl-class, i.e. a d-pointer to store the qobject_cast<KTextEditor::ViewPrivate *>(view) as a member variable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In addition to this constructor, most of the API from KTextEditor::DocumentCursor [1] and what the Wrapping cursor etc can do right now.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does that make sense / is that feasible?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[1] http://api.kde.org/frameworks-api/frameworks5-apidocs/ktexteditor/html/classKTextEditor_1_1DocumentCursor.html</p></pre>
<br />
<p>- Dominik</p>
<br />
<p>On May 5th, 2016, 10:53 a.m. UTC, Jan Grulich wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Kate, KDE Frameworks, Christoph Cullmann, and Dominik Haumann.</div>
<div>By Jan Grulich.</div>
<p style="color: grey;"><i>Updated May 5, 2016, 10:53 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ktexteditor
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When using Indic locales composed characters are not properly removed. Pressing "delete" or "backspace" should remove the entire composed character and not only one base character. You can see how it should behave when using another text editor (e.g. libreoffice)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can be tested with these words: ?????????? or ??????????? or ????????</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Technical details:
I'm not really sure whether exporting current text layout is the right way to do this, I found that this is used when calling moveChar() which moves the cursor exactly by one composed character so I was trying to find a way to do it similarly.</p></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/document/katedocument.cpp <span style="color: grey">(73778a1)</span></li>
<li>src/view/kateview.h <span style="color: grey">(08db0df)</span></li>
<li>src/view/kateview.cpp <span style="color: grey">(fda6b2d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127843/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>