Something dumb that we're currently doing is expecting list items
to define an ID method which returns a string. We use that when copying
items to clipboard with ctrl+o and when getting a ref name for diffing.
This commit gets us a little deeper into that hole by explicitly requiring
list items to implement that method so that we can easily use the new
helper functions in list_controller_trait.go.
In future we need to just remove the whole ID thing entirely but I'm too
lazy to do that right now.
This wasn't necessary before, because the only available branch sorting option
was by recency, so the sort order couldn't change except by checking out
branches. Now, you can sort by committer date, so the branch order can change by
fetching; in this case it's important to keep the same branch selected. One
important use case is to rebase the checked-out branch onto master; you select
master, press "f" to fetch it (this can now change its position in the list),
and then press "r" to rebase. To make this work smoothly it's important to keep
master selected after pressing "f".
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
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.
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.
The algorithm works by blaming the deleted lines, so if a hunk contains only
added lines, we can only hope that it also belongs in the same commit. Warn the
user about this.
Note: the warning might be overly agressive, we'll have to see if this is
annoying. The reason is that it depends on the diff context size whether added
lines go into their own hunk or are grouped together with other added or deleted
lines into one hunk. However, our algorithm uses a diff context size of 0,
because that makes it easiest to parse the diff; this results in hunks having
only added lines more often than what the user sees. For example, moving a line
of code down by two lines will likely result in a single hunk for the user, but
in two hunks for our algorithm. On the other hand, being this strict makes the
warning consistent. We could consider using the user's diff context size in the
algorithm, but then it would depend on the current context size whether the
warning appears, which could be confusing. Plus, it would make the algorithm
quite a bit more complicated.
There are two possible fixes for this bug, and they differ in behavior when
rewording a commit. The one I chose here always splits at the first line feed,
which means that for an improperly formatted commit message such as this one:
This is a very long multi-line subject,
which you shouldn't really use in git.
And this is the body (we call it "description" in lazygit).
we split after the first line instead of after the first paragraph. This is
arguably not what the original author meant, but splitting after the first
paragraph doesn't really work well in lazygit, because we would try to put both
lines into the one-line subject field of the message panel, and you'd only see
the second and not even know that there are more.
The other potential fix would have been to join subject and description with two
line feeds instead of one in JoinCommitMessageAndDescription; this would have
fixed our bug in the same way, but would result in splitting the above message
after the second line instead of the first. I think that's worse, so I decided
for the first fix.
While we're at it, simplify the code a little bit; strings.Cut is documented to
return (s, "") when the separator is not found, so there's no need to do this on
our side.
We do have to trim spaces on the description now, to support the regular reword
case where subject and body are separated by a blank line.
Without this it's not reliably possible to ask whether a given view is visible
by asking
windowHelper.TopViewInWindow(context.GetWindowName()) == context.GetView()
because there could be transient, invisible contexts after it in the Z order.
I guess it's a bit of a coincidence that this has never been a problem so far.
The output of the GetWindowDimensions function is hard to understand just by looking at it,
so I've added a helper function in the tests to render the window layout as text, so that
in order to create a new test you just come up with some args and paste the output as the
expected output.
This has the same downsides that any snapshot-based testing has: it's more brittle than
targeted assertions. But it is much easier to make sense of these snapshots than it is
to make sense of more fine-grained assertions, and I like the fact that these tests can
serve as documentation.
We are also removing the single-character padding on the left/right edges of the bottom
line because it's unnecessary
Unfortunately we need to create views for each spacer: it's not enough to just
layout the existing views with padding inbetween because gocui only renders
views meaning if there is no view in a given position, that position will just
render whatever was there previously (at least that's what I recall from talking
this through with Stefan: I could be way off).
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
refreshWorktrees re-renders the branches view, because the branches view shows
worktrees against branches. This means that when both BRANCHES and WORKTREES are
requested to be refreshed, the branches view would be rendered twice in short
succession. This causes an ugly visual glitch when force-pushing a branch,
because when pushing is done, we would see the ↑4↓9 status come back from under
the Pushing status for a brief moment, to be replaced with a green checkmark a
moment later.
Fix this by including the worktree refresh in the branches refresh when both are
requested. This means that the two are no longer running in parallel for an
async refresh, but hopefully that's not so bad.
When pulling/pushing/fast-forwarding a branch, show this state in the branches
list for that branch for as long as the operation takes, to make it easier to
see when it's done (without having to stare at the status bar in the lower
left).
This will hopefully help with making these operations feel more predictable, now
that we no longer show a loader panel for them.
Very similar to WithWaitingStatus, except that the status is shown in a view
next to the affected item, rather than in the status bar.
Not used by anything yet; again, committing separately to get smaller commits.
