ViewPrivate: Make deselection by arrow keys more handy
ClosedPublic

Authored by loh.tar on Mar 8 2019, 6:37 PM.

Details

Summary

This patch move the cursor to the start or end of the selection
instead of simple one position into the key direction

BUG: 296500
FIXED-IN: 5.57

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Mar 8 2019, 6:37 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 8 2019, 6:37 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Mar 8 2019, 6:37 PM

Nice! This is one of my top annoyances with Kate vs other text editors I use!

There's one issue: while text is selected, there's still a blinking insertion point at the start or end of the selection which is no longer accurate, since it will jump to the start or end of the text field rather than to the right or left of where the blinking cursor is. I would recommend also hiding the blinking insertion point while text is selected.

I would recommend also hiding the blinking insertion point while text is selected.

Then you lost the hint what may happens by e.g. Shift-Left/RightArrow

I would recommend also hiding the blinking insertion point while text is selected.

Then you lost the hint what may happens by e.g. Shift-Left/RightArrow

I guess that makes sense. Maybe it could not blink when there's a selection?

dhaumann added a subscriber: dhaumann.EditedMar 9 2019, 7:11 AM
  1. Please add a unit test (easy in this case)
  2. Please test with right to left text layout, and mixed text layout.
  3. Please keep the blinking cursor, this would be separate patch anyways.
  4. What happens if you have "persistent selection" enabled?

PS: years ago we had the same patch, and it was rejected, because the current behavior is also not bad.

brauch added a subscriber: brauch.Mar 9 2019, 8:49 AM

Hm, so shift-selecting with Shift+Left 5 times, then pressing Shift+Right once clears the selection? Just for the statistics, deselecting one character is a feature I use all the time.

Why is this behaviour helpful? If you want to clear the selection, press Esc.

@brauch I think you misunderstood: Select a text in any Qt application (line edit, ...), and then see what happens if you <cursor left> or <cursor right> without holding shift down. This patch is a good one, we just need to sort out possible regressions, and that's it.

loh.tar updated this revision to Diff 53501.Mar 9 2019, 9:27 AM
  • Do nothing in persistent selection mode
  1. Please test with right to left text layout, and mixed text layout.

Can't say what is to be expect in such case, so test is difficult for me

Looks good, now just the autotest is missing :-)

loh.tar updated this revision to Diff 53508.Mar 9 2019, 12:31 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Add autotest

I think for the bidi stuff, one needs to honor the if (m_viewInternal->m_view->currentTextLine().isRightToLeft()) flag.

brauch added a comment.Mar 9 2019, 3:46 PM

Sorry, yeah, I misunderstood!

loh.tar updated this revision to Diff 53540.Mar 9 2019, 8:09 PM
  • Consider RTL text
dhaumann edited the summary of this revision. (Show Details)Mar 9 2019, 8:59 PM

The unit test is good, but it does not yet test the right-to-left case (e.g. arabic text). Could you add one for this as well?

Maybe you can even use KateViewTest::testDeselectByArrowKeys_data() along with QFETCH to reuse the same code in the test function (see other usage of QFETCH).

src/view/kateview.cpp
2793 ↗(On Diff #53501)

Since we have this line twice, I suggest to move this after line 2791 as follows:

const bool moveToEndOfSelection = selection() && !config()->persistentSelection();

Then, you can use a simple if (moveToEndOfSelection) {...} which is a bit more readable.

loh.tar updated this revision to Diff 53574.Mar 10 2019, 7:42 AM
loh.tar set the repository for this revision to R39 KTextEditor.

me wrote:

  1. Please test with right to left text layout, and mixed text layout.

Can't say what is to be expect in such case, so test is difficult for me

OK. It pointed out that these RTL implementation is slightly buggy in Kate/Qt and therefore I was lost. After recent "forced" request to implement it anyway and to ignore my inner aversions to do it, I found good old BUG 165397. These test data and comments helped me to judge now that this patch works as best as it is possible.

  • Remove pointless "m_viewInternal->m_view->"
  • Re-arrange if cases, looks now similar as rest of the code
  • Use QFETCH, but not for all needed data. Requests to fix that will ignored :p
dhaumann accepted this revision.Mar 10 2019, 6:52 PM

Ok with me.

I still think that the unit test can be improved, like mixed rtl + ltr text, but ok :)

This revision is now accepted and ready to land.Mar 10 2019, 6:52 PM

Ok with me.

Thanks.

I still think that the unit test can be improved, like mixed rtl + ltr text, but ok :)

That could be more difficult as you may expect. I think the current tests are OK, thisway has it to work always. The current behaviour in mixed text is strange and should not be verified by tests.

This revision was automatically updated to reflect the committed changes.