fix: Disable jumping to window when confirmation is active

[ChrisMcD1]

• PR Description

This PR feels fundamentally inelegant, and even has some known bugs. Since the global context is still fundamentally active, the following keys will still be able to produce #4052:

m, <c->, W from the GlobalController
[, ], @, <c-r> from GetInitialKeybindings

We could go ahead and add the same sort of exception to each of them, but all future bindings of global keys that change the active state would recreate the bug again. The odds of users finding these keybindings is relatively low though, since we tell them about it nowhere, so maybe it is fine to leave that bug in?

I was getting deep into the weeds of the event loop in gocui before I decided to call the stopping point and throw this PR up here as a draft. It seemed quite challenging to somehow say "The global context is no longer active until this context closes". And even that wouldn't be 100% desirable because they would lose the ability to close with q, which I think should definitely remain enabled!

Fixes #4052

• 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

[ChrisMcD1]

A question I had about the current implementation: Is there a way to express the idea that This is a ConfirmationContext without specifying if it is a pointer? I don't particularly care if it is or not, but Go seems to deeply care and will not recognize it if it is the wrong kind.

Also I have no idea why that windows test failed.....

[stefanhaller]

Interesting findings. A few thoughts:

• I wouldn't be opposed to disabling all global commands generically if we can find a good way to do that (I haven't looked into how). The fact that this would also disable q doesn't bother me at all. I actually do find this convenient during development (shortens my edit-compile-test cycles by one keystroke 😄), but as a normal user I have never needed to quit lazygit while a popup is showing. And most GUI applications disable all global menu commands (including Command-Q) while a modal dialog is showing, so I think that's fine.
• If we don't find a good way to do this generically, then yes, I'd advocate for putting the check manually into all those individual handlers. Good that you found them!
• But actually, we don't have to put the check inside the handlers; there is already an existing mechanism for blocking handlers while a popup is showing, with the Guards.NoPopupPanel construct. See e.g. here.
• I still greatly prefer blocking those commands over auto-closing the popup; I think this could be confusing. Curious what @jesseduffield thinks about this.

[stefanhaller]

One potential solution for blocking all global keybindings generically could be something like this:

diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go
index 51a1bf2c7..38a03ca45 100644
--- a/pkg/gui/gui.go
+++ b/pkg/gui/gui.go
@@ -819,6 +819,7 @@ func (gui *Gui) Run(startArgs appTypes.StartArgs) error {
 	defer gui.g.Close()
 
 	g.ErrorHandler = gui.PopupHandler.ErrorHandler
+	g.IsGlobalKeybindingBlockedCallback = func() bool { return gui.helpers.Confirmation.IsPopupPanelFocused() }
 
 	// if the deadlock package wants to report a deadlock, we first need to
 	// close the gui so that we can actually read what it prints.
diff --git a/vendor/github.com/jesseduffield/gocui/gui.go b/vendor/github.com/jesseduffield/gocui/gui.go
index 03d55912a..697a1b50a 100644
--- a/vendor/github.com/jesseduffield/gocui/gui.go
+++ b/vendor/github.com/jesseduffield/gocui/gui.go
@@ -177,6 +177,8 @@ type Gui struct {
 
 	ErrorHandler func(error) error
 
+	IsGlobalKeybindingBlockedCallback func() bool
+
 	screen         tcell.Screen
 	suspendedMutex sync.Mutex
 	suspended      bool
@@ -1539,6 +1541,9 @@ func (g *Gui) execKeybindings(v *View, ev *GocuiEvent) error {
 	}
 
 	if globalKb != nil {
+		if g.IsGlobalKeybindingBlockedCallback != nil && g.IsGlobalKeybindingBlockedCallback() {
+			return nil
+		}
 		return g.execKeybinding(v, globalKb)
 	}
 	return nil

It's rather ugly, but seems to do the job. (But also disables q in popups.)

If we don't want this, I'd propose to not spend any more time and energy on a generic solution, and simply add the Guards.NoPopupPanel to all those individual handlers and be done with it. I don't think it's important enough to spend more energy on it.

[jesseduffield]

I much prefer blocking the commands over auto-closing the popup, however I also like having 'q' quit the application regardless of whether a popup is shown. What's more, we also have ctrl+c in that global context, and I feel that as a cultural norm, in a terminal application, ctrl+c should quit an application no matter what context you're in (albeit vim is an exception, but it's also renowned for being unintuitive to quit!)

Maybe we could find some other way to treat the quit keybindings specially, like putting them in a separate controller (e.g. QuitController), and blocking the GlobalController rather than the global context.

[stefanhaller]

How bad do you think it would be to simply block all those individual handlers with Guards.NoPopupPanel though? This strikes me as the simplest solution for now, it's not that many.

[jesseduffield]

Yeah, I'm fine with that. We rarely add new global keybindings

[ChrisMcD1]

Pushed up new version using the guard, and added a line at the bottom of the english intro message that tells them what key to hit. I know the confirm key is visible in the bottom bar, but I find in the default color scheme that doesn't stick out as an obvious next step. And clearly some non-zero number of people not seeing it.

[ChrisMcD1]

Removed the double-popup test since that is now not allowed behavior. I could rewrite the test to assert that it isn't allowed? Didn't feel necessary to me, but happy to do it if others want.

[stefanhaller]

Looks awesome, I'm happy to merge in this state. The added text to the startup popup looks fine to me, and removing the test is ok with me too.

It would be good if you could bring the PR description up to date, since it is going to be used for the merge commit.