D19621: ViewPrivate: Make deselection by arrow keys more handy

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:5c94789d28ea: ViewPrivate: Make deselection by arrow keys 
more handy (authored by loh.tar, committed by ngraham).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19621?vs=53574&id=53631

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateview.cpp

To: loh.tar, #ktexteditor, dhaumann
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor, dhaumann
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor, dhaumann
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar updated this revision to Diff 53574.
loh.tar set the repository for this revision to R39 KTextEditor.
loh.tar added a comment.


  >> me wrote:
  >> 
  >> 2. 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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19621?vs=53540&id=53574

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateview.cpp

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann added a comment.


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

INLINE COMMENTS

> kateview.cpp:2793
>  if (m_viewInternal->m_view->currentTextLine().isRightToLeft()) {
> -m_viewInternal->cursorNextChar();
> +if (selection() && !config()->persistentSelection()) {
> +m_viewInternal->updateCursor(selectionRange().end());

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.

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann edited the summary of this revision.

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar updated this revision to Diff 53540.
loh.tar added a comment.


  - Consider RTL text

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19621?vs=53508&id=53540

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateview.cpp

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Sven Brauch]

brauch added a comment.


  Sorry, yeah, I misunderstood!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Christoph Cullmann]

cullmann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: cullmann, brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar updated this revision to Diff 53508.
loh.tar set the repository for this revision to R39 KTextEditor.
loh.tar added a comment.


  - Add autotest

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19621?vs=53501&id=53508

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateview.cpp

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann added a comment.


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

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar added a comment.


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

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar updated this revision to Diff 53501.
loh.tar added a comment.


  - Do nothing in persistent selection mode

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19621?vs=53470&id=53501

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  src/view/kateview.cpp

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Sven Brauch]

brauch added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: brauch, dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Dominik Haumann]

dhaumann added a comment.


  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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, bruns, demsking, cullmann, sars

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Nathaniel Graham]

ngraham added a comment.


  In D19621#427614 , @loh.tar wrote:
  
  > > 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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: ngraham, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars, dhaumann

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar added a comment.


  > 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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: ngraham, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars, dhaumann

D19621: ViewPrivate: Make deselection by arrow keys more handy

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

To: loh.tar, #ktexteditor
Cc: ngraham, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars, dhaumann

D19621: ViewPrivate: Make deselection by arrow keys more handy

[loh tar]

loh.tar created this revision.
loh.tar added a reviewer: KTextEditor.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
loh.tar requested review of this revision.

REVISION 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.56

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19621

AFFECTED FILES
  src/view/kateview.cpp

To: loh.tar, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann