Add range selection ability on list contexts

[jesseduffield]

• PR Description

Issue: #3196

This PR adds the ability to select a range of items in list contexts. It does this in two ways:

• Sticky range select: like what we already have in the staging view, you press 'v' to toggle range select and then use up/down keys to extend the range
• Non-sticky range select: rather than explicitly toggling this on/off, you use shift+up/down to extend the range

The PR adds the ability to range select in all list contexts, but it's up to individual actions to opt-in to supporting a range. This PR only supports it for copying a range of commits for cherry-picking. We can add more support iteratively so that we're not merging a single giant PR. For all actions requiring selection of a single-item, an error will be shown if a range is selected.

Other use cases we want to support in the near future:

• marking commits as pick/drop/fixup/squash/etc when mid-rebase
• fixup/squash/drop when outside rebase
• moving commits up/down (in or out of rebase)
• staging/unstaging multiple files
• discarding multiple files

Updated keybindings

Because the 'v' binding is now globally dedicated to toggling range select, I've changed the cherry-pick copy/paste keys from 'c' and 'v' to 'shift+C' and 'shift+V' respectively. I've also nullified the 'v' keybinding on the 'view divergence from upstream' option in the upstream options menu (conveniently it was the first option in the menu so you can press enter on it).

Standardised range select display

As a bonus, this PR standardises how we display a range select. We already had range select support in the patch explorer view and merge conflicts view, but they were directly rendering the highlighted selection (i.e. blue background colour) in the content written to the view, rather than tell the view which lines were selected and have the view highlight them itself. A convenient benefit here is that now the entire line is highlighted, including trailing space, rather than just the content of the line. Another convenient benefit is that our integration tests can now easily ask the view which lines are selected, rather than depending on the specific context, because the view keeps track of it.

I've removed the selectedRangeBgColor config option because selectedLineBgColor should be fine. I don't see the need for two options, but tell me if you think otherwise.

Also, another thing we're standardising on: hitting escape will cancel the range select, which in the staging/patch-building views means if you're selecting a range, you'll need to hit escape twice to exit out of the view. For consistency, we're also applying this logic if you have a hunk selected. I personally would much prefer this and have several times accidentally exited out of the view when trying to cancel a range select by pressing escape. In lazygit in general, 'escape' means 'exit out of the innermost mode' and I would consider range select to be a kind of mode.

Sticky vs non-sticky range interaction

Here's the state machine that explains how the sticky and non-sticky range select modes interact. Although users will typically pick one or the other, it's important to be clear on what the logic is if you swap between them:

(no range, press 'v') -> sticky range
(no range, press arrow) -> no range
(no range, press shift+arrow) -> nonsticky range
(sticky range, press 'v') -> no range
(sticky range, press arrow) -> sticky range
(sticky range, press shift+arrow) -> nonsticky range
(nonsticky range, press 'v') -> no range
(nonsticky range, press arrow) -> no range
(nonsticky range, press shift+arrow) -> nonsticky range

Also if you press escape in either range mode, it cancels the range select.

Some implementation details

• when the action involves toggling e.g. toggling cherry-pick copy or toggling staged, we decide what to do based on the selection: for example with staging: if there are any unstaged changes in the selection, we'll stage everything, otherwise we unstage everything. This is the logic we already had when staging individual directories.
• we retain range selection if a view loses focus
• where we previously set SetSelectedLineIdx all over the place (e.g. setting selected line idx to 0 when checking out a branch) we're now using SetSelection which also resets the range select. There are only a couple of places where we would still want to use SetSelectedLineIdx (e.g. when the user moves up/down a page, because they would want to retain range select in that case)

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
❌ -8.87% (target: -2.00%)	✅ 83.42%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (37590a4)	46028	37907	82.36%
Head commit (73710b7)	46409 (+381)	34108 (-3799)	73.49% (-8.87%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3207)	1098	916	83.42%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

[jesseduffield]

@stefanhaller you mentioned you have an upcoming branch that touches a lot of controller keybinding definitions, relating to GetDisabledReason. I need to add a bunch of checks for there only being a single item selected. Do you have any preference for how I do that? One idea that comes to mind which should minimise conflicts for now is to add a SingleItemOnly field to the keybinding struct: handle that in callKeybindingHandler in pkg/gui/keybindings.go before we call GetDisabledReason.

[stefanhaller]

@stefanhaller you mentioned you have an upcoming branch that touches a lot of controller keybinding definitions, relating to GetDisabledReason.

Don't worry about conflicts at this point. That branch is sufficiently out of date already that I will probably have to redo it anyway, and that's no problem for me. Also, most of the work I wanted to do on that branch is adding disabled reasons for things like "is there a selected commit" checks, and it sounds like you are going to have to do that work anyway for the range selection feature. Let me know if that's not your understanding, or if you'd like us to devide up the work somehow.

I need to add a bunch of checks for there only being a single item selected. Do you have any preference for how I do that? One idea that comes to mind which should minimise conflicts for now is to add a SingleItemOnly field to the keybinding struct

This doesn't feel right to me, as this field would only be meaningful for list views. I'd rather suggest to extend your new required() concept with things like requireSelected and requireSingleSelected.

[jesseduffield]

Sounds good to me

[jesseduffield]

@stefanhaller See the latest WIP commit 73710b7 (#3207)

I've copied over your DisabledReason struct definition (though not the part about checking the bool field) and I've created a ListControllerTrait struct to define helper functions like require and singleItemSelected for use by list controllers. I plan to go through and attach this trait to all list controllers and use that for asserting that only a single item is selected. This still leaves more work for moving logic out of existing handlers that should be disabled reasons but that's not a priority for me right now.

[stefanhaller]

@stefanhaller See the latest WIP commit 73710b7 (#3207)

I've copied over your DisabledReason struct definition (though not the part about checking the bool field) and I've created a ListControllerTrait struct to define helper functions like require and singleItemSelected for use by list controllers. I plan to go through and attach this trait to all list controllers and use that for asserting that only a single item is selected. This still leaves more work for moving logic out of existing handlers that should be disabled reasons but that's not a priority for me right now.

Very nice. I read the rest of the branch, great improvements there. Looking forward to having this on master.

When testing it though, it crashed on me when pressing 'e' or 'i' in the local commits panel (consistently, it seems). No time to investigate, sorry...

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.28% (target: -2.00%)	✅ 94.79%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (fc8998e)	46026	37893	82.33%
Head commit (fc2120c)	46381 (+355)	38314 (+421)	82.61% (+0.28%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3207)	1247	1182	94.79%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

[jesseduffield]

@stefanhaller this is ready for review now. I've fixed the issue with the e/i keybindings

[stefanhaller]

Really great. Unfortunately I don't have time for an in-depth look at every hunk, but I'm sure we can fix little things later if we find any after merging. The general direction is awesome.

A few random thoughts below.

If we're thinking about cutting a release soon, I think we should try to support a few more commands before we do; right now it looks a bit embarrassing to be able to select ranges everywhere but not be able to do anything with them (almost).

[stefanhaller]

Not a very intuitive name, I had to look at the implementation to figure out what it does. How about SortRange?

[jesseduffield]

Done

[stefanhaller]

Don't we want this to fail the test if it's called while there's a selection range?

[jesseduffield]

Good point

[stefanhaller]

I don't have a problem with removing this, I always use u enter to invoke it. I wonder though: shouldn't it be possible to override the global key locally? This is a menu, there's no reason to do a range selection there.

Also, I really think we want to disable selecting ranges for menus. Most people probably won't even try, but if you do, it looks confusing, and there's really no reason.

[jesseduffield]

I think that 'v' will be for range-select in pretty much all contexts, meaning that if we use it for something else in some situations, it will be confusing, so I think we should avoid using it in any local context for something else.

As for disabling range select in menus, I agree.

[jesseduffield]

If we're thinking about cutting a release soon, I think we should try to support a few more commands before we do; right now it looks a bit embarrassing to be able to select ranges everywhere but not be able to do anything with them (almost).

I agree, I've got some of that implemented already but didn't want to bloat out this PR more than it already is.

Something else I just noticed: on master, if you're selecting hunks in the patch building view, then when you escape and go to another file, hunk-selection mode is still enabled. In this PR, you press escape to go back to line select mode, then escape again to escape the view, meaning if you wanted to stage multiple files hunk-by-hunk, you'll need to press 'a' each time you go to the next file. Funnily on master the staging view does not persist selection mode across files. I'm not sure whether we should be persisting it though: my current plan is to have escape always take you from hunk/range mode back to line mode, and see if anybody cares. Thoughts?

[jesseduffield]

Also, integration tests are failing but github reports them as succeeding: https://github.com/jesseduffield/lazygit/actions/runs/7526670716/job/20485285388?pr=3207

No idea what caused that: ci.yml hasn't changed in a while.

[stefanhaller]

Something else I just noticed: on master, if you're selecting hunks in the patch building view, then when you escape and go to another file, hunk-selection mode is still enabled. In this PR, you press escape to go back to line select mode, then escape again to escape the view, meaning if you wanted to stage multiple files hunk-by-hunk, you'll need to press 'a' each time you go to the next file.

I like the change to make hunk mode less sticky. I usually only turn it on temporarily, and it has bothered me sometimes that it was still on next time.

Also, once people discover how easy it is to select a range using shift-arrow, I expect them to use that instead of hunk mode more often. I know I do.

[stefanhaller]

Huh? Did this come back accidentally?

[jesseduffield]

Yes it did haha. I re-introduced it to ensure that if we wanted, we could set 'v' as a key in a menu item. I'll remove it again given that we don't want to do that.

[stefanhaller]

Here's a thought about integration tests involving selection ranges. Consider this test fragment:

		t.Views().Commits().
			Focus().
			Lines(
				Contains("CI commit 04").IsSelected(),
				Contains("CI commit 03"),
				Contains("CI commit 02"),
				Contains("CI commit 01"),
			).
			Press(keys.Universal.RangeSelectDown).
			Lines(
				Contains("CI commit 04").IsSelected(),
				Contains("CI commit 03"),
				Contains("CI commit 02"),
				Contains("CI commit 01"),
			)

This succeeds, which I find unexpected. In fact, any combination of putting the IsSelected() call on either commit 03 or 04 succeeds, including none, or both.

I wonder if we can make it so that once you put an IsSelected() on any row, you need to add it to all selected rows for the test to succeed? I haven't thought about how easy or hard this would be to implement, but it is what I would expect.

Alternatively, we could make it so that IsSelected() is required everywhere, if that's easier to do. It would probably require changes to a lot of tests, but maybe it's good to be more explicit everywhere?

[jesseduffield]

Yep, I was thinking the same thing. We should definitely make it that if you say you want one to be selected, it's implied that all others must be non-selected.

As for whether we should always require the selection to be specified: I think we'd need to see if on net that makes things more noisy or not. We certainly wouldn't want to enforce it for views which aren't focused, but for views which are focused, it's possible that it makes tests more readable and clear on average.

This PR is large enough for now and I've got some follow-up PRs on the way so I'm going to merge this now the test code change can come in a separate PR

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 8a946631	✅ 94.79%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (8a94663)	Report Missing	Report Missing	Report Missing
Head commit (ab3004b)	46381	38322	82.62%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3207)	1247	1182	94.79%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩