DocumentPrivate: Review del/backspace
ClosedPublic

Authored by loh.tar on Mar 15 2019, 2:03 PM.

Details

Summary
  • Avoid bad selection in some case of undo in block mode
  • Don't try to expand selection in block mode when cursor is in col 0 on backspace
  • Improve/fix documentation
  • Code cosmetic

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
loh.tar created this revision.Mar 15 2019, 2:03 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 15 2019, 2:03 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Mar 15 2019, 2:03 PM

This patch is the byproduct of some playing with RTL text, Bug 385694.
But a sane solution may need some more effort, so I tend to not offer my current state of that playing.

Wow! Great :)
Thanks for looking into this, but I'm afraid that the problem isn't with backspace/del, but with the cursor positioning, at least for RTL and mixed text.
I actually found this to be a problem in Qt after trying to solve it myself in KTextEditor: https://bugreports.qt.io/browse/QTBUG-65508
Not sure what's the root of the problem :(

I had recently submit a patch which was also affected by RTL text D19621. Should this patch here be accepted I can upload my current state of the mentioned playing, which works similar as that D19621. It's for me very frustrating to test because I never know what is to be expect or what currently happens. Your Qt report make it even worse.

Probably can we not much do for a perfect solution as long Qt has some serious problem as your Qt bug report says.

BTW I noticed that typing a bracket in RTL text these is shown mirrored, very confusing too.

I'm afraid that the problem isn't with backspace/del, but with the cursor positioning

hm, currently is by backspace something to the right removed and by del to the left. So I think that's wrong and my patch fix that at least for backspace...in most cases IIRC. Then I stopped my playing.

cullmann accepted this revision.Sun, May 26, 10:38 AM
This revision is now accepted and ready to land.Sun, May 26, 10:38 AM
This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Tue, May 28, 12:22 PM

See https://bugs.kde.org/show_bug.cgi?id=408016

Guess this needs a revert, + some tests for that cases + a fix, thanks ;=)

This revision is now accepted and ready to land.Tue, May 28, 12:22 PM

That may fix it.

diff --git a/src/document/katedocument.cpp b/src/document/katedocument.cpp
index e365c380..7c24f67f 100644
--- a/src/document/katedocument.cpp
+++ b/src/document/katedocument.cpp
@@ -3350,7 +3350,9 @@ void KTextEditor::DocumentPrivate::backspace(KTextEditor::ViewPrivate *view, con
 
     } else {
         // col == 0: wrap to previous line
-        if (line > 0) {
+        const Kate::TextLine textLine = m_buffer->plainLine(line - 1);
+
+        if (line > 0 && textLine) {
             if (config()->wordWrap() && textLine->endsWith(QLatin1String(" "))) {
                 // gg: in hard wordwrap mode, backspace must also eat the trailing space
                 removeText(KTextEditor::Range(line - 1, textLine->length() - 1, line, 0));

I ATM have no access to my compile env. here, please commit the fix.
It would be best to have some test for this, too, for future reference.

This revision was automatically updated to reflect the committed changes.