Skip to content

Use TranslationSet.Close for the Close keybind label in ConfirmationController #4808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

kyu08
Copy link
Contributor

@kyu08 kyu08 commented Aug 10, 2025

PR Description

WHAT

I changed the wording of ConfirmationController like below.

As-Is image
This PR's change image

WHY

Now, the keybindings are like below when the confirmation window is shown.

Close/Cancel: <esc> | Cancel: <esc>

I felt that Close/Cancel and Cancel are duplicated.

And it looks like Cancel is added to all controllers. So I changed the wording like MenuController is doing, instead of deleting Close in ConfirmationController.

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)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@@ -161,7 +161,6 @@ type TranslationSet struct {
NoBranchesThisRepo string
CommitWithoutMessageErr string
Close string
CloseCancel string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted this line because this field is no longer referenced.

@kyu08 kyu08 marked this pull request as ready for review August 10, 2025 09:22
@stefanhaller
Copy link
Collaborator

I'm not sure this is the best fix; ideally, we shouldn't see Esc twice in the bottom line. The same happens for menus, you see Close: <esc> | Cancel: <esc>, this doesn't make sense to me.

We should find a way to make sure every keybinding only appears once; I'm not sure yet what the best way is, I'll have to spend some time with the code to make up my mind about that. I also see that Cancel: <esc> appears at the end for every view, even in cases where Esc doesn't do anything; this could be improved too.

Since this is only a minor cosmetic problem, it might take me a while to get around to it.

@kyu08
Copy link
Contributor Author

kyu08 commented Aug 11, 2025

We should find a way to make sure every keybinding only appears once; I'm not sure yet what the best way is, I'll have to spend some time with the code to make up my mind about that.

This problem might be fixed by an implementation like this (NOTE: This is a kind of PoC. When this code is needed, I will refine it).

diff --git i/pkg/gui/options_map.go w/pkg/gui/options_map.go
index bacc3b106..b284a9c89 100644
--- i/pkg/gui/options_map.go
+++ w/pkg/gui/options_map.go
@@ -39,6 +39,20 @@ func (self *OptionsMapMgr) renderContextOptionsMap() {
 	globalBindings := self.c.Contexts().Global.GetKeybindings(self.c.KeybindingsOpts())
 
 	allBindings := append(currentContextBindings, globalBindings...)
+	result := make([]*types.Binding, 0, len(allBindings))
+
+OUTER:
+	for _, binding := range allBindings {
+		for i, existingBinding := range result {
+			if existingBinding.Key == binding.Key {
+				// If the binding already exists, we just update its description
+				result[i].Description = fmt.Sprintf("%s/%s", existingBinding.Description, binding.Description)
+				continue OUTER
+			}
+		}
+		result = append(result, binding)
+	}
+	allBindings = result
 
 	bindingsToDisplay := lo.Filter(allBindings, func(binding *types.Binding, _ int) bool {
 		return binding.DisplayOnScreen && !binding.IsDisabled()

But if we could delete all the unnecessary keybindings like Cancel: <esc>, this implementation might become unnecessary.

So maybe it is more important to solve the problem below first.

I also see that Cancel: appears at the end for every view, even in cases where Esc doesn't do anything; this could be improved too.

What do you think?

@stefanhaller
Copy link
Collaborator

@kyu08 I opened #4819 which should solve both problems.

@kyu08
Copy link
Contributor Author

kyu08 commented Aug 12, 2025

@stefanhaller
Thanks for the work!
It looks good to me. So I close this PR.

@kyu08 kyu08 closed this Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants