It should have been half-screen from the get-go. I think I just used
full-screen to make demos look nicer. Now that we have a CLI arg for the
screen mode we can make use of that in the demos.
Introduce a new "screen-mode" command line argument that allows a user
to specify which screen mode (normal, half or full) Lazygit should use
when it runs.
This argument will take precedence over a default Window Size specified
in user config.
When the user checks out a commit which has a local branch ref attached
to it, they can select between checking out the branch or checking out
the commit as detached head.
Sometimes we populate the commit message panel with a pre-created commit
message. The two cases where this happens is:
- you type `w` to commit, in which case we put the skipHookPrefix in the subject
- you have a commitPrefix pattern, in which case we match it against the branch
name and populate the subject with the replacement string if it matches
In either case, if you have a preserved commit message, we use that.
Now, when you use either of these and then cancel, we preserve that initial,
unchanged message and reuse it the next time you commit. This has two problems:
it strips spaces, which is a problem for the commitPrefix patterns, which often
end with a space. And also, when you change your config to experiment with
commitPrefix patterns, the change seemingly doesn't take effect, which can be
very confusing.
To fix both of these problems, only preserve the commit message when it is not
identical to the initial message.
This makes it so that when the staging view is resized, we keep the same patch
line selected (as opposed to the same view line, which may correspond to a
different patch line after resizing). It doesn't seem like a terribly important
feature for resizing the window, but it is essential when initially entering the
staging view: we select the first line of the first hunk in this case, but we do
that before layout runs. At layout time the view is then split into
unstaged/staged changes, and if this split is horizontal, the view gets narrower
and may be wrapped in a different way. With this commit we ensure that the first
line of the first hunk is still selected after that.
So far, lines in the view corresponded 1:1 to lines in the patch. Once we turn
on wrapping for the staging view (which we don't do yet), this is no longer
true, so we need to convert from view lines to patch lines or vice versa all
over the place.
to make it more generally usable by clients other than ConfirmationHelper, which
we will do later in this branch. Rename it to WrapViewLinesToWidth while we're
at it.
Add tests; in particular, add a sanity check that we wrap lines the same way as
gocui does. The tests that are added here are the same ones as in gocui for its
lineWrap function, but we'll extend them a bit in later commits in this branch.
This is also what we do in the staging controller, and it makes it so that when
you exit the patch building view and then enter it again (for another file, or
the same one) we select the first hunk again.
For non-merge commits we change "pick" to "drop" when we delete them. We do this
so that we can use the same code for dropping a commit no matter whether we are
in an interactive rebase or not. (If we aren't, we could just as well delete the
pick line from the todo list instead of setting it to "drop", but if we are, it
is better to keep the line around so that the user can change it back to "pick"
if they change their mind.)
However, merge commits can't be changed to "drop", so we have to delete them
from the todo file. We add a new daemon instruction that does this.
We still don't allow deleting a merge commit from within an interactive rebase.
The reason is that we don't show the "label" and "reset" todos in lazygit, so
deleting a merge commit would leave the commits from the branch that is being
merged in the list as "pick" commits, with no indication that they are going to
be dropped because they are on a different branch, and the merge commit that
would have brought them in is gone. This could be very confusing.
Not much of a change in behavior, because moving merge commits was already not
possible. However, it failed with a cryptic error message ("Todo fa1afe1 not
found in git-rebase-todo"), so disable it properly instead.
When enabled, it adds "+n -m" after each file in the Files panel to show how
many lines were added and deleted, as with `git diff --numstat` on the command
line.
It is needed by both call sites of this function. This has the added benefit
that the argument doesn't unnecessarily show up in the status view when diffing
mode is on.
In 67b8ef449c we changed the "edit" command to insert a "break" after the
selected commit, rather than setting the selected todo to "edit". The reason for
doing this was that it now works for merge commits too.
Back then, I claimed "In most cases the behavior is exactly the same as before."
Unfortunately that's not true, there are two reasons why the previous behavior
was better (both are demonstrated by tests earlier in this branch):
- when editing the last commit of a branch in the middle of a stack of branches,
we are now missing the update-ref todo after it, which means that amending the
commit breaks the stack
- it breaks auto-amending (see the added test earlier in this branch for an
explanation)
For these reasons, we are going back to the previous approach of setting the
selected commit to "edit" whenever possible, i.e. unless it's a merge commit.
The only scenario where this could still be a problem is when you have a stack
of branches, and the last commit of one of the branches in the stack is a merge
commit, and you try to edit that. In my experience with stacked branches this is
very unlikely, in almost all cases my stacked branches are linear.
We allow deleting remote branches (or local and remote branches) only if *all*
selected branches have one.
We show the a warning about force-deleting as soon as at least one of the
selected branches is not fully merged.
The added test only tests a few of the most interesting cases; I didn't try to
cover the whole space of possible combinations, that would have been too much.
Since we want to select multiselections, this will make it easier to pass a
slice of remote branches. It does require that for the case of the local
branches panel we need to synthesize a RemoteBranch object from the selected
local branch, but that's not hard.
Mouse wheel scrolling of the custom patch view worked *unless* a file (as
opposed to a directory) is selected in the commit files view. The reason was an
obvious typo in the AttachControllers call.
This code had a lot of logic that (fortunately) didn't work because it was
buggy:
- it was supposed to wait for the auto-fetch delay before fetching for the first
time in case we start with a repo that we had open in a previous session (i.e.
that appears in the recent repos list). This code actually ran always, not
just for known repos, because the IsNewRepo flag is only set later, after this
function runs. Fortunately, the code didn't work, because time.After starts a
timer but doesn't wait for it (to do that, it would have to be
`<-time.After`).
- if the first fetch fails with error 128, it was supposed to show an error
message and not start the background fetch loop. Fortunately, this didn't work
because 1) it was guarded by isNew which is always false here, and 2) because
git's error message in this case is actually "exit code: 128", not "exit
status 128" (maybe this has changed in git at some point).
I find both of these undesirable. Whenever I open a repo I want an auto-fetch to
be triggered immediately to get my branch information up to date as quickly as
possible. And if the initial fetch fails (e.g. because one of my remotes is
offline or doesn't exist any more), then that's no reason not to start the
auto-fetch loop anyway.
So let's simplify the code to do what it did before, but with much fewer lines
of code.
Original commit message of the gocui change:
This fixes View.Size, Width and Height to be the correct (outer) size of a view
including its frame, and InnerSize/InnerWidth/InnerHeight to be the usable
client area exluding the frame. Previously, Size was actually the InnerSize (and
a lot of client code used it as such, so these need to be changed to InnerSize).
InnerSize, on the other hand, was *one* less than Size (not two, as you would
have expected), and in many cases this was made up for at call sites by adding 1
(e.g. in calcRealScrollbarStartEnd, parseInput, and many other places in the
lazygit code).
There are still some weird things left that I didn't address here:
- a view's lower-right coordinates (x1/y1) are one less than you would expect.
For example, a view with a 2x2 client area like this:
╭──╮
│ab│
│cd│
╰──╯
in the top-left corner of the screen (x0 and y0 both zero) has x1/xy at 3, not
4 as would be more natural.
- a view without a frame has its coordinates extended by 1 on all sides; to
illustrate, the same 2x2 view as before but without a frame, sitting in the
top-left corder of the screen, has coordinates x0=-1, y0=-1, x1=2, y1=2. This
is highly confusing and unexpected.
I left these as they are because they would be even more of a breaking change,
and also because they don't have quite as much of an impact on general app code.
When creating a PR against a selected branch (via O = "create pull request
options"), the user will first be asked to select a remote (if there is more
than one). After that, the suggestion area is populated with all remote branches
at that origin - instead of all local ones. After all, creating a PR against a
branch that doesn't exist on the remote won't work.
Please note that for the "PR is not filed against 'origin' remote" use case
(e.g. when contributing via a fork that is 'origin' to a GitHub project that is
'upstream'), the opened URL will not be correct. This is not a regression and
will be fixed in an upcoming PR.
Fixes#1826.
As far as I understand, this was needed back when the staging context was still
responsible for rendering its highlight (as opposed to the gocui view, as it is
today). It was necessary to call it with isFocused=false so that it removed the
highlight. The isFocused bool is no longer used today (and we'll remove it in
the next commit), so there's no need to render the view here any more.
This fixes flickering when leaving the staging view by pressing escape. The
reason is that in this case the patch state was already set to nil by the time
we get here, so we would render an empty view for a brief moment.
On top of that, it fixes unwanted scrolling to the top when leaving the staging
view. The reason for this is that we have code in layout that scrolls views up
if needed, e.g. because the window got taller or the view content got shorter
(added in #3839). This kicked in because we emptied the view, and scrolled it
all the way to the top, which we don't want.
This way it won't scroll to the top; we want this when entering the staging
panel or the patch building panel by clicking into the view, and also when
returning from these views by pressing escape. Note that there's a bug in this
latter case: the focused panel still scrolls to the top when hitting escape, we
will fix this in the next commit.
Change it in the same way for NewRenderStringWithScrollTask, just for
consistency, although it's not really necessary there. We use this function only
for focusing the merge conflict view, and in that case we already have an empty
task key before and after, so it doesn't change anything there.
When clicking in a single-file diff view to enter staging (or custom patch
editing, when coming from the commit files panel), and then pressing shift-down
or shift-up to select a range, it would move the selected line rather than
creating a range. Only on the next press would it start to select a range from
there.
This is very similar to the fix we made for pressing escape in 0e4d266a52.
So far we only had tests that called Click() only once. If you have a test that
calls Click twice (we'll add one in the next commit), you'll notice that the
second click is interpreted as a drag because the mouse button wasn't released
in between. Fix this by sending a "mouse-up" event after the click.
After pasting commits once, we hide the cherry-picking status (as if it had been
reset), and no longer paint the copied commits with blue hashes; however, we
still allow pasting them again. This can be useful e.g. to backport a bugfix to
multiple major version release branches.
The string literal "\uf0868" does *not* create a single rune with the code point
f0868, as was intended; instead, it creates two runes, one with the code point
f086, followed by the character '8'.
Currently we try to delete a branch normally, and if git returns an error and
its output contains the text "branch -D", then we prompt the user to force
delete, and try again using -D. Besides just being ugly, this has the
disadvantage that git's logic to decide whether a branch is merged is not very
good; it only considers a branch merged if it is either reachable from the
current head, or from its own upstream. In many cases I want to delete a branch
that has been merged to master, but I don't have master checked out, so the
current branch is really irrelevant, and it should rather (or in addition) check
whether the branch is reachable from one of the main branches. The problem is
that git doesn't know what those are.
But lazygit does, so make the check on our side, prompt the user if necessary,
and always use -D. This is both cleaner, and works better.
See this mailing list discussion for more:
https://lore.kernel.org/git/bf6308ce-3914-4b85-a04b-4a9716bac538@haller-berlin.de/
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.
I can only guess, but I think this was a typo (or a copy-paste-o) when this code
was written. It was introduced in 55af07a1bb, and I think the defer was kept by
accident; if it had been on purpose, then the statement would have been put
right after the Lock call.
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.
So far, lazygit has always auto-staged files as soon as the conflict markers
disappeared from them, which means that we could rely on any file that still had
a status of "UU" to still contain conflict markers.
We are going to make the auto-staging optional in the next commit, and in that
case the user will want to manually stage "UU" files; so we must now check
whether the file contains conflict markers, and disallow the staging in that
case.
We make the name of the GetSelectedRefRangeForDiffFiles very specific on purpose
to make it clear that this is only for switching to diff files, so the
implementations can make assumptions about that (unlike GetSelectedRef, which is
used for different purposes and needs to stay more generic).
Right now it doesn't do very much yet, but it's still worth it even in this
state, I'd say. The function is going to become a bit longer in the next commit,
though.
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.
Hopefully this will help alleviate the problem that diffing mode is sticky, and
you often forget that it's still on.
Make it magenta like the mode text in the information view.
This fixes two problems:
- Set the title ref to the commit description (like
SwitchToDiffFilesController.enter does), rather than just the hash
- SetTitleRef only sets the title of the DynamicTitleBuilder, but doesn't set it
on the view; this only happens in ContextMgr.Activate, so you'd have to switch
to a different side panel and then back to see the new title.
The three nested `if` statements may looks strange, and you might wonder why we
don't have single one with &&. The answer is that later in this branch we will
add an `else` block to the middle one.
In the next commit it will be reused for checking whether we need to reset the
patch; and extracting it makes it easier to extend it for non-sticky diff ranges
later in the branch.
viewFiles is only called from enter; it doesn't make much sense to fill in a
SwitchToCommitFilesContextOpts struct to pass it to viewFiles for this one call.
Simply inline viewFiles into enter and get rid of all that.
I found this indirection confusing when reading the code. It looks like
SwitchToDiffFilesController is instantiated with different such contexts, but
it's always Contexts.CommitFiles, so just use that directly.
Note that this doesn't only affect the diff views, which are the ones where we
want this, but also the status view, where the plan was to keep them underlined
always for better discoverability. We could make this configurable dynamically
(by adding another flag to ViewUpdateOpts), but actually I think it's fine this
way.
We fixed one specific scenario where this happened ealier in this branch, but in
case there are more that we don't know about yet, at least make sure we don't
crash.
