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.
When rerendering a view at the end of a refresh, we call HandleFocus only if the
view has the focus. This is so that we rerender the main view for the new
selection.
What was missing here is to update the view selection from the list selection if
the view doesn't have the focus, so that the selection is painted properly.
Normally this is not relevant because you don't see the selection if another
side panel has the focus; however, you do see it as an inactive selection when
e.g. a popup is shown, in which case it does matter.
This will become more important when we introduce section headers for commits,
because in that case the view selection needs to change when the working copy
state changes from normal to rebasing or vice versa, even if the list selection
stays the same.
The changed test submodule/reset.go shows how this was wrong before: when
entering the submodule again after resetting, there is a refresh which keeps the
same branch selected as before (master); however, since the branches panel is
not focused, the view didn't notice and kept thinking that the detached head is
selected (which it isn't, you can tell by running the test in sandbox mode and
focusing the branches panel at the end: you'll see that master is selected). So
the change in this commit fixes that.
It looks like enums.go was supposed to be file that collects a bunch of enums,
but actually there's only one in there, and since it has methods, it deserves to
be in a file of its own, named after the type.
Apparently this was an attempt at working around go's lack of default arguments,
but it's very unidiomatic and a bit confusing. Make it a normal parameter
instead, so all clients have to pass it explicitly.
There are two ways to jump to the editor on a specific line: pressing `e` in the
staging or patch building panels, or clicking on a hyperlink in a delta diff. In
both cases, this works perfectly in the unstaged changes view, but in other
views (either staged changes, or an older commit) it can often jump to the wrong
line; this happens when there are further changes to the file being viewed in
later commits or in unstaged changes.
This commit fixes this so that you end up on the right line in these cases.
This might seem controversial; in many cases the client code gets longer,
because it needs an extra line for an explicit `return nil`. I still prefer
this, because it makes it clearer which calls can return errors.
This solves several problems that arise from opening a menu while the prompt is
open. We might try to solve these in a different way, e.g. by dismissing the
search prompt before opening a menu, but restricting what you can do while the
prompt is open seems like the more robust fix.
To achieve this, we
- call resetKeyBindings both when opening and when closing the search/filter
prompt
- change the keybindings to only contain the ones for the search prompt when
that context is active.
In other views that show lists of commits (reflog and stash) it doesn't make
sense to show a range diff of selected entries because they don't form a linear
sequence, so we keep the previous behavior of showing the diff for the free end
of the selection range in those view.
The same applies to the commits view if the selection range includes rebasing
todos; these can have an arbitrary order, and a range diff doesn't make sense
for those.
