jesseduffield
Refactor for better encapsulation
- PR Description
This PR refactors lots of gui code into helpers/controllers/contexts. It's goal is to whittle away at the Gui god struct that we have.
First off: don't freak out if you've got a PR in flight: I'm happy to hold off merging this for a while so that other people can get their stuff in.
Admittedly, I'm not really comfortable with the pattern of helpers/controllers/contexts either. But I'd rather go all-in on that pattern and then think about how to improve it than to have two patterns co-existing.
With this PR we still have two patterns co-existing because there's just lots of files to migrate, but given how many changes already exist in this PR, I'd rather merge it than add even more stuff to it.
Here's the new situation:
I'm gonna describe this situation how it is, knowing full-well that much of this is convoluted and should be changed in future.
We still have our gui package (with its gui god struct) doing much of the heavy lifting. It has visibility over basically everything.
Our contexts (in the context package) represent a view (in fact they each have a reference to the corresponding view) and encapsulate view-model stuff that's outside the view itself. There was a time when one context could be re-used with different views but that's no longer the case so in future I'd like to properly merge contexts and views together (contexts live in lazygit, views live in gocui). Contexts refer to the presentation package and use it to render the content in the view.
Controllers define keybindings and their corresponding actions, as well as some other things like what to render to the main view when the controller's context is focused, and what to do if the controller's focus gains/loses focus. One context can have multiple controllers, and controllers can be defined such that they take a context as an argument.
Controllers's methods can not be directly invoked from other code (which is potentially a bad decision). If one of a controller's actions needs to be invoked as part of some other action in another controller, the action can be moved into a helper.
Helpers contain general code for doing pretty much anything you want related to the gui.
Controllers and helpers do not maintain their own state but they can update the state of contexts via methods exposed by those contexts, and they can update the application state as well.
Each of controllers, helpers, and contexts, all have a 'common' struct passed in which provides commonly needed functionality. I'm aware that many see this as an anti-pattern, but if you only pass in exactly what you need to one of these structs' constructors, it adds a lot of friction. I'm happy to revise this pattern after we've migrated everything across.
To avoid circular dependencies, helpers that depend on other helpers must have those helpers passed in explicitly in their constructor. Controllers on the other hand can call a Helpers() method to access all helpers. Contexts have no access to helpers (again, to avoid cyclic dependencies).
There are some files in this PR which still live in the gui package but which are now wrapped in a struct for the sake of encapsulating the methods specific to that file. Some of these maintain their own state and that's subject to change.
Apologies for the convoluted architecture but Lazygit was my first Go program, and I had no experience with gui patterns (I'm still lacking). The code has grown in complexity with not many opportunities for DRYing things up (recall that generics were only recently introduced). In order to get a better handle on the code now, the first step is to break up the god struct for the sake of better method namespacing, and then decide what a cleaner architecture looks like.
I should mention some issues I specifically have with the current setup:
- it's not clear where state should live: if it all lives in one place, anybody has access to it.
- contexts and views are separate but should be the one thing
- controllers probably should just be very thin and specifically concerned about mapping keybindings to actions, and actions should be defined separately (probably in helpers or in a separate 'actions' thing)
- we're using a lot of indirection so that we don't have to care when certain structs have been instantiated. It would be better to just have a clearer setup step
TODO:
- [ ] clean up commits
- Please check if the PR fulfills these requirements
- [ ] Cheatsheets are up-to-date (run
go run scripts/cheatsheet/main.go generate) - [ ] Code has been formatted (see here)
- [ ] Tests have been added/updated (see here for the integration test guide)
- [ ] Text is internationalised (see here)
- [ ] Docs (specifically
docs/Config.md) have been updated if necessary - [ ] You've read through your own file changes for silly mistakes etc
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I spent some time today looking into the potential effort of rebasing this onto newer PRs versus rebasing newer PRs onto this. I can only speak for my own branches, and every single one of them conflicts with this in some way; however, those conflicts are mostly trivial and easy to deal with. On the other hand, this PR has already run out of sync with master quite heavily, and rebasing it onto master now is going to be quite some work, and will be more and more work as PRs such as https://github.com/jesseduffield/lazygit/pull/2370 and https://github.com/jesseduffield/lazygit/pull/2547 get merged.
I came to the conclusion that it would be good if this could be finished and merged sooner rather than later.
@stefanhaller for sure: I've gotten sidetracked in the last couple weeks but I'm gonna spend some time this weekend revisiting this PR
@stefanhaller I've rebased this onto master now. I'll clean up some commits and then my plan is as follows:
- cut a release
- merge this PR
- see where I can help out rebasing existing PRs onto the new master
Reason for cutting a release before merging is so that if this PR introduces any intermittent issues that aren't caught by tests, we can fix them without blocking the existing features on master from being released.
Hm, the only thing that slightly bothers me about the plan is that we are cutting a release that has half-baked, unfinished support for stacked branches. We are now showing update-ref commands in a rebase, making it somehow look like we're supporting them, but hitting "e" or moving commits up or down still destroys the stack all the time.
Wondering if there's a chance to get another PR in before the release that fixes these cases: https://github.com/jesseduffield/lazygit/pull/2552. I tried rebasing that PR onto this refactoring; there are a bunch of conflicts, but they are very straight-forward to resolve.