With sync vs async now derived from the calling thread, the Mode field
and its SYNC/ASYNC constants no longer carry any information: Refresh is
always async, RefreshFromWorker always sync. Drop the field, the type,
and the Mode argument at every call site, and reduce the debug log's
mode name to a plain sync/async derived from calledFromWorker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This is preparation for upcoming commits that will bounce refresh-scope
model updates (e.g. Model.Files) onto the UI thread by enqueuing the
write via OnUIThread instead of applying it directly on the worker
goroutine. Once that lands, a Then callback that reads the model must
run after that queued write has been processed, not synchronously at
wg.Wait() time — at that point the workers have returned, but a bounce
they queued may not have been processed yet.
Queuing Then via OnUIThread here, ahead of that change, guarantees the
right ordering once it lands: a bounce queued earlier in the same
refresh is already sitting in the channel by the time wg.Wait()
returns, so Then enqueued after it will always be processed after, and
see the post-refresh model.
The signature change to func() error lets Then propagate errors
through gocui's normal error handler (the same path key-handler errors
take).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Constructing a menu item key from a literal character requires
gocui.NewKeyRune('r'), which is a bit noisy. Add a private menuKey helper in
both the controllers and helpers packages so the common case in either reads as
menuKey('r'). Duplicating the one-liner is cheaper than a cross-package import
dependency and avoids forcing every controller file to qualify the call.
The reason for doing this now is that we are going to change MenuItem.Key to a
slice of keys later in the branch, which means we'd have to add `[]gocui.Key{`
at each call site, making them even more noisy. With the menuKey helper we can
just change its signature and leave all clients unchanged.
This bundles the keyName and a rune, so that we don't have to pass these around
separately everywhere. This should make it easier to swap out the rune for a
string when we upgrade to tcell v3.
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.
I took the set of enabled checks from revive's recommended configuration [1],
and removed some that I didn't like. There might be other useful checks in
revive that we might want to enable, but this is a nice improvement already.
The bulk of the changes here are removing unnecessary else statements after
returns, but there are a few others too.
[1] https://github.com/mgechev/revive?tab=readme-ov-file#recommended-configuration
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.
Something dumb that we're currently doing is expecting list items
to define an ID method which returns a string. We use that when copying
items to clipboard with ctrl+o and when getting a ref name for diffing.
This commit gets us a little deeper into that hole by explicitly requiring
list items to implement that method so that we can easily use the new
helper functions in list_controller_trait.go.
In future we need to just remove the whole ID thing entirely but I'm too
lazy to do that right now.
We want to show an error when the user tries to invoke an action that expects only
a single item to be selected.
We're using the GetDisabledReason field to enforce this (as well as DisabledReason
on menu items).
I've created a ListControllerTrait to store some shared convenience functions for this.
The only time we should call SetSelectedLineIdx is when we are happy for a
select range to be retained which means things like moving the selected line
index to top top/bottom or up/down a page as the user navigates.
But in every other case we should now call SetSelection because that will
set the selected index and cancel the range which is almost always what we
want.
For marking as good or bad, the current commit is pretty much always the one you
want to mark, not the selected. It's different for skipping; sometimes you know
already that a certain commit doesn't compile, for example, so you might
navigate there and mark it as skipped. So in the case that the current commit is
not the selected one, we now offer two separate menu entries for skipping, one
for the current commit and one for the selected.
This can be useful if you want to find the commit that fixed a bug (you'd use
"broken/fixed" instead of "good/bad" in this case), or if you want to find the
commit that brought a big performance improvement (use "slow/fast"). It's pretty
mind-bending to have to use "good/bad" in these cases, and swap their meanings
in your head.
Thankfully, lazygit already had support for using custom terms during the bisect
(for the case that a bisect was started on the command-line, I suppose), so all
that's needed is adding a way to specify them in lazygit.
We have not been good at consistent casing so far. Now we use 'Sentence case' everywhere. EVERYWHERE.
Also Removing 'Lc' prefix from i18n field names: the 'Lc' stood for lowercase but now that everything
is in 'Sentence case' there's no need for the distinction.
I've got a couple lower case things I've kept: namely, things that show up in parentheses.
more
and more
move rebase commit refreshing into existing abstraction
and more
and more
WIP
and more
handling clicks
properly fix merge conflicts
update cheatsheet
lots more preparation to start moving things into controllers
WIP
better typing
expand on remotes controller
moving more code into controllers
Stefan HallerandClaude Opus 4.8
pikomondeandStefan Haller
Jesse Duffield
Moritz HaaseandJesse Duffield