Show progress indicator for more operations, e.g. "discard changes"

[grossjohann]

Is your feature request related to a problem? Please describe. When pushing and pulling, a progress indicator shows up that lets me know that the keypress has been received and lazygit is working on it.

But there are other operations that can take a longer time, such as rebasing, or "discard all changes" when the file tree is showing.

Describe the solution you'd like Show a progress indicator, similar to the one for push and pull.

Describe alternatives you've considered I wonder whether the indicator would be disruptive in case the operation is fast.

Additional context Add any other context or screenshots about the feature request here.

[jesseduffield]

I completely agree with this. We currently have two ways to show a loader: one in a blocking popup and one in the bottom-left. I much prefer the one in the bottom-left and I think we should use this for anything new.

We should do an audit of commands that can be long-running and that don't already have loader a loader applied and apply loaders to them.

This is actually straightforward to implement: it's just a matter of wrapping the action in the WithWaitingStatus function e.g.: going from

HandleConfirm: func() error {
    self.c.LogAction(self.c.Tr.Actions.FixupCommit)
    return self.interactiveRebase(todo.Fixup)
},

to

HandleConfirm: func() error {
    return self.c.WithWaitingStatus(self.c.Tr.FixingStatus, func(gocui.Task) error {
        self.c.LogAction(self.c.Tr.Actions.FixupCommit)
        return self.interactiveRebase(todo.Fixup)
    })
},

The only hard part is defining a loading message e.g. self.c.Tr.FixingStatus in the above snippet is defined in pkg/i18n/english.go as Fixing up.

I'm gonna chuck a good-first-issue label on this. Would you be game to raise a PR for this @grossjohann ?

[kgrossjo]

Thank you @jesseduffield, I have started with “rebase / simple”, but now I'm struggling to figure out if it works. I tested it with the lazygit repo by checking out the branch copper and trying to rebase it on top of master. But there is no progress indicator, the rebase finishes instantly.

Hm.

Ah. I can put a sleep in (*gitCmdObjRunner).RunWithOutput. Oh yes, that shows a little spinner in the bottom left corner. So let me submit this PR.

I don't know go, I just made the compiler believe I do...

[stefanhaller]

TL;DR: please don't change every operation to WithWaitingStatus mindlessly. On the contrary, some operations actually benefit from removing the WithWaitingStatus.

While I agree that in many cases doing things async with WithWaitingStatus is a good idea, both for visual feedback and for responsiveness of the UI, it's not always the case. There are situations where async operations do more harm than good.

My favorite example is VS Code, which performs almost everything async, and that's great in principle, except when it isn't. Here's an example: you have a tab with modifications and you want to close it, so you press Command-S, Command-W in quick succession, to save the changes and close the window. Now, saving the file is done asynchronously, and if you have "format on save" turned on and your auto-formatter is slow, it might not be done by the time the "close" command is dispatched. In that case you get a dialog asking you whether you want to save the changes. That's super annoying; yes, thank you, I do want to save my changes, and I just told you so by pressing Command-S. As a user, I have to be careful not to type things too quickly; I have to pay attention to the status bar to see when the modified marker has gone away, and only press Command-W then. But I don't want to spend mental energy on that, I want to work quickly. Had the "save" command been done synchronously, we wouldn't have this problem.

In lazygit, a command that is similar is ctrl-j/k to move a commit up or down. This has two problems: 1) if I type the key twice to move a commit by two steps, the second one is likely to fail with a confusing error message if I type too quickly, and 2) there's a slight visual glitch because the commit and the selection don't move exactly in lockstep (to be fair, this one is hard to see because we don't do a FocusLine after moving the selection, so it's only the graph update that shows the glitch). Both of these can be fixed by removing the WithWaitingStatus, and changing the refresh at the end to Mode: types.SYNC. For me, this is a big improvement in usability.

Of course it's a pity that we don't have visual feedback any more then. But since drawing to gocui views appears to be thread safe, I wonder if we can have a WithWaitingStatusSync that launches a background go routine and draws to the status bar periodically, updating only the status bar while the blocking operation is running on the main thread? I experimented with this a little bit today, but couldn't get it to work yet.

@jesseduffield Very interested in your thoughts on this.

[jesseduffield]

TL;DR: I agree, we should have a WithWaitingStatusSync to resolve issues like moving commits up/down.

I totally agree that we need a way to show a loader without processing any more user actions, to solve certain problems like moving commits.

But the fact that with moving commits we've got an annoying waiting period between each move tells me that perhaps we should focus on improving that UX, especially because I can't think of any cases other than moving commits where I've felt like I needed to pace my keypresses.

We could have UX where you specify the change you want to make and only afterwards you apply the change (meaning only one action requires a loader). You can already do this by hitting 'e' on a base to edit and then moving the target commit down, then continuing the rebase. But that approach requires more keypresses and brain cells. What about an approach where upon trying to move a commit down we enter a pre-rebase mode (internal to lazygit) where we build up a todo file and then once you 'submit' the todo file we work out which commit should be the base for the interactive rebase and then do all the work.

Some arguments against this idea:

• need to support two kinds of rebase mode (pre-rebase and rebase)
• violates my principle of letting git maintain as much state as possible
• you may want to actually do a full rebase for each step of moving the commit down to see where it conflicts

[stefanhaller]

Good points. I'm torn on changing the UX for moving up/down; in smaller repos it's perfectly acceptable the way it is (if we make it blocking, that is). It's only in bigger repos that it's not ideal. I use the e workaround a lot in our work repo, and I find it acceptable. And I agree that occasionally it's useful to move one step at a time to see where it starts to conflict. So for me, changing the UX is not a priority right now.

And it's not just about having to pace my keypresses; it's also about avoiding visual glitches. I find it important that the commit list and the selected line update at the same time; this might seem like unimportant polishing, but I find that it contributes a lot to the perception of how solid lazygit is. Another example: when reverting a commit we move the selection down by one to keep the same commit selected; right now there's no glitch here, but that's only because the whole thing is done on the UI thread. If we were to add a WithWaitingStatus, it would show the same ugly glitch that moving up/down does right now. Another example is squashing fixups: I'm working on a PR that keeps the selection the same after squashing fixups, and this will have the same problem.

(Side note: there's another type of glitch that I find very ugly, and that's modified files in the files panel flashing up during rebase operations. This happens when the 10s background refresh kicks in during a rebase. You can provoke this to happen much more often by setting the refresh to 1s. This would also be fixed by using WithWaitingStatusSync, but only for those operations that use it; I have the feeling there should be a more general fix for this, by postponing the background refresh until there are no busy tasks.)

[stefanhaller]

Here's a proof-of-concept PR for WithWaitingStatusSync: #2966

[jesseduffield]

I agree with all your above points (and I too find the flashing files annoying)

[kgrossjo]

Is there any meaningful work I can do in lazygit while an operation is in progress? For example, if I move to another branch in the branches panel and rebase the current branch on top of the selected branch, I can't imagine anything meaningful to do until that rebase has finished.

• I think any modifying operation would confuse git a lot.
• But even read operations will not be very useful, since the current state changes rapidly while the rebase is in progress.

Perhaps one meaningful action I could do is to abort the current operation (e.g. the rebase that I just initiated).

So in my view, those operations should provide an indicator in the UI to show that they are in progress, plus maybe an option to abort them. And it doesn't really make sense to show the normal UI suggesting I could hit some keys with some useful effects.

I haven't tried recently, but I think it used to be the case that any keypress while pull or push was in progress would abort that operation. (I used to hit q while pull was in progress, expecting lazygit to return to quit after the pull, but I think it aborted the operation. Since then, I got too scared to press any key while pull (or push) is in progress.)

Perhaps I've overlooked something obvious here, in which case I apologize for having taken your time.

[jesseduffield]

Is there any meaningful work I can do in lazygit while an operation is in progress?

I'll often push my branch and while it's pushing I'll go and do something else. Even something as simple as navigating to the branch so that I can press 'o' once the push is done.

So in my view, those operations should provide an indicator in the UI to show that they are in progress, plus maybe an option to abort them.

We don't really have a way of aborting in-flight commands, in fact I think the only way to do it is to quit the program and I'm not sure whether it's safely killing commands in that case. But I agree it should be possible.

I haven't tried recently, but I think it used to be the case that any keypress while pull or push was in progress would abort that operation.

I don't believe this was ever the case. If you hit escape it'll close the loading popup but the operation will continue.

[stefanhaller]

I don't believe this was ever the case. If you hit escape it'll close the loading popup but the operation will continue.

Yes, but this is super confusing, because the status bar says <enter>: Confirm, <esc>: Close/Cancel while the loader panel is up. For a long time I thought that esc would actually abort the push; I had to read the code to find out that that's not the case. It would be very nice if we could remove that prompt for loader panels.

Actually, what was the reason again why push and fetch use loader panels and not just WithWaitingStatus?

[jesseduffield]

I agree, we are lying to the user haha.

The push/pull/fetch actions could use WithWaitingStatus. Those actions are typically slower than other actions and can raise password prompts so it's less surprising to see a password prompt after you've been looking at a loader popup for a few seconds (given the occupy the same position). But this is post-rationalisation. In the beginning I only had the loader popups for push/pull/fetch: no other actions had any loading states (either in popup form or otherwise). I added WithWaitingStatus for the sake of actions that could take time or could take a fraction of a second, so as not to distract the user.

[stefanhaller]

Won't the password prompt appear right at the beginning of the operation though? In that case it doesn't seem like a big problem to me.

Would you be ok with a PR that removes all loader panels and makes them waiting statuses?

[jesseduffield]

Won't the password prompt appear right at the beginning of the operation though? In that case it doesn't seem like a big problem to me.

Yeah: there will still be some delay but you're right it'll be at the start

Would you be ok with a PR that removes all loader panels and makes them waiting statuses?

Yep I think it's the right idea. I will miss those loader popups (they've been with me from the start) but there's not a good enough reason for them to exist when everything else uses the loader at the bottom-left.

[stefanhaller]

I need some help with getting the task pausing for the credentials input to work; after changing the WithLoaderPanel to WithWaitingStatus, the test push_with_credential_prompt.go hangs. What happens here is that WithWaitingStatus creates two OnWorker tasks: one in the function itself (this is the task that gets passed down and will be paused for the credentials input), and one inside of renderAppStatus, and this one won't be paused and causes the test to hang.

I tried changing the one in renderAppStatus to a simple go routine, thinking that it doesn't have to be a task at all. However, this causes lots of tests fail, and I don't really understand why. Any suggestions how to solve this?

I pushed a branch remove-loader-panel to your repo in case you want to have a look at what I have so far.

[kgrossjo]

Is there any meaningful work I can do in lazygit while an operation is in progress?

I'll often push my branch and while it's pushing I'll go and do something else. Even something as simple as navigating to the branch so that I can press 'o' once the push is done.

It makes it easy to shoot yourself in the foot, though: If you hit o too soon, then you have a problem.

So in my view, those operations should provide an indicator in the UI to show that they are in progress, plus maybe an option to abort them.

We don't really have a way of aborting in-flight commands, in fact I think the only way to do it is to quit the program and I'm not sure whether it's safely killing commands in that case. But I agree it should be possible.

Ah, yes. Actually, I pulled or pushed, hitting q while the loader panel was showing, expecting that lazygit would wait for the push/pull operation to finish, then quit. (The loader panel is even kind of telling me that lazygit is waiting! With a nicer spinner animation! It was totally unexpected to me that it was accepting any input at that point.)

Also for your o use case, I guess you just navigate to the right panel before hitting o, right? If lazygit were to process input after the operation finishes (instead of while it is still ongoing), then you could type P3o and then just wait for the web browser to show up.

But maybe this is just me being a greybeard; back when telnet sessions were often slow, I learned to anticipate what the server would do, and just queue up some keystrokes... (Nowadays people use ssh not telnet...) I still regularly type things like cd /u<TAB>/loc<TAB>/b<TAB> without waiting for the effect of the TAB presses...

[jwhitley]

@stefanhaller It appears that this issue was fixed in #2973. Is that correct?

[stefanhaller]

@jwhitley Not really. The issue was partly addressed in #2851, and then several different side topics were discussed in the comments of this issue, which were addressed in #2973 and #2966. But the real point of this issue is to audit all our commands and see which of them don't have a waiting status but should (e.g. discarding changes). This hasn't happened yet, so I'd leave the issue open.