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
Details
Details
- Reviewers
dhaumann - Group Reviewers
KTextEditor - Commits
- R39:5c94789d28ea: ViewPrivate: Make deselection by arrow keys more handy
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
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.
Comment Actions
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
Comment Actions
- Please add a unit test (easy in this case)
- Please test with right to left text layout, and mixed text layout.
- Please keep the blinking cursor, this would be separate patch anyways.
- 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.
Comment Actions
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.
Comment Actions
@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.
Comment Actions
- 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
Comment Actions
I think for the bidi stuff, one needs to honor the if (m_viewInternal->m_view->currentTextLine().isRightToLeft()) flag.
Comment Actions
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. |
Comment Actions
me wrote:
- 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
Comment Actions
Ok with me.
I still think that the unit test can be improved, like mixed rtl + ltr text, but ok :)
Comment Actions
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.