Implement single click on line number to select line of text
ClosedPublic

Authored by rkron on Mar 25 2018, 2:19 PM.

Details

Summary

Implements selecting entire line of text with a single click on line number.

FEATURE: 372503

Test Plan

Click on a line number and check that it selects the line the same way that triple-clicking did before.
Check that selecting a line and then Shift+Click on another line selects all intervening lines.

Diff Detail

Repository
R39 KTextEditor
Branch
Bug372503 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
rkron created this revision.Mar 25 2018, 2:19 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 25 2018, 2:19 PM
rkron requested review of this revision.Mar 25 2018, 2:19 PM
rkron edited the test plan for this revision. (Show Details)Mar 25 2018, 2:26 PM

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.

rkron added a comment.Mar 25 2018, 7:37 PM

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.

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.

rkron added a comment.Mar 25 2018, 7:55 PM

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

How about double click to highlight instead of tripple click?

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.

rkron added a comment.Mar 25 2018, 8:11 PM

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

richardbowen added a comment.EditedMar 25 2018, 8:12 PM

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. The previous functionality would be lost with the new single-click.

I haven't really tried the current one click feature until seeing this diff but it's pretty cool.

rkron added a comment.Mar 25 2018, 8:16 PM

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

richardbowen added a comment.EditedMar 25 2018, 8:18 PM

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 border is. What is it?

ngraham added inline comments.Mar 25 2018, 8:18 PM
src/dialogs/navigationconfigwidget.ui
43 ↗(On Diff #30517)

Unrelated change?

92 ↗(On Diff #30517)

Unrelated change?

181 ↗(On Diff #30517)

Unrelated change? Maybe do this in another patch?

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

rkron added a comment.Mar 25 2018, 8:22 PM

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

rkron updated this revision to Diff 30560.Mar 25 2018, 8:30 PM
  • Update revision per review comments.

Removed the config option to enable and disable the single-click behavior.

rkron edited the summary of this revision. (Show Details)Mar 26 2018, 9:56 PM
rkron edited the test plan for this revision. (Show Details)

+1 behaviorally!

rkron updated this revision to Diff 31182.EditedApr 2 2018, 11:58 PM
  • Removed two unnecessary lines in patch.

Zeroing m_scrollX and m_scrollY in beginSelectLine is not relevant to line selection. Removed these lines.

mwolff added a subscriber: mwolff.Apr 3 2018, 7:52 PM

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?

src/view/kateviewhelpers.cpp
2018 ↗(On Diff #31182)

const

2019 ↗(On Diff #31182)

can you comment this code, why exclude these areas?

2183 ↗(On Diff #31182)

const

src/view/kateviewinternal.cpp
2787 ↗(On Diff #31182)

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?

rkron updated this revision to Diff 31505.Apr 6 2018, 4:10 PM
  • Revise patch per review comments.

Added comments and removed code to fix view scrolling inconsistancy
when dragging over line numbers.

rkron edited the summary of this revision. (Show Details)Apr 6 2018, 4:13 PM
rkron edited the test plan for this revision. (Show Details)
rkron marked 4 inline comments as done.Apr 6 2018, 6:36 PM
rkron added a subscriber: rkron.

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

Where are we with this patch?

cullmann accepted this revision.Apr 26 2018, 4:44 AM

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.

This revision is now accepted and ready to land.Apr 26 2018, 4:44 AM
rkron added a comment.Apr 26 2018, 7:17 AM

Would someone please land this for me? Thanks.

ngraham closed this revision.Apr 26 2018, 3:04 PM

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

Yes, tested and it works.