With the recently added external change detection, it happens more often
now that we refresh the commits list because an agent made a commit in
the background. In this case, if we keep the selection index the same,
it now points at a different commit, making the main view show a
different commit too, which is confusing and annoying. To fix this,
track the selected commit and range anchor by hash before reloading,
then restore those rows if both hashes still exist. This also allows us
to get rid of some bespoke code that did this for the specific cases of
reverting a commit or cherry-picking commits, because those are now
handled by the generic mechanism.
We set GIT_OPTIONAL_LOCKS=0 for every git command we run. That env var
only affects `git status`: it tells git not to take the optional lock it
would otherwise use to write the index back after refreshing the cached
stat information. The intent was to avoid contending for index.lock with
git commands the user runs in a terminal.
The downside is that our `git status` never persists the refreshed
stat-cache. So whenever the working tree's cached stat info goes stale
(e.g. editing files and discarding the changes, or a checkout), every
subsequent status re-hashes the affected files to confirm they're clean,
and stays slow until something else writes the index (such as the user
running `git status` in a terminal).
Fix this by only suppressing optional locks for refreshes that run
unattended in the background; foreground refreshes triggered by a user
action now run a plain `git status` that writes the refreshed index back,
just like the command line does. Background refreshes keep passing
--no-optional-locks so they still can't cause lock contention.
RefreshOptions gains a Background flag that the background routines set,
threaded down to the status command.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Several commands (rewording or amending an earlier commit, custom patch
operations, etc.) are implemented by starting an interactive rebase that stops
at a commit, amending it, and continuing. When no conflict occurs, the user
isn't meant to notice a rebase happened at all.
But a background file refresh can fire while the rebase is mid-flight and render
a dirty working copy of whatever the behind-the-scenes rebase is doing (e.g.
applying a custom patch).
To fix this, we pause the background routines for the duration of any
waiting-status operation — exactly the window in which lazygit is driving the
git operation itself and will refresh once at the end. The boundary is also
right for the conflict case: when a rebase stops on a conflict the operation
returns, the pause releases, and background refreshes resume for the interactive
resolution that follows.
Until now every keybinding config field was a plain string. That meant a user
couldn't ask for two keys to invoke a command — the config silently accepted
only one form.
Convert every string-typed field across all 13 KeybindingXxxConfig structs to
Keybinding so the union type extends to every command. Defaults wrap their
single-key value in Keybinding{...} so the generated Config.md still renders one
scalar key per binding.
The alt fields keep their separate Binding registrations for now: this commit
does not yet introduce the merge mechanism that folds them into the main field —
that comes in a follow-up. Consumers previously calling opts.GetKeys on a string
field now call opts.GetKeys on the Keybinding, or take .String() / Keys[0] where
a single value is needed.
Adds a Keybinding.String helper for rendering, schema-generator work that
inlines the Keybinding union into each consuming property, and a unit test
covering the user-facing scalar/sequence YAML forms for quit.
This is a pure refactor in preparation for letting users configure multiple
alternate bindings for a single command. Every Binding still has exactly one
key, so nothing changes visibly: the cheatsheet, the on-screen options bar,
and the keybindings menu all render identically.
When a Binding ends up with multiple keys, the on-screen options bar will
show only the first (to avoid clutter); the cheatsheet will show all of them (in
a later commit). For now both paths take Key[0].
MenuItem.Key is changed in the same way, it also has a slice of keys now.
In this commit we keep the name `Key` in Binding, KeybindingOpts and MenuItem,
instead of renaming them to `Keys` right away, in order to keep the diff a bit
more readable. We'll do the rename separately in the next commit.
Modifiers were moved into Key in 22169e22f, but the separate Modifier field
on types.Binding and gocui.keybinding was left behind. The keypress matcher
already compares modifiers via Key.Equals, so the old field is never read on
the dispatch path; it just got passed through SetKeybinding and stored.
Drop it from gocui.keybinding, types.Binding, and the SetKeybinding /
DeleteKeybinding signatures, and remove every now-redundant Modifier:
gocui.ModNone struct field. Mouse bindings keep their own Modifier (on
ViewMouseBinding) since that path still consults it.
This bundles the keyName and a rune, so that we don't have to pass these around
separately everywhere. This should make it easier to swap out the rune for a
string when we upgrade to tcell v3.
I copied all files except dot files (.github and .gitignore), the _examples
folder, and go.mod/go.sum.
At some point we may want to copy the files back to the gocui repo when other
clients (e.g. lazydocker) want to use the newer versions of them.
For esthetic reasons, checking out a branch (or other ref) blocks the UI until
the refresh is done, so it's important that the refresh doesn't do unnecessary
work. Refreshing pull requests is unnecessary (but costly, when waiting for it)
when a branch is checked out that already existed locally. However, it is
required when checking out a remote branch for the first time, so that the PR
icon appears immediately when there is one.
When switching to a different repo, and then back to the original one, searching
would no longer work. The reason is that our contexts set callbacks on their
views; when switching to a different repo we instantiate a new set of contexts,
so they will overwrite the views' callbacks with their new ones, but when
switching back to the original repo, we reuse the old contexts because they are
still in memory, but they won't set their callbacks again since they only do
this on construction.
To fix this, replace the view-local callbacks with a global one on the gui that
takes the view as an argument, so that the callback can look up the associated
context dynamically.
Can be used for doing additional click handling in list views.
Like the GetOnDoubleClick hook we should try to find a better design for this
than putting it in HasKeybindings and BaseContext, since it is only used by list
contexts.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
We have some documentation for the corresponding setters in IBaseContext, but
that's part of the controller infrastructure and not client facing. For somebody
implementing a new view, this is where they will probably look for what methods
they can override.
When this was originally introduced, it handled single clicks on a list entry
(treating them similar to a double-click by checking whether the click was on
the selected entry). Arguably it should have been called OnDoubleClick back then
already; but when we later changed it to do actual double-click detection (see
37197b8e9a), we should have renamed the methods.
This fixes a bug in ListContextTrait.FocusLine whereby the view would go blank
when scrolling by page (using ',' or '.') in views that have
renderOnlyVisibleLines set to true but refreshViewportOnChange set to false.
Currently we don't have any such views; the only ones who use
renderOnlyVisibleLines are commits and subcommits, and they also use
refreshViewportOnChange. However, we are going to add one in the next commit,
and eventually it might be a good idea to convert all our list views to that by
default, and get rid of the renderOnlyVisibleLines flag.
Move SetContentLineCount into OverwriteLinesAndClearEverythingElse. Calling it
separately beforehand is not concurrency safe; we need both to happen
when the view's writeMutex is locked.
It is possible to scroll the selection out of view using the mouse wheel; after
doing this, it would sometimes scroll into view by itself again, for example
when a background fetch occurred. In the files panel this would even happen
every 10s with every regular files refresh.
Fix this by adding a scrollIntoView parameter to HandleFocus, which is false by
default, and is only set to true from controllers that change the selection.
This doesn't really solve a pressing problem, because I guess it's unlikely that
users add spaces at the beginning or end of what they type into a prompt; but it
could happen, and in this case we almost always want to strip it. Just adding
this here for completeness while I was working on this code.
The only exception is the input prompt of custom commands, because who knows
what users want to use that input for in their custom command.
Most of our prompts don't (shouldn't) allow empty input, but most callers didn't
check, and would run into cryptic errors when the user pressed enter at an empty
prompt (e.g. when creating a new branch). Now we simply don't allow hitting
enter in this case, and show an error toast instead.
This behavior is opt-out, because there are a few cases where empty input is
supported (e.g. creating a stash).
This is an object that is owned by Gui, is accessible through GuiCommon.State(),
and also passed down to GitCommand, where it is mostly needed. Right now it
simply wraps access to the Git.Paging config, which isn't very exciting, but
we'll extend it in the next commit to handle a slice of pagers (and maintain the
currently selected pager index), and doing this refactoring up front allows us
to make that change without having to touch clients.
In all other menus besides the keybindings menu it makes sense to hide
keybindings that match the confirmMenu binding. This is important to make it
clear which action will be triggered when you press the key.
In the keybindings menu this is different; the main purpose of that menu is not
to allow triggering commands by their key while the menu is open, but to serve
as a reference for what the keybindings are when it is not open. Because of
this, it is more important to show all bindings in this menu, even if they
conflict with the confirmMenu key.
This fixes a regression introduced in b3a3410a1a.
So far, confirmations and prompts were handled by the same view, context, and
controller, with a bunch of conditional code based on whether the view is
editable. This was more or less ok so far, since it does save a little bit of
code duplication; however, now we need separate views, because we don't have
dynamic keybindings, but we want to map "confirm" to different keys in
confirmations (the "universal.confirm" user config) and prompts (hard-coded to
enter, because it doesn't make sense to customize it there).
It also allows us to get rid of the conditional code, which is a nice benefit;
and the code duplication is actually not *that* bad.
Using the filtered one is probably not a good idea. It didn't do much harm
because the split of ReflogCommits and FilteredReflogCommits doesn't really work
right now (FilteredReflogCommits is always the same as ReflogCommits, even in
filtering mode), but we'll fix this in the next commit.
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.
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.
Refresh is one of those functions that shouldn't require error handling (similar
to triggering a redraw of the UI, see
https://github.com/jesseduffield/lazygit/issues/3887).
As far as I see, the only reason why Refresh can currently return an error is
that the Then function returns one. The actual refresh errors, e.g. from the git
calls that are made to fetch data, are already logged and swallowed. Most of the
Then functions do only UI stuff such as selecting a list item, and always return
nil; there's only one that can return an error (updating the rebase todo file in
LocalCommitsController.startInteractiveRebaseWithEdit); it's not a critical
error if this fails, it is only used for setting rebase todo items to "edit"
when you start an interactive rebase by pressing 'e' on a range selection of
commits. We simply log this error instead of returning it.
Instead, pass the entire Mutexes struct by pointer to controllers.
This is better because Mutexes now has a zero value and doesn't need to be
initialized.
If a DisabledReason has its AllowFurtherDispatching flag set, it is returned as
a ErrKeybindingNotHandled error, instead of shown as a toast right away. This
allows gocui to continue to dispatch the keybinding, and we can unwrap the error
at the other end (in our global ErrorHandler) and display it then.
This allows having keybindings for the same key at the local and global levels,
and they will continue to be dispatched even if the first one returns a
DisabledReason. It is opt-in, so we only use it for cases where we know that a
local and a global handler share the same (default) keybinding.
This in itself is not an improvement, because hashes are unique (they are shared
between real commits and rebase todos, but there are so few of those that it
doesn't matter). However, it becomes an improvement once we also store parent
hashes in the same pool; but the real motivation for this change is to also
reuse the hash pointers in Pipe objects later in the branch. This will be a big
win because in a merge-heavy git repo there are many more Pipe instances than
commits.
