Previously we would call pullFiles() from the pick() handler if we were not in a
rebase, assuming that the default keybinding for both is "p". This needn't be
the case of course, if the user has remapped one or the other.
The consequence of this was that swapping the keybindings for "pullFiles" and
"pushFiles" would work in all panels except the Commits panel (unless "pick" was
also remapped in the same way).
Fix this by using the new AllowFurtherDispatching mechanism of DisabledReasons
to pass the keybinding on to the next handler.
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.
There was no reason to declare a variable for disabledReason, assign it inside
the "if binding.GetDisabledReason != nil" statement, and then check its value
again after that if statement. Move all that code inside the first if statement
to make the control flow easier to understand.
Adaptions are for this gocui commit:
Cleanup: remove Is* error functions
- Use errors.Is instead of quality comparisons. This is better because it
matches wrapped errors as well, which we will need later in this branch.
- Inline the errors.Is calls at the call sites. This is idiomatic go, we don't
need helper functions for this.
See https://go.dev/blog/go1.13-errors for more about this.
When switching between repos, each repo might have a different focused panel; in
this case, the previously focused panel would show the "inactive" highlight. By
default this is only bold text, so it's barely noticeable, but it becomes more
pronounced when setting e.g.
gui:
theme:
inactiveViewSelectedLineBgColor:
- "#666666"
I noticed this especially when entering or leaving submodules; for example,
enter a submodule by pressing enter in the Files panel, then switch to the
Commits panel in the submodule, then press Esc to go back to the parent repo.
This would put the focus back into the Files panel, but keep the inactive
highlight in the Commits panel.
Moving the getter of the suggested branch name to a separate function
allows us to reuse it in situations where we are not calling the regular
create new branch function, such as move commits to a new branch
Signed-off-by: Elias Assaf <elyas51000@gmail.com>
When refreshing the branches list, we have code to keep the same branch selected
even when the refresh changes the sort order; this code remembers the selected
branch before the refresh, and then tries to select it again afterwards (looking
it up by name) if it is still there.
However, we stored the previously selected branch too early, before even
obtaining the branches list; if the user moved the selection between that point
and the end of the refresh, it would jump back. Fix this by remembering the
previous selection only at the last moment, right before assigning the new
branches slice.
We still have a race condition here between the UI code that manages the
selection as the user presses arrow keys, and the background thread doing the
refresh that reads and restores the selection; however, the race was there
before, and we make it neither better nor worse with this PR. It doesn't seem to
be a problem in practice.
We will need a user config in the file tree in the next commit, and passing the
entire common is the easiest way to do that while ensuring hot-reloading when
users change the config while lazygit is running.
The click handler of MainViewController was registered as a global handler, so
it was used when a side panel was focused that doesn't have a
SwitchToFocusedMainViewController attached (e.g. Status, Worktrees, or
Submodules). This handler would then push the main view context, but with the
code that is meant only for toggling between the main view pair contexts, i.e.
with taking over the parentContext from the otherContext, which doesn't have one
at that point. This would later lead to a crash in onClick because the
parentContext was nil.
Fix this by splitting the click handler in two, one for when it already has the
focus, and one for toggling from the other view, and make these focus specific.
Previously we would re-render the focused main view several times during a
refresh, once for every side panel. While this should usually not be noticeable
for users because we are careful to avoid flicker when refreshing the main view,
this would sometimes lead to the main view scrolling up to the top by itself;
see PR description for more details.
I'm not aware of any real scenario where this can happen, but we have seen one
stack trace where it crashed with an out-of-bounds error in the range expression
below, so there must be a way. And it seems better to guard against it anyway,
rather than assuming it can't happen.
This now allows for leaving the status panel and returning back to the
same log command. Previously any return to the status panel would result
in the next command in the list being shown. Now, you need to press `a`,
with a log command being rendered, to rotate to the next
allBranchesLogCmd.
In one case it was actually called *before* making a commit (when switching from
the commit message panel to committing in the editor). And clearing the
preserved message is the only thing it does, so name it after what it does
rather than when it's called.
Previously it would only clear the message if the panel had been opened with
preserveMessage=true; we don't need this check, because callers know how they
opened the panel, and whether they want to clear the message.
This is no change in behavior because OnCommitSuccess only clears the message
when the commit message panel was opened with preserveMessage=true, which it
isn't in the case of creating a tag.
And this is in fact the desired behavior, because we don't want creating a tag
to interfere with preserving commit messages in any way.
The default binding for ConfirmInEditor is <a-enter>, which has two problems:
- some terminal emulators don't support it, including the default terminal on
Mac (Terminal.app)
- on Windows it is bound to toggling full-screen
Ideally we would use <c-enter> instead (and Command-Enter on Mac), but neither
is possible without https://github.com/gdamore/tcell/issues/671, so for the time
being add an alternate keybinding which works everywhere.
Show both bindings in the footer of the commit description panel if they are
both non-null. While we're at it, fix the footer for the case where either or
both of the keybindings are set to <disabled>.
And finally, change "commit" to "submit" in that footer; we use the same panel
also for creating tags, in which case "commit" is not quite right.
It's never called, the binding ListController.HandleGotoBottom wins.
The functionality of loading more commits is implemented by GetOnFocus, and this
way it works not only for '>', but also for other navigation keys like page
down.
Hopefully, graphs will never get wider than 32768 characters. (They would get
kind of hard to navigate if they did...)
This reduces the size of the Pipe struct from 48 to 32 bytes, which makes a
significant difference when there are many millions of instances.
This saves some memory at the cost of a slight performance increase (I suppose
reallocting the slice when adding new Pipes is slightly more expensive now).
Performance of the BenchmarkRenderCommitGraph benchmark is 130μs before, 175μs
after. I'm guessing this is still acceptable.
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.
The "// merge commit" comment was plain wrong, this is any commit that has a
parent, merge or not. The "else if" condition was unnecessary, a plain "else"
would have been enough. But the code in the two blocks was almost identical, so
extract the one thing that was different and unify it.
And while we're at it, use IsFirstCommit() instead of counting parents.
This makes it easier to copy diff hunks and paste them into code. We only strip
the prefixes if the copied lines are either all '+' or all '-' (possibly
including context lines), otherwise we keep them. We also keep them when parts
of a hunk header is included in the selection; this is useful for copying a diff
hunk and pasting it into a github comment, for example.
A not-quite-correct edge case is when you select the '--- a/file.txt' line of a
diff header on its own; in this case we copy it as '-- a/file.txt' (same for the
'+++' line). This is probably uncommon enough that it's not worth fixing (it's
not trivial to fix because we don't know that we're in a header).
This is very old; I can only guess that this was added at a time where today's
list column handling wasn't in place yet, so the space was needed to separate
columns. This now causes a gap of two spaces between the rebase todo column and
the author column, which I'm sure wasn't intended. Funny that I never noticed.
This is very similar to what we are doing for staging or discarding hunks in the
Files panel. Git doesn't allow applying patches with a zero context size (unless
you use the --unidiff-zero option, which is discouraged).
