This was recently introduced, but it was done the wrong way.
WithWaitingStatusSync should only be called from the main thread, and it is
meant to be used for updating the bottom line while the UI is blocked. It is a
bad idea to call this from a background thread, and it results in ugly flicker
(occasionally).
Use the newly extracted WithWaitingStatusImpl instead, this is the same as
WithWaitingStatus (which is exactly what we need) but without the implicit
OnWorker, which we don't want because we are on a background thread already.
This shows a status as if the user had typed 'f' manually in the files panel.
I want this particularly for the first fetch after startup. There are often
situations where I need to wait for this first background fetch to be done
before I can do what I want (e.g. rebase my branch onto its base branch, or
check out a branch that my coworker has told me they just pushed), but currently
it's hard to tell when that is.
For every subsequent background fetch after the first one it is less important,
but it hopefully doesn't hurt, and it might be nice to have some visual
indication that background activity is happening.
As far as I can tell, this is not needed. The call to Refresh at the end of
backgroundFetch takes care of redrawing after refreshing.
The call was added in 2fc1498517, that's a long time ago, and we had multiple
big refactorings since then. Maybe it was needed back then but no longer is
today.
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.
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 lets us get rid of a few more calls to Error(), and it simplifies things
for clients of OnWorker: they can simply return an error from their callback
like we do everywhere else.
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.
The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.
In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).
Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
This begins a big refactor of moving more code out of the Gui struct into contexts, controllers, and helpers. We also move some code into structs in the
gui package purely for the sake of better encapsulation
