Add StatusPanelView config

[oakio]

• PR Description
  Adds a new config statusPanelView which allows you to select the default view of the main window: dashboard (default), allBranchesLog.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run 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

[karimkhaleel]

Looked over the code and tested the feature.

Was a little confused as to what this feature changes from the PR title and description. Had to look over the code to see that it's about the status panel's main view.

Other than perhaps thinking about a different name for the config variable, it looks fine to me.

[karimkhaleel]

I am not sure the name defaultMainView is descriptive enough here. Maybe we can consider changing it to something like defaultStatusPanelView?

[stefanhaller]

I like defaultStatusPanelView, I agree it's better than defaultMainView.

Another concern is that "default" makes it sound as if this is only the initial view, but you can then switch to the respective other one. But this is only true for dashboard; once you see the "all branches log" view, there's no going back. If we think that's acceptable, then I'd omit the "default" from the name and just call it statusPanelView.

But we could also consider changing the "Show all branch logs" command to a toggle that changes to "Hide all branch logs" if it is showing. I'm not sure anybody needs that functionality though.

[karimkhaleel]

Makes sense. I also doubt anyone would want to be switching between those two views. statusPanelView sounds good to me.

[oakio]

Renamed DefaultMainView to StatusPanelView.

[stefanhaller]

@oakio Thanks; the schema seems to be out of date now, though, please regenerate it.

[oakio]

Fixed

[karimkhaleel]

I am also not sure if we should be quietly choosing the dashboard view in case of an error in the config file.

[oakio]

Something like this?

default:
	return func() error {
		return self.c.Error(fmt.Error("Unexpected value for StatusPanelView config"));
	}

[stefanhaller]

True, but I'm not sure what else to do. In other cases we panic at the place where it's used (e.g. for git.log.showGraph, when it was still a config), and that's a terrible UX, we shouldn't do that.

We could validate the config value right after loading, but then there's the risk that the validation code runs out of sync with the code that uses it, since we're working with string literals everywhere.

[stefanhaller]

Oops, our messages crossed. Yes, that would also work, but I think I'd prefer to do the validation right after loading (i.e. at the end of loadUserConfig.

[karimkhaleel]

We could validate the config value right after loading, but then there's the risk that the validation code runs out of sync with the code that uses it, since we're working with string literals everywhere.

😬 you're right. Maybe we do just fallback to default then. We have the schema to at least help the user out here anyway.

[oakio]

Added UserConfig validation. In particular validation for Git.StatusPanelView.

[stefanhaller]

Very nice, thanks for the great work. Two more very small things below, apart from that I'm very happy with this now.

I'm not sure if we need to run this by @jesseduffield before merging it. I'll wait a day or two to see if he wants to chime in on anything here.

[stefanhaller]

Nitpick: the name string should be "config.gui.statusPanelView", since that's what the user sees.

[stefanhaller]

Ah, actually we should omit the config. too, I guess.

[oakio]

Fixed.

[stefanhaller]

Did you leave this in intentionally, or is this a left-over? I think we can revert this back to return self.showDashboard.

[oakio]

Revert back to return self.showDashboard.

[karimkhaleel]

Looks good. Just two minor changes with the error strings.

[karimkhaleel]

Suggested change

	return nil, fmt.Errorf("The config at `%s` has validation error.\n%w", path, err)
	return nil, fmt.Errorf("The config at `%s` has a validation error.\n%w", path, err)

[oakio]

Fixed

[karimkhaleel]

Suggested change

	return fmt.Errorf("unexpectd value '%s' for '%s'. Allowed values: %s", value, name, allowedValuesStr)
	return fmt.Errorf("Unexpected value '%s' for '%s'. Allowed values: %s", value, name, allowedValuesStr)

[oakio]

Fixed

[mark2185]

Should we update the copyright to display a range of years?

[jesseduffield]

We can just use time.Now().Year()

[oakio]

Changed to:
fmt.Sprintf("Copyright %d Jesse Duffield", time.Now().Year())

[karimkhaleel]

Suggested change

	return fmt.Errorf("Unexpectd value '%s' for '%s'. Allowed values: %s", value, name, allowedValuesStr)
	return fmt.Errorf("Unexpected value '%s' for '%s'. Allowed values: %s", value, name, allowedValuesStr)

There's still an e missing.

[oakio]

Sorry, fixed

[karimkhaleel]

Everything looks good. I think all that's left is just squashing and rebasing.

[oakio]

@karimkhaleel
Rebased on master.
Is a necessary to squash all commits in one? I specifically tried to logically break down the changes into commits.

[oakio]

@karimkhaleel rebased on master

[karimkhaleel]

@stefanhaller, I looked over this again and tested it. I think everything looks good here.

[stefanhaller]

I agree the code looks goods, thanks for the great work @oakio.