Show current value in menus

[stefanhaller]

• PR Description

Some of our menus let you pick a value for some option (e.g. the sort order menus for branches and commits). It's nice to see which one is the current value when opening such a menu, so this PR implements that.

For menus that also have key bindings, the radio button goes between the key and the label.

As an alternative, I considered selecting the current value when the menu is opened; however, we currently have no menus where we select a different entry than the first one, and it might be confusing for people who are used to opening a menu and then pressing down-arrow a certain number of times to get to a particular value.

• 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 have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 205357a1	✅ 74.19%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (205357a)	Report Missing	Report Missing	Report Missing
Head commit (33c495e)	51523	43455	84.34%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3628)	31	23	74.19%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[AzraelSec]

Hello @stefanhaller - the code looks good, but it doesn't make sense for me to approve it since it's probably better to wait for Jesse to see and approve this change.

While I was testing this code, it came to my mind that maybe (in the future) it would be great to have a specialized component to handle these radio-button-like settings. Showing a marker between the keybindings and the description is clear enough, but what do you think about rendering it like this?

[ ] a description 1
[x] b description 2
[ ] c description 3

Also, using a specialized component would allow us to move the selection logic to the component itself instead of letting the developer handle it. Do you think this would help keep the codebase cleaner?

[stefanhaller]

@AzraelSec Thanks for the input.

That's two different questions: how we render it, and what's the API. They are independent; I could keep the API the same but change the rendering as you suggest, or we could introduce a specialized component but still render it like I do here.

As for the rendering: I changed it to your suggestion and pushed it as a different branch (show-current-option-in-menus-2); however, I chose [✓] instead of [x] as I think that looks better, and I also kept it after the keybinding as I like the keybinding to always be the leftmost thing.

I'm a bit undecided which version I prefer, and would be happy to accept a majority vote. I think I have a slight tendency towards the bare ✓ since I find it cleaner. (Also, the code is simpler.)

As for the API: I'm unsure about that one as well, but I feel the current solution is adequate enough. I don't see a big risk of clients accidentally checking two entries, for example. It's just not the biggest problem in the code base, and I'd rather spend my time on more important things.

[AzraelSec]

Thanks for considering my feedback, @stefanhaller. I agree with treating the rendering and the API as separate and independent themes.

I checked out your branch where you implemented the alternative UI, and I personally prefer it over the original one. In my opinion

• it makes more evident to the users the distinction between a canonical menu and a radio button one
• its complexity in code is comparable to the original

That said, I want to avoid blocking this PR, so feel free to proceed with your first implementation if you think it's the best option.

Regarding the API, creating a specific component could still be an improvement, but it's definitely not a priority. I'd still be happy to experiment a bit with this theme after the current PR gets merged into master, if it's okay with you.

[stefanhaller]

I checked out your branch where you implemented the alternative UI, and I personally prefer it over the original one.

OK. Thinking about it more though, I guess we should consider using (✓) instead of [✓]. The square brackets look more like a checkbox, using round parens may make it look more like radio buttons. The thinking here is that we might introduce checkboxes as well in the future, e.g. for something like a preferences dialog. (There are still questions around that, e.g. how to toggle a checkbox without closing the dialog, but that can be solved later; for now it's only about the visual appearance.)

@jesseduffield Are you following this? Any opinion?

That said, I want to avoid blocking this PR,

No, the input is valuable and we should try to get this right instead of just doing something.

I'd still be happy to experiment a bit with this theme after the current PR gets merged into master, if it's okay with you.

Well sure, as long as I don't have to spend time on that... 😄

[jesseduffield]

I agree: the bare tick is more aesthetic but using parentheses is superior because it lets us disambiguate radios from checkboxes.

[stefanhaller]

OK. I force-pushed the branch to change the implementation: MenuItem now has a Widget field that lets you draw either checkboxes or radio buttons. See commit message of first commit.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for a5620eb1	✅ 64.58%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (a5620eb)	Report Missing	Report Missing	Report Missing
Head commit (4967e51)	52734	45680	86.62%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3628)	48	31	64.58%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

---

_{🚀 Don’t miss a bit, follow what’s new on Codacy.}

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[AzraelSec]

Looks great! 💪🏻

[jesseduffield]

LGTM, great work