- **PR Description**
Issue: https://github.com/jesseduffield/lazygit/issues/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**
* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [ ] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [x] Docs (specifically `docs/Config.md`) have been updated if
necessary
* [ ] You've read through your own file changes for silly mistakes etc
<!--
Be sure to name your PR with an imperative e.g. 'Add worktrees view'
see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for
examples
-->
Often if a test fails and there's an unaknowledged toast message, that message will
explain why the test failed. Given that we don't display toast messages in
integration tests when they run (for reasons I can't recall right now), we need to
log it as part of the error message.
We don't need it there so no need to enable it.
I'm leaving the disabled reason checks there, even though they're now redundant,
because they're only one-liners and they communicate intent.
We want to show an error when the user tries to invoke an action that expects only
a single item to be selected.
We're using the GetDisabledReason field to enforce this (as well as DisabledReason
on menu items).
I've created a ListControllerTrait to store some shared convenience functions for this.
This requires us to change the 'v' keybinding for paste to something else,
now that 'v' is used globally for toggling range select. So I'm using
'shift+v' and I'm likewise changing 'c' to 'shift+c' for copying, so
that they're consistent.
We will need to clearly communicate this change in keybindings.
The only time we should call SetSelectedLineIdx is when we are happy for a
select range to be retained which means things like moving the selected line
index to top top/bottom or up/down a page as the user navigates.
But in every other case we should now call SetSelection because that will
set the selected index and cancel the range which is almost always what we
want.
This is the highest priority of the escape actions because it's the thing you're
most likely to want to do upon hitting escape if you have a range selected.
Applying this to the staging/patch-building views is tricky: if we want this logic
for when a range of lines is selected, we'll also need to apply it when a hunk
is selected too. I still think it's worth it though: I've often accidentally
escaped from the staging view when trying to cancel a range selection.
We're not fully standardising here: different contexts can store their range state however
they like. What we are standardising on is that now the view is always responsible for
highlighting the selected lines, meaning the context/controller needs to tell the view
where the range start is.
Two convenient benefits from this change:
1) we no longer need bespoke code in integration tests for asserting on selected lines because
we can just ask the view
2) line selection in staging/patch-building/merge-conflicts views now look the same as in
list views i.e. the highlight applies to the whole line (including trailing space)
I also noticed a bug with merge conflicts not rendering the selection on focus though I suspect
it wasn't a bug with any real consequences when the view wasn't displaying the selection.
I'm going to scrap the selectedRangeBgColor config and just let it use the single line
background color. Hopefully nobody cares, but there's really no need for an extra config.
This adds range select ability in two ways:
1) Sticky: like what we already have with the staging view i.e. press v then use arrow keys
2) Non-sticky: where you just use shift+up/down to expand the range
The state machine works like this:
(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
CI is erroneously saying that our integration tests are passing.
As @stefanhaller says in a comment:
> It broke with this commit:
aaecd6cc40.
Since that change, the run_integration_tests.sh script will exit with
the exit status of the the last mv command, not the test run. Should be
pretty easy to fix.
Thanks to Stefan for fixing this
(The reason I'm saying this all here in the PR description is that PR
descriptions now get included in merge commits)
Previously we included all navigation keybindings from all views in the keybindings menu, meaning
if you pressed enter on 'next page' in the commits view, you'd end up triggering the action
in the sub-commits view.
- **PR Description**
Addresses #3116.
I'm not 100% sure I like the behavior, but I put it out there so that
others can test it and form an opinion. It not only affects keybindings,
but also invoking menu items (either with enter or with their key
binding): the menu now stays open in that case, which I think is
actually better.
There's a horrible hack for keeping the integration tests working, I
don't have a good idea how to fix that for real. Suggestions welcome.
- **PR Description**
A common issue I have is that I want to move a commit from the top of my
branch all the way down to the first commit on the branch. To do that, I
need to navigate down to the first commit on my branch, press 'e' to
start an interactive rebase, then navigate back up to the top of the
branch, then move my commit back down to the base. This is annoying.
Similarly annoying is moving the commit one-by-one without explicitly
starting an interactive rebase, because then each individual step is its
own rebase which takes a while in aggregate.
This PR allows you to press 'i' from the commits view to start an
interactive rebase from an 'appropriate' base. By appropriate, we mean
that we want to start from the HEAD and stop when we reach the first
merge commit or commit on the main branch. This may end up including
more commits than you need, but it doesn't make a difference.
- **Please check if the PR fulfills these requirements**
* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [x] Docs (specifically `docs/Config.md`) have been updated if
necessary
* [x] You've read through your own file changes for silly mistakes etc
<!--
Be sure to name your PR with an imperative e.g. 'Add worktrees view'
see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for
examples
-->
Because we obtain disabled reasons after every action, we need to keep the code for doing so
super fast. As such, we should not be hitting the filesystem to get rebase state, instead
we should just get the cached state.
I feel like we should actually be using the cached state everywhere like we do with all
our other models if only for the sake of consistency.
A common issue I have is that I want to move a commit from the top of my branch
all the way down to the first commit on the branch. To do that, I need to navigate
down to the first commit on my branch, press 'e' to start an interactive rebase,
then navigate back up to the top of the branch, then move my commit back down to
the base. This is annoying.
Similarly annoying is moving the commit one-by-one without explicitly starting
an interactive rebase, because then each individual step is its own rebase which
takes a while in aggregate.
This PR allows you to press 'i' from the commits view to start an interactive
rebase from an 'appropriate' base. By appropriate, we mean that we want to start
from the HEAD and stop when we reach the first merge commit or commit on the main
branch. This may end up including more commits than you need, but it doesn't make
a difference.
We need to fetch our list of tests both outside of our test binary and
within. We need to get the list from within so that we can run the code
that drives the test and runs assertions. To get the list of tests we
need to know where the root of the lazygit repo is, given that the tests
live in files under that root.
So far, we've used this GetLazyRootDirectory() function for that, but it
assumes that we're not in a test directory (it just looks for the first
.git dir it can find). Because we didn't want to properly fix this
before, we've been setting the working directory of the test command to
the lazygit root, and using the --path CLI arg to override it when the
test itself ran. This was a terrible hack.
Now, we're passing the lazygit root directory as an env var to the
integration test, so that we can set the working directory to the actual
path of the test repo; removing the need to use the --path arg.
- **PR Description**
- **Please check if the PR fulfills these requirements**
* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [x] Docs (specifically `docs/Config.md`) have been updated if
necessary
* [x] You've read through your own file changes for silly mistakes etc
<!--
Be sure to name your PR with an imperative e.g. 'Add worktrees view'
see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for
examples
-->
For some bizarre reason `pkg/integration/tests/filter_by_path/cli_arg.go` is failing as of 8c716184 like so:
```
test_lazygit
Usage:
test_lazygit [git-arg]
Positional Variables:
git-arg Panel to focus upon opening lazygit. Accepted values (based on git terminology): status, branch, log, stash. Ignored if --filter arg is passed.
Flags:
-h --help Displays help with available flag, subcommand, and positional value parameters.
-p --path Path of git repo. (equivalent to --work-tree=<path> --git-dir=<path>/.git/)
-f --filter Path to filter on in `git log -- <path>`. When in filter mode, the commits, reflog, and stash are filtered based on the given path, and some operations are restricted
-v --version Print the current version
-d --debug Run in debug mode with logging (see --logs flag below). Use the LOG_LEVEL env var to set the log level (debug/info/warn/error) (default: false)
-l --logs Tail lazygit logs (intended to be used when `lazygit --debug` is called in a separate terminal tab)
-c --config Print the default config
-cd --print-config-dir Print the config directory
-ucd --use-config-dir override default config directory with provided directory
-w --work-tree equivalent of the --work-tree git argument
-g --git-dir equivalent of the --git-dir git argument
-ucf --use-config-file Comma separated list to custom config file(s)
Unknown arguments supplied: filterFile
```
where the CLI args are:
```
([]string) (len=5 cap=5) {
(string) (len=25) "/tmp/lazygit/test_lazygit",
(string) (len=6) "-debug",
(string) (len=108) "--use-config-dir=/Users/jesseduffieldduffield/repos/lazygit/test/_results/filter_by_path/cli_arg/used_config",
(string) (len=2) "-f",
(string) (len=10) "filterFile"
}
```
This appears to be a bug in flaggy itself. I've updated to the latest version but it still breaks. Bizarrely it works fine on CI and
only fails locally. Running lazygit locally with `lg -f pkg/gui/controllers/helpers/refresh_helper.go` it works fine. So I don't
know what's going on there. At any rate, I'm just going to get the test passing by passing `-f=filterFile` as a single argument.
We need to fetch our list of tests both outside of our test binary and within. We need
to get the list from within so that we can run the code that drives the test and runs
assertions. To get the list of tests we need to know where the root of the lazygit repo
is, given that the tests live in files under that root.
So far, we've used this GetLazyRootDirectory() function for that, but it assumes that
we're not in a test directory (it just looks for the first .git dir it can find). Because
we didn't want to properly fix this before, we've been setting the working directory of
the test command to the lazygit root, and using the --path CLI arg to override it when
the test itself ran. This was a terrible hack.
Now, we're passing the lazygit root directory as an env var to the integration test, so
that we can set the working directory to the actual path of the test repo; removing the
need to use the --path arg.
Git has a bug [1] whereby running multiple fetch commands at the same time
causes all of them to append their information to the .git/FETCH_HEAD file,
causing the next git pull that wants to use the information to become confused,
and show an error like "Cannot rebase onto multiple branches". This error would
occur when pressing "f" and "p" in quick succession in the files panel, but also
when pressing "p" while a background fetch happens to be running. One likely
situation for this is pressing "p" right after startup.
Since lazygit never uses the information written to .git/FETCH_HEAD, it's best
to avoid writing to it, which fixes the scenarios described above.
However, it doesn't fix the problem of repeatedly pressing "f" quickly on the
checked-out branch; since we call "git pull" in that case, the above fix doesn't
help there. We'll address this separately in another PR.
[1] See https://public-inbox.org/git/xmqqy1daffk8.fsf@gitster.g/ for more
information.
