At the same time, we change the defaults for both of them to "date" (they were
"recency" and "alphabetical", respectively, before). This is the reason we need
to touch so many integration tests. For some of them I decided to adapt the test
assertions to the changed sort order; for others, I added a SetupConfig step to
set the order back to "recency" so that I don't have to change what the test
does (e.g. how many SelectNextItem() calls are needed to get to a certain
branch).
When toggling the value in the UI we simply overwrite the value in UserConfig;
this would be bad if there was ever a chance that we want to write the user
config back to disk, but it is very unlikely that we can do that, because
currently we have no way to tell which parts of the config come from the global
config file and which ones come from a repo-local one.
The version choice is a little arbitrary, but see discussion at
https://github.com/jesseduffield/lazygit/pull/4559#issuecomment-2876201680.
The main reason why I'm updating the version now is that versions before 2.27
had a bug with branch sorting, where sorting by -committerdate (which will be
our default soon) would sort branches that point at the same commit in reverse
alphabetical order rather than alphabetical order. While this is only slightly
annoying but not a huge deal for users, it makes maintaining our integration
tests across versions very hard. So I wanted to update to at least 2.27 to get
around this problem, and went with 2.32 after the discussion linked to above.
The choice of which versions to run integration tests on is pretty arbitrary
too, I just picked some at random which are about 5 to 6 minor versions apart.
It's a very common pattern in the code base to have some code that we want to
run either directly, or with a confirmation, depending on some condition. In
most cases this is solved by creating a local helper function that we call
either directly or from within the HandleConfirm of the confirmation; provide a
convenience helper that makes this easier.
These were found by commenting out the initialization of the struct literal in
EnglishTranslationSet.TranslationSet(), and then running
$ punused pkg/i18n/**/*.go
Punused can be installed with `go install github.com/bep/punused@latest`.
The code was copied from StagingController in 0496e3af50, and I did add the new
text in that commit, I just forgot to adapt the code to actually use it.
If the hunk to be selected was partially scrolled offscreen, the view wouldn't
scroll enough to make it completely visible (the last line of the hunk was still
offscreen).
This is only a minimal fix for a pressing problem. The code to handle scrolling
after selection changes has lots of problems, and is also inconsistent between
list views and the patch explorer, but cleaning this up needs more time than I
have right now.
We only want to do this when the function is called from the remote branches
panel. It can also be called with a selection of local branches in order to
delete their remote branches, but in this case the selection shouldn't be
collapsed because the local branches stay around.
We had code already that was supposed to do this, but it didn't work. It should
have used SetSelection() instead of SetSelectedLineIdx(); the latter doesn't
actually cancel a range selection.
Introduce a new function specifically for collapsing the range after deleting
multiple items, so that clients don't need two calls (we'll add a bunch more in
this branch).
While it's true that the behavior is a little different from the staging panel,
where the staged lines are actually removed from the view and in many cases the
selection stays more or less in the same place, it is still very useful to move
to the next stageable thing in the custom patch building view too.
This improves the experience when selecting a hunk generously with the mouse, by
dragging over it including some context lines above and below. Previously we
would consider the "moving end" of the selection range for whether things need
to be added or removed, but this doesn't make sense if it's a context line. Now
we consider the first actual change line that is included in the range.
It is confusing to get header lines, hunk headers, or context lines rendered as
being included in a custom patch, when including these makes no difference to
the patch.
This is only a visual change; internally, we still record these non-patch lines
as being included in the patch. It doesn't matter though; you can press space on
a header line and nothing happens.
It would probably be cleaner to only record + and - lines in the includedLines
array, but that would be a bit more work, and doesn't seem worth it.
CheckMergeOrRebase calls Refresh already. However, it does an async refresh by
default, so we must turn this into a sync refresh so that moving the selection
down by one works even for the very first commit in history. Also, we must add
an explicit call to FocusLine so that the view selection is in sync with the
model selection; previously this was taken care of by the PostRefreshUpdate call
that happens as part of a refresh.
Keep the same commit selected, by moving the selection down by the number of
cherry-picked commits. We also do this when reverting commits, and it is
possible now that we use a sync waiting status.
We also need to turn the refresh that happens as part of CheckMergeOrRebase into
a sync one, so that the commits list is up to date and the new selection isn't
clamped.
We are about to change the selection behavior when cherry-picking, and it's good
to have tests that document in what way it changes in the next commit.
For the case of creating a new branch by moving commits to it, we were using the
current (old) branch name in the stash name; change this to use the new name
instead.
Unlike moving a patch to the index, applying or reverting a patch didn't
auto-stash, which means that applying a patch when there's a modified (but
unstaged) file in the working tree would error out with the message "error:
file1: does not match index", regardless of whether those modifications conflict
with the patch or not.
To fix this, we *could* add auto-stashing like we do for the "move patch to
index" command. However, in this case we rather simply stage the affected files
(after asking for confirmation). This has a few advantages:
- it only changes the staging state of those files that are contained in the
patch (whereas auto-stashing always changes all files to unstaged)
- it doesn't unnecessarily show a confirmation if none of the modified files are
affected by the patch
- if the patch conflicts with the modified files, the conflicts were "backwards"
("ours" was the patch, "theirs" the modified file); it is more logical if "ours"
is the current state of the file, and "theirs" is the patch.
It's a little unfortunate that the behavior isn't exactly the same as for "move
patch to index", but for that one we do need the auto-stash because of the
rebase that runs behind the scenes.
