When exiting filtering mode while the focus is not in the local commits panel,
the call to HandleFocus would render the selection in the commits panel as if
the panel had the focus.
The call to HandleFocus was introduced recently in 4981419ba9 in an attempt to
rerender the main view correctly, but it should only have been called when local
commits is the focused context. But instead, we can use PostRefreshUpdate, which
handles this distinction.
We forgot to convert the model indices to view indices in searchModelCommits.
This needs to be done for search results to be highlighted correctly in the
"divergence from upstream" view, which adds "--- Remote/Local ---" entries, and
during a rebase, where we have "--- Pending rebase todos ---" and "--- Commits
---" which offset view indices from model indices.
I have seen cases where during a rebase (two nonModelItems) all entries in
viewIndicesByModelIndex beyond the second nonModelItem were off by 4 rather than
2 as I would expect. The only explanation I have for this is that the function
was called concurrently.
Improve this by working on a local variable and only assign to self at the end;
this is not a real fix for the concurrency issue of course, but it makes it much
less likely to be a problem in practice.
This doesn't change behavior, just makes the code a little bit easier to
understand: the outermost condition is "do we show a popup and is the mouse
event for some other view than the focused one". Only if that's true do we need
to define the isCommitMessageView function, and check whether both views belong
to the commit message editor.
Previously a click was detected as a double-click whenever the click was on the
already selected line, regardless of how long ago the last click was (or even
when it wasn't selected by clicking at all). Now that gocui supports proper
double-click detection, we can do better.
The rationale for this is the same as in the previous commit; however, for these
functions we only allow a single controller to set them, because they are event
handlers and it doesn't make sense for multiple controllers to handle them.
Trying to do this would previously have the second one silently overwrite the
first one's.
We don't currently have this in lazygit, but I ran into the situation once
during development, and it can lead to bugs that are hard to diagnose.
Instead of holding a list of functions, we could also have added a panic in case
the function was set already; this would have been good enough for the current
state, and enough to catch mistakes early in the future. However, I decided to
allow multiple controllers to attach these functions, because I can't see a
reason not to.
These are never called on the context, they only exist to satisfy the
HasKeybindings interface. But that's wrong, HasKeybindings has nothing to do
with focus handling or rendering the main view. Remove them from that interface
and add them to IController instead.
When entering filtering we would only call FocusLine, which takes care of
highlighting the selected line in the commits list, but not of re-rendering the
main view. HandleFocus does that.
When exiting filtering, the HandleFocus call was missing entirely.
The tests needed to be reworked a little bit to make this testable.
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.
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.
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.
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.
