Reconsider storing UI toggles in state.yml

[stefanhaller]

In #2904 we had a long debate about where/how to persist settings that users can change in the UI. After some discussion and a twitter poll we decided to persist them in state.yml (option C from the above link). I'm challenging this decision now, after gaining some more real-world experience with it.

First, an assessment of the status quo in lazygit:

ignore whitespace	stored in state.yml, no config
diff context size	stored in state.yml, no config
rename threshold	stored in state.yml, no config
local branch sort order	stored in state.yml, no config
remote branch sort order	stored in state.yml, no config
show file tree	stored in user config, not reset when config changes
show command log	both user config and state.yml, interacting in interesting ways (see below)
git log order	stored in state.yml, deprecated config exists for legacy reasons
show graph	stored in state.yml, deprecated config exists for legacy reasons

Of these, the "show command log" setting is the most interesting one. It has a user config that defaults to true, and if it is true, the state is persisted in state.yml (in other words, hiding the log in the UI will be remembered, and it will stay hidden at the next start). If the config is false, this overrides the state.yml value, and the log will be hidden at every start. (I happen to like this latter behavior.)

The "git log order" and "show graph" settings are special in that they do have a user config, however it is only used for initializing the AppState values if no state.yml exists, i.e. only at the very first start of lazygit.

Discussion

There are arguments for and against our current default policy of storing settings in state.yml. The alternative is to have a user config for the initial value at startup, and then let users change this at runtime, but don't persist the change (option A from the discussion linked to above. Side note: after re-reading that discussion I still don't understand what option B is supposed to mean, so I'm ignoring it here).

Advantage of C (storing in state.yml):

1. If a user changes, say, the diff context size and realizes that they do like the new value better in general, there's nothing else they need to do.

Advantages of A (user config):

2. If they only changed the diff context size temporarily but usually prefer their default of 3 or 5, then they always get back to the preferred state at the next start.
3. Some users accidentally changed a value like diff context size, and then got confused or ran into problems from that; for these users, the problems would have been fixed on the next restart by not persisting the value that they didn't even realize they changed.
4. User configs can be repo-local. While this is probably not relevant for most of the settings, for some it is very useful. For example, you could keep git log order at the default of "topo-order" for most repos, and only set it to "default" for those few giant repos where it is needed for better performance. Similarly, some repos have such a messed up commit history that seeing the graph is more confusing than helpful, so you can turn the graph off for those, but keep it on for all others.

A middle-ground option (D)

There is another possible approach that is somewhere in the middle between A and C: allow users to set a user config to get a defined value at every start, but if they omit it, persist the value in state.yml. This is similar to what we do for "show command log" today, except there we do it only when the config is false. Here the suggestion is to do this for any config value if it exists. To implement this, we'd define these configs as pointers so that we can distinguish false from unset.

While this approach seems like the most flexible one, I'm not suggesting it because it doesn't have as many advantages as A. It supports 1. and 4. from the list above, but not 2. and 3.

Conclusion

I'm proposing to go back to option A. We've also seen people in various issues say that this is what they intuitively expect (e.g. here and here).

[wolfgangwalther]

+1 for A. It's the most intuitive. The only thing I can imagine that happens is, that you're going to change a value at run-time, then restart lazygit and be annoyed that it's back to the old value. But that is easily understood and fixed: Just put it into the config file.

If this goes into direction of D, then I think it's important to clarify what happens when you start without a setting in config, then persist it by changing it in the UI and then change the config. This will result in having it both in config and state files. The only reasonable thing, imho, would be to say that config always wins, so state would only take effect when config is unset.

Doing it this way, D essentially becomes an extension of A:

• Instead of not persisting UI changes, save them to state.
• If config is set, use that value.

I'm coming from NixOS, which means I really favor declarative config files over changing things in the UI. So for me, A alone would be perfect. But I can see how others might want to be able to persist changes in the UI and I think D would be a fair compromise for both. I would force all settings via config file and get the "UI changes are never persisted" behavior - and others would keep the config file to a minimum and get "I can change stuff via UI" behavior.

[stefanhaller]

The only thing I can imagine that happens is, that you're going to change a value at run-time, then restart lazygit and be annoyed that it's back to the old value. But that is easily understood and fixed: Just put it into the config file.

It's only easily understood if they know that the config exists, which might not be obvious if they only found the UI toggle. We should probably add tooltips to all those UI elements telling them how they can change the default.

If this goes into direction of D, then I think it's important to clarify what happens when you start without a setting in config, then persist it by changing it in the UI and then change the config. This will result in having it both in config and state files. The only reasonable thing, imho, would be to say that config always wins, so state would only take effect when config is unset.

Yes, this was the idea, and I feel this would be quite intuitive.

[stefanhaller]

I just remembered one more argument against C (and D), but maybe it's only relevant for developers: A is better for bisecting. It has happened to me multiple times that I bisected to a version earlier than when we started to store the branches sort order in state.yml, and this removed it from my state.yml, falling back to recency. Quite annoying, and can't happen with option A.

[stefanhaller]

@wolfgangwalther I just realized that I swapped A and C in the discussion above (compared to this discussion), which made things very confusing if you read both. I edited all comments here to fix that (including yours), just so you know.

[stefanhaller]

There's one detail that I hadn't really considered yet, which is when to reload the value from the config. There's two aspects to this (behavior and implementation).

Behavior

Whenever reloading the config, the value will switch back to what's stored in the config. This is needed for switching repos (including going in and out of submodules), in which case the value might be in a repo-local config file and thus needs to be updated. It is also useful for users who haven't found the UI toggle, and simply change the value in their config file while lazygit is running, and have the change take effect immediately.

However, this has the consequence that the value will also revert back to the config when changing something unrelated in the config file. This is probably undesirable, but I'm afraid we'll have to live with it, and maybe it's not such a big problem.

Implementation

Where do we store the value that is changed by the UI toggle at runtime? If we store it in a separate property (e.g. in a view model) then we have to write extra code in Gui.onUserConfigLoaded to update that copied value from the config (like we currently do for the ShowExtrasWindow property). This is easy to forget. Also, having to add that extra property is annoying.

To make this easier, we could consider changing it directly in UserConfig, on the assumption that we will never write it back to disk (which we currently don't). If we ever have a reason in the future to write the config back, we'll persist the current ad-hoc values, which is not desired. I find it unlikely that we will ever do this, but who knows.

Any thoughts welcome.

[stefanhaller]

@wolfgangwalther Here's the PR: #4717. I'd appreciate testing and/or reviews!

[wolfgangwalther]

Whenever reloading the config, the value will switch back to what's stored in the config.

How hard would it be to only change the value at run-time, if the config explicitly sets it?

Aka, if the config file doesn't have setting A, I changed setting A at run-time, then change the config in an unrelated way - would setting A still toggle back?

Although.. maybe it's actually a feature to "reload config/defaults asap", even on unrelated changes. My thinking goes like this:

• You change a config option at run-time, most likely to test it.
• You make some unrelated config changes.
• Due to reload you will realize quickly that the option you just changed at run-time has reverted back and needs to be persisted.

In a way the "If you like it, put it in the config file" feedback cycle is shorter that way. Compare that to a scenario where you work on other stuff in your config, the setting you never touched stays at the run-time value... and a few days later, when you finally restart lazygit (my instances are running for weeks, sometimes) "something" changed. But you can't exactly tell what and why.

To make this easier, we could consider changing it directly in UserConfig, on the assumption that we will never write it back to disk (which we currently don't). If we ever have a reason in the future to write the config back, we'll persist the current ad-hoc values, which is not desired. I find it unlikely that we will ever do this, but who knows.

This seems reasonable from the perspective of a NixOS user, who wants to store configuration declaratively in .git controlled config files anyway. I don't want lazygit to ever change my configuration file.

But also for anyone managing their config imperatively, the only reason I can imagine to actually want to write back to the config file... would be exactly this: Saving that runtime state in the config file! Aka allowing it to "modify the config via UI".

TLDR: I think both considerations are fine. Always reload and keep the implementation simple.