Review Request 127843: Properly remove composed characters

[Dominik Haumann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127843/#review95257
-----------------------------------------------------------



I can see that the patch works, since Qt's QTextLayout functions are used for cursor navigation.

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?

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).


src/document/katedocument.cpp (line 3123)
<https://git.reviewboard.kde.org/r/127843/#comment64611>

    The textlayout navigation functions already take care of not ending up in a surrogate character, so it's safe to delete this if.



src/document/katedocument.cpp (line 3153)
<https://git.reviewboard.kde.org/r/127843/#comment64612>

    Same here, not needed if.



src/document/katedocument.cpp (line 3207)
<https://git.reviewboard.kde.org/r/127843/#comment64613>

    if not needed


- Dominik Haumann


On May 5, 2016, 10:53 a.m., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127843/
> -----------------------------------------------------------
> 
> (Updated May 5, 2016, 10:53 a.m.)
> 
> 
> Review request for Kate, KDE Frameworks, Christoph Cullmann, and Dominik Haumann.
> 
> 
> Repository: ktexteditor
> 
> 
> Description
> -------
> 
> 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)
> 
> Can be tested with these words: ?????????? or ??????????? or ????????
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   src/document/katedocument.cpp 73778a1 
>   src/view/kateview.h 08db0df 
>   src/view/kateview.cpp fda6b2d 
> 
> Diff: https://git.reviewboard.kde.org/r/127843/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160507/fc74fe06/attachment-0001.html>