D11685: Implement single click on line number to select line of text

[Dominik Haumann]

dhaumann added a comment.


  Ok, thanks!

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, rkron, mwolff, ngraham, 
#ktexteditor, #kate, #frameworks, michaelh, kevinapavew, bruns, demsking, 
cullmann, sars

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  Yes, tested and it works.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, rkron, mwolff, ngraham, 
#ktexteditor, #kate, #frameworks, michaelh, kevinapavew, bruns, demsking, 
cullmann, sars

D11685: Implement single click on line number to select line of text

[Dominik Haumann]

dhaumann added a comment.
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel.


  Btw, did anyone test with dynamic word wrap enabled? Does it still work, when 
a line spans multiple view lines?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: kwrite-devel, kde-frameworks-devel, dhaumann, rkron, mwolff, ngraham, 
#ktexteditor, #kate, #frameworks, michaelh, kevinapavew, bruns, demsking, 
cullmann, sars

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: dhaumann, rkron, mwolff, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  Would someone please land this for me? Thanks.

REPOSITORY
  R39 KTextEditor

BRANCH
  Bug372503 (branched from master)

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: dhaumann, rkron, mwolff, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars

D11685: Implement single click on line number to select line of text

[Christoph Cullmann]

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


  I have no strong opinions on that, as I am not really using the line numbers 
in such a way.
  But I think it can't hurt to have this in.

REPOSITORY
  R39 KTextEditor

BRANCH
  Bug372503 (branched from master)

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: dhaumann, rkron, mwolff, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  Where are we with this patch?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: dhaumann, rkron, mwolff, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a subscriber: dhaumann.
ngraham added a comment.


  @cullmann @dhaumann ping? I'm quite fond of this, myself.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: dhaumann, rkron, mwolff, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron marked 4 inline comments as done.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: rkron, mwolff, richardbowen, ngraham, #ktexteditor, #kate, #frameworks, 
michaelh, kevinapavew, bruns, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron edited the summary of this revision.
rkron edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: mwolff, richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron updated this revision to Diff 31505.
rkron added a comment.


  - Revise patch per review comments.
  
  Added comments and removed code to fix view scrolling inconsistancy
  when dragging over line numbers.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11685?vs=31182=31505

BRANCH
  Bug372503 (branched from master)

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

AFFECTED FILES
  src/view/kateviewhelpers.cpp
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: mwolff, richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Milian Wolff]

mwolff added a comment.


  the "also" in your commit message: can you split this commit into two parts, 
or is the feature addition also fixing the bug? Put differently: Could you 
first fix the bug, then add the feature, in separate commits?

INLINE COMMENTS

> kateviewhelpers.cpp:2018
>  m_lastClickedLine = t.line();
> -if (positionToArea(e->pos()) != IconBorder && 
> positionToArea(e->pos()) != AnnotationBorder) {
> +auto area = positionToArea(e->pos());
> +if (area != IconBorder && area != AnnotationBorder) {

const

> kateviewhelpers.cpp:2019
> +auto area = positionToArea(e->pos());
> +if (area != IconBorder && area != AnnotationBorder) {
> +auto pos = QPoint(0, e->y());

can you comment this code, why exclude these areas?

> kateviewhelpers.cpp:2183
>  const KateTextLayout  = m_viewInternal->yToKateTextLayout(e->y());
> +auto area = positionToArea(e->pos());
>  if (t.isValid()) {

const

> kateviewinternal.cpp:2787
> +placeCursor(pos);
> +m_possibleTripleClick = true;
> +}

dito comment, what has triple click to do with *begin* of select line? Is this 
b/c an actual triple click would select the line?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: mwolff, richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron updated this revision to Diff 31182.
rkron added a comment.


  - Removed two unnecessary lines in patch.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11685?vs=30560=31182

BRANCH
  Bug372503 (branched from master)

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

AFFECTED FILES
  src/view/kateviewhelpers.cpp
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  +1 behaviorally!

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a reviewer: cullmann.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron edited the summary of this revision.
rkron edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron updated this revision to Diff 30560.
rkron added a comment.


  - Update revision per review comments.
  
  Removed the config option to enable and disable the single-click behavior.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11685?vs=30517=30560

BRANCH
  Bug372503 (branched from master)

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

AFFECTED FILES
  src/view/kateviewhelpers.cpp
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  @ngraham Qt Designer made those changes when I added the checkbox. They will 
be gone with the update. The four files concerning the config option will be as 
they were before this revision.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  @richardbowen, this isn't a support forum, it's for patch review. Please only 
leave comments related to the patch itself. Thanks!

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> navigationconfigwidget.ui:43
>
> -   Autocenter cursor:
> +   Autocenter cursor:
>

Unrelated change?

> navigationconfigwidget.ui:92
>
> -   Text selection mode:
> +   Text selection mode:
>

Unrelated change?

> navigationconfigwidget.ui:181
> +  chkClickLineNumberSelectsLine
> + 
>   

Unrelated change? Maybe do this in another patch?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Richard Bowen]

richardbowen added a comment.


  In D11685#234040 , @rkron wrote:
  
  > And actually, if you have the line modification marker border turned on, 
clicking in it will just move the cursor without selecting the line.
  
  
  Not sure what the line modification marker is. What is it?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  And actually, if you have the line modification marker border turned on, 
clicking in it will just move the cursor without selecting the line.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Richard Bowen]

richardbowen added a comment.


  With the double click option, it allows for two different functionality 
(place at home row, and highlight) as opposed to one if there was only 1 click 
to highlight.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  @ngraham I agree. Single-click is pretty much standard.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  In D11685#234009 , @richardbowen 
wrote:
  
  > How about double click to highlight instead of tripple click?
  
  
  I think consistency with the common interaction method is important here: 
other text editors implement this via single-click. No sense replacing one 
nonstandard UI with another one.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Richard Bowen]

richardbowen added a comment.


  How about double click to highlight instead of tripple click?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  OK. I will remove the option and update the revision. Thanks for the input!

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  IMHO, a mouse-based workflow for selecting large blocks of text makes sense, 
which is why this should be made at least the default setting. But using the 
mouse to go to the first column makes less sense, since you're probably about 
to do something with the text, which means your fingers need to be on the 
keyboard, which means you should just use the default keyboard shortcut 
([Home]).
  
  I'm okay with making it the default if we do have to add an option. But every 
option must be carefully considered, or else configuration windows become 
intimidating and impossible to navigate, and the genuinely useful options 
become buried. Everything that can be turned on or off must be that way because 
there is a genuinely useful workflow or a reasonable user preference attached 
to each setting. We shouldn't add options just to be conservative and avoid 
blame in case someone doesn't like it. Every change has someone who doesn't 
like it; completely avoiding that is sadly impossible. Instead, we should try 
to do the most good for the most people.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron added a comment.


  If you think it should be on by default, that's easily done. I know what you 
mean, that nobody would ever find it.
  
  When I first started using Kate, I thought the triple-clicking was very 
annoying. But I got used to it and now it's automatic for me. It also became 
automatic for me to move the cursor to the start of a line by clicking the line 
number. When I started using this patch, I found I was always selecting a line 
when I just wanted to move the cursor. That's why I hesitated to make such a 
big change to this workflow without making it optional.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Nathaniel Graham]

ngraham added a comment.


  Gave it a whirl; this is very nice!
  
  I don't think making it optional and off by default is the best choice, 
though. Nobody will ever find it. I would support making this enabled by 
default at the very least, and possibly even not turn-off-able with an option. 
Most advanced text editors already have this feature on by default with no 
option to disable it, and it's really hard for me to imagine what kind of 
workflow it would would negatively impact. After all, the line numbers 
themselves are off by default! Good defaults are important, and highly relevant 
to T6831: Top-notch usability and productivity for basic software 
.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann

D11685: Implement single click on line number to select line of text

[Randy Kron]

rkron created this revision.
rkron added reviewers: Frameworks, Kate, KTextEditor, ngraham.
Restricted Application added projects: Kate, Frameworks.
rkron requested review of this revision.

REVISION SUMMARY
  Implements selecting entire line of text with a single click on line number.
  
  Introduce a new config setting on the 'Text Navigation' editor config page
  to enable this, disabled by default to retain old behavior.
  
  Also fixes inconsistancy when dragging on line numbers to select multiple
  lines. When dragging in text editor, view would scroll down when you get
  to the top, but view did not scroll when dragging on line numbers.
  
  FEATURE: 372503

TEST PLAN
  With option enabled, check that single click on line number selects line
  the save way that triple-clicking does by default. Check that dragging
  over line numbers selects multiple lines and view scrolls down when
  you get to the top. Check that selecting a line and then Shift+Click
  another line selects all intervening lines. Check that the single click
  behavior can be turned off with the config setting.

REPOSITORY
  R39 KTextEditor

BRANCH
  Bug372503 (branched from master)

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: rkron, #frameworks, #kate, #ktexteditor, ngraham
Cc: ngraham, #ktexteditor, #kate, #frameworks, michaelh, kevinapavew, demsking, 
cullmann, sars, dhaumann