Merge pull request #2079 from jesseduffield/fix-menu-popup-focus-issue

docs/Integration_Tests.md

@@ -7,7 +7,7 @@ Integration tests are located in `test/integration`. Each test will run a bash s
 An example of a `test.json` is:
 
 ```
-{ "description": "stage a file and commit the change", "speed": 20 }
+{ "description": "Open a confirmation, then open a menu over that, then close the menu. Verify that the confirmation popup also closes automatically", "speed": 20 }
 ```
 
 The `speed` key refers to the playback speed as a multiple of the original recording speed. So 20 means the test will run 20 times faster than the original recording speed. If a test fails for a given speed, it will drop the speed and re-test, until finally attempting the test at the original speed. If you omit the speed, it will default to 10.

pkg/gui/confirmation_panel.go

@@ -16,9 +16,9 @@ import (
 // This file is for the rendering of confirmation panels along with setting and handling associated
 // keybindings.
 
-func (gui *Gui) wrappedConfirmationFunction(handlersManageFocus bool, function func() error) func() error {
+func (gui *Gui) wrappedConfirmationFunction(function func() error) func() error {
 	return func() error {
-		if err := gui.closeConfirmationPrompt(handlersManageFocus); err != nil {
+		if err := gui.c.PopContext(); err != nil {
 			return err
 		}
 
@@ -32,9 +32,9 @@ func (gui *Gui) wrappedConfirmationFunction(handlersManageFocus bool, function f
 	}
 }
 
-func (gui *Gui) wrappedPromptConfirmationFunction(handlersManageFocus bool, function func(string) error, getResponse func() string) func() error {
+func (gui *Gui) wrappedPromptConfirmationFunction(function func(string) error, getResponse func() string) func() error {
 	return func() error {
-		if err := gui.closeConfirmationPrompt(handlersManageFocus); err != nil {
+		if err := gui.c.PopContext(); err != nil {
 			return err
 		}
 
@@ -48,27 +48,15 @@ func (gui *Gui) wrappedPromptConfirmationFunction(handlersManageFocus bool, func
 	}
 }
 
-func (gui *Gui) closeConfirmationPrompt(handlersManageFocus bool) error {
+func (gui *Gui) deactivateConfirmationPrompt() {
 	gui.Mutexes.PopupMutex.Lock()
 	gui.State.CurrentPopupOpts = nil
 	gui.Mutexes.PopupMutex.Unlock()
 
-	// we've already closed it so we can just return
-	if !gui.Views.Confirmation.Visible {
-		return nil
-	}
-
-	if !handlersManageFocus {
-		if err := gui.c.PopContext(); err != nil {
-			return err
-		}
-	}
-
-	gui.clearConfirmationViewKeyBindings()
 	gui.Views.Confirmation.Visible = false
 	gui.Views.Suggestions.Visible = false
 
-	return nil
+	gui.clearConfirmationViewKeyBindings()
 }
 
 func (gui *Gui) getMessageHeight(wrap bool, message string, width int) int {
@@ -219,14 +207,13 @@ func (gui *Gui) setKeyBindings(opts types.CreatePopupPanelOpts) error {
 	_ = gui.renderString(gui.Views.Options, actions)
 	var onConfirm func() error
 	if opts.HandleConfirmPrompt != nil {
-		onConfirm = gui.wrappedPromptConfirmationFunction(opts.HandlersManageFocus, opts.HandleConfirmPrompt, func() string { return gui.Views.Confirmation.TextArea.GetContent() })
+		onConfirm = gui.wrappedPromptConfirmationFunction(opts.HandleConfirmPrompt, func() string { return gui.Views.Confirmation.TextArea.GetContent() })
 	} else {
-		onConfirm = gui.wrappedConfirmationFunction(opts.HandlersManageFocus, opts.HandleConfirm)
+		onConfirm = gui.wrappedConfirmationFunction(opts.HandleConfirm)
 	}
 
 	keybindingConfig := gui.c.UserConfig.Keybinding
 	onSuggestionConfirm := gui.wrappedPromptConfirmationFunction(
-		opts.HandlersManageFocus,
 		opts.HandleConfirmPrompt,
 		gui.getSelectedSuggestionValue,
 	)
@@ -248,7 +235,7 @@ func (gui *Gui) setKeyBindings(opts types.CreatePopupPanelOpts) error {
 			ViewName: "confirmation",
 			Contexts: []string{string(context.CONFIRMATION_CONTEXT_KEY)},
 			Key:      gui.getKey(keybindingConfig.Universal.Return),
-			Handler:  gui.wrappedConfirmationFunction(opts.HandlersManageFocus, opts.HandleClose),
+			Handler:  gui.wrappedConfirmationFunction(opts.HandleClose),
 		},
 		{
 			ViewName: "confirmation",
@@ -277,7 +264,7 @@ func (gui *Gui) setKeyBindings(opts types.CreatePopupPanelOpts) error {
 			ViewName: "suggestions",
 			Contexts: []string{string(context.SUGGESTIONS_CONTEXT_KEY)},
 			Key:      gui.getKey(keybindingConfig.Universal.Return),
-			Handler:  gui.wrappedConfirmationFunction(opts.HandlersManageFocus, opts.HandleClose),
+			Handler:  gui.wrappedConfirmationFunction(opts.HandleClose),
 		},
 		{
 			ViewName: "suggestions",

pkg/gui/context.go

@@ -79,9 +79,10 @@ func (gui *Gui) pushContext(c types.Context, opts ...types.OnFocusOpts) error {
 
 	gui.State.ContextManager.Lock()
 
-	// push onto stack
+	if len(gui.State.ContextManager.ContextStack) == 0 {
-	// if we are switching to a side context, remove all other contexts in the stack
+		gui.State.ContextManager.ContextStack = append(gui.State.ContextManager.ContextStack, c)
-	if c.GetKind() == types.SIDE_CONTEXT {
+	} else if c.GetKind() == types.SIDE_CONTEXT {
+		// if we are switching to a side context, remove all other contexts in the stack
 		for _, stackContext := range gui.State.ContextManager.ContextStack {
 			if stackContext.GetKey() != c.GetKey() {
 				if err := gui.deactivateContext(stackContext); err != nil {
@@ -91,12 +92,26 @@ func (gui *Gui) pushContext(c types.Context, opts ...types.OnFocusOpts) error {
 			}
 		}
 		gui.State.ContextManager.ContextStack = []types.Context{c}
-	} else if len(gui.State.ContextManager.ContextStack) == 0 || gui.currentContextWithoutLock().GetKey() != c.GetKey() {
+	} else {
-		// Do not append if the one at the end is the same context (e.g. opening a menu from a menu)
+		topContext := gui.currentContextWithoutLock()
-		// In that case we'll just close the menu entirely when the user hits escape.
 
-		// TODO: think about other exceptional cases
+		// if we're pushing the same context on, we do nothing.
-		gui.State.ContextManager.ContextStack = append(gui.State.ContextManager.ContextStack, c)
+		if topContext.GetKey() != c.GetKey() {
+			// if top one is a temporary popup, we remove it. Ideally you'd be able to
+			// escape back to previous temporary popups, but because we're currently reusing
+			// views for this, you might not be able to get back to where you previously were.
+			// The exception is when going to the search context e.g. for searching a menu.
+			if topContext.GetKind() == types.TEMPORARY_POPUP && c.GetKey() != context.SEARCH_CONTEXT_KEY {
+				if err := gui.deactivateContext(topContext); err != nil {
+					gui.State.ContextManager.Unlock()
+					return err
+				}
+
+				_, gui.State.ContextManager.ContextStack = slices.Pop(gui.State.ContextManager.ContextStack)
+			}
+
+			gui.State.ContextManager.ContextStack = append(gui.State.ContextManager.ContextStack, c)
+		}
 	}
 
 	gui.State.ContextManager.Unlock()
@@ -111,7 +126,7 @@ func (gui *Gui) pushContextWithView(viewName string) error {
 	return gui.c.PushContext(gui.State.ViewContextMap.Get(viewName))
 }
 
-func (gui *Gui) returnFromContext() error {
+func (gui *Gui) popContext() error {
 	gui.State.ContextManager.Lock()
 
 	if len(gui.State.ContextManager.ContextStack) == 1 {
@@ -120,12 +135,10 @@ func (gui *Gui) returnFromContext() error {
 		return nil
 	}
 
-	n := len(gui.State.ContextManager.ContextStack) - 1
+	var currentContext types.Context
+	currentContext, gui.State.ContextManager.ContextStack = slices.Pop(gui.State.ContextManager.ContextStack)
 
-	currentContext := gui.State.ContextManager.ContextStack[n]
+	newContext := gui.State.ContextManager.ContextStack[len(gui.State.ContextManager.ContextStack)-1]
-	newContext := gui.State.ContextManager.ContextStack[n-1]
-
-	gui.State.ContextManager.ContextStack = gui.State.ContextManager.ContextStack[:n]
 
 	gui.g.SetCurrentContext(string(newContext.GetKey()))
 

pkg/gui/context/menu_context.go

@@ -36,7 +36,7 @@ func NewMenuContext(
 			Context: NewSimpleContext(NewBaseContext(NewBaseContextOpts{
 				ViewName:        "menu",
 				Key:             "menu",
-				Kind:            types.PERSISTENT_POPUP,
+				Kind:            types.TEMPORARY_POPUP,
 				OnGetOptionsMap: getOptionsMap,
 				Focusable:       true,
 			}), ContextCallbackOpts{

pkg/gui/context_config.go

@@ -124,6 +124,10 @@ func (gui *Gui) contextTree() *context.ContextTree {
 			}),
 			context.ContextCallbackOpts{
 				OnFocus: OnFocusWrapper(gui.handleAskFocused),
+				OnFocusLost: func() error {
+					gui.deactivateConfirmationPrompt()
+					return nil
+				},
 			},
 		),
 		CommitMessage: context.NewSimpleContext(

pkg/gui/controllers/helpers/merge_and_rebase_helper.go

@@ -149,17 +149,12 @@ func (self *MergeAndRebaseHelper) CheckMergeOrRebase(result error) error {
 		return nil
 	} else if isMergeConflictErr(result.Error()) {
 		return self.c.Confirm(types.ConfirmOpts{
-			Title:               self.c.Tr.FoundConflictsTitle,
+			Title:  self.c.Tr.FoundConflictsTitle,
-			Prompt:              self.c.Tr.FoundConflicts,
+			Prompt: self.c.Tr.FoundConflicts,
-			HandlersManageFocus: true,
 			HandleConfirm: func() error {
 				return self.c.PushContext(self.contexts.Files)
 			},
 			HandleClose: func() error {
-				if err := self.c.PopContext(); err != nil {
-					return err
-				}
-
 				return self.genericMergeCommand(REBASE_OPTION_ABORT)
 			},
 		})

pkg/gui/gui.go

@@ -424,7 +424,8 @@ func NewGui(
 		cmn,
 		gui.createPopupPanel,
 		func() error { return gui.c.Refresh(types.RefreshOptions{Mode: types.ASYNC}) },
-		func() error { return gui.closeConfirmationPrompt(false) },
+		gui.popContext,
+		gui.currentContext,
 		gui.createMenu,
 		gui.withWaitingStatus,
 		gui.toast,

pkg/gui/gui_common.go

@@ -43,7 +43,7 @@ func (self *guiCommon) PushContext(context types.Context, opts ...types.OnFocusO
 }
 
 func (self *guiCommon) PopContext() error {
-	return self.gui.returnFromContext()
+	return self.gui.popContext()
 }
 
 func (self *guiCommon) CurrentContext() types.Context {

pkg/gui/popup/popup_handler.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/jesseduffield/gocui"
 	"github.com/jesseduffield/lazygit/pkg/common"
+	"github.com/jesseduffield/lazygit/pkg/gui/context"
 	"github.com/jesseduffield/lazygit/pkg/gui/style"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
 	"github.com/jesseduffield/lazygit/pkg/utils"
@@ -17,7 +18,8 @@ type PopupHandler struct {
 	sync.Mutex
 	createPopupPanelFn  func(types.CreatePopupPanelOpts) error
 	onErrorFn           func() error
-	closePopupFn        func() error
+	popContextFn        func() error
+	currentContextFn    func() types.Context
 	createMenuFn        func(types.CreateMenuOptions) error
 	withWaitingStatusFn func(message string, f func() error) error
 	toastFn             func(message string)
@@ -30,7 +32,8 @@ func NewPopupHandler(
 	common *common.Common,
 	createPopupPanelFn func(types.CreatePopupPanelOpts) error,
 	onErrorFn func() error,
-	closePopupFn func() error,
+	popContextFn func() error,
+	currentContextFn func() types.Context,
 	createMenuFn func(types.CreateMenuOptions) error,
 	withWaitingStatusFn func(message string, f func() error) error,
 	toastFn func(message string),
@@ -41,7 +44,8 @@ func NewPopupHandler(
 		index:               0,
 		createPopupPanelFn:  createPopupPanelFn,
 		onErrorFn:           onErrorFn,
-		closePopupFn:        closePopupFn,
+		popContextFn:        popContextFn,
+		currentContextFn:    currentContextFn,
 		createMenuFn:        createMenuFn,
 		withWaitingStatusFn: withWaitingStatusFn,
 		toastFn:             toastFn,
@@ -93,11 +97,10 @@ func (self *PopupHandler) Confirm(opts types.ConfirmOpts) error {
 	self.Unlock()
 
 	return self.createPopupPanelFn(types.CreatePopupPanelOpts{
-		Title:               opts.Title,
+		Title:         opts.Title,
-		Prompt:              opts.Prompt,
+		Prompt:        opts.Prompt,
-		HandleConfirm:       opts.HandleConfirm,
+		HandleConfirm: opts.HandleConfirm,
-		HandleClose:         opts.HandleClose,
+		HandleClose:   opts.HandleClose,
-		HandlersManageFocus: opts.HandlersManageFocus,
 	})
 }
 
@@ -139,8 +142,8 @@ func (self *PopupHandler) WithLoaderPanel(message string, f func() error) error
 		}
 
 		self.Lock()
-		if index == self.index {
+		if index == self.index && self.currentContextFn().GetKey() == context.CONFIRMATION_CONTEXT_KEY {
-			_ = self.closePopupFn()
+			_ = self.popContextFn()
 		}
 		self.Unlock()
 	})

pkg/gui/recent_repos_panel.go

@@ -60,8 +60,12 @@ func (gui *Gui) getCurrentBranch(path string) string {
 }
 
 func (gui *Gui) handleCreateRecentReposMenu() error {
-	// we skip the first one because we're currently in it
+	// we'll show an empty panel if there are no recent repos
-	recentRepoPaths := gui.c.GetAppState().RecentRepos[1:]
+	recentRepoPaths := []string{}
+	if len(gui.c.GetAppState().RecentRepos) > 0 {
+		// we skip the first one because we're currently in it
+		recentRepoPaths = gui.c.GetAppState().RecentRepos[1:]
+	}
 
 	currentBranches := sync.Map{}
 

pkg/gui/types/common.go

@@ -85,9 +85,6 @@ type CreatePopupPanelOpts struct {
 	HandleConfirmPrompt func(string) error
 	HandleClose         func() error
 
-	// when HandlersManageFocus is true, do not return from the confirmation context automatically. It's expected that the handlers will manage focus, whether that means switching to another context, or manually returning the context.
-	HandlersManageFocus bool
-
 	FindSuggestionsFunc func(string) []*Suggestion
 	Mask                bool
 }
@@ -97,7 +94,6 @@ type ConfirmOpts struct {
 	Prompt              string
 	HandleConfirm       func() error
 	HandleClose         func() error
-	HandlersManageFocus bool
 	HasLoader           bool
 	FindSuggestionsFunc func(string) []*Suggestion
 	Editable            bool

pkg/gui/types/context.go

@@ -8,12 +8,22 @@ import (
 type ContextKind int
 
 const (
+	// this is your files, branches, commits, contexts etc. They're all on the left hand side
+	// and you can cycle through them.
 	SIDE_CONTEXT ContextKind = iota
+	// This is either the left or right 'main' contexts that appear to the right of the side contexts
 	MAIN_CONTEXT
-	TEMPORARY_POPUP
+	// A persistent popup is one that has its own identity e.g. the commit message context.
+	// When you open a popup over it, we'll let you return to it upon pressing escape
 	PERSISTENT_POPUP
+	// A temporary popup is one that could be used for various things (e.g. a generic menu or confirmation popup).
+	// Because we re-use these contexts, they're temporary in that you can't return to them after you've switched from them
+	// to some other context, because the context you switched to might actually be the same context but rendering different content.
+	// We should really be able to spawn new contexts for menus/prompts so that we can actually return to old ones.
+	TEMPORARY_POPUP
+	// This contains the command log, underneath the main contexts.
 	EXTRAS_CONTEXT
-	// only used by the one global context
+	// only used by the one global context, purely for the sake of defining keybindings globally
 	GLOBAL_CONTEXT
 )
 

test/integration/popupFocus/expected/repo/.git_keep/COMMIT_EDITMSG

@@ -0,0 +1 @@
+WIP

test/integration/popupFocus/expected/repo/.git_keep/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/master

test/integration/popupFocus/expected/repo/.git_keep/config

@@ -0,0 +1,10 @@
+[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = false
+	logallrefupdates = true
+	ignorecase = true
+	precomposeunicode = true
+[user]
+	email = CI@example.com
+	name = CI

test/integration/popupFocus/expected/repo/.git_keep/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

test/integration/popupFocus/expected/repo/.git_keep/info/exclude

@@ -0,0 +1,7 @@
+# git ls-files --others --exclude-from=.git/info/exclude
+# Lines that start with '#' are comments.
+# For a project mostly in C, the following would be a good set of
+# exclude patterns (uncomment them if you want to use them):
+# *.[oa]
+# *~
+.DS_Store

test/integration/popupFocus/expected/repo/.git_keep/logs/HEAD

@@ -0,0 +1,3 @@
+0000000000000000000000000000000000000000 353cce986c61f361452f43522426c120b4ee9461 CI <CI@example.com> 1659355870 +1000	commit (initial): myfile1
+353cce986c61f361452f43522426c120b4ee9461 6ecdae79ff53548670039abee9008b6bb36cdf4f CI <CI@example.com> 1659355870 +1000	commit: myfile2
+6ecdae79ff53548670039abee9008b6bb36cdf4f 0478d727ea0ebf57ed9ca85acef9e60a324d86f0 CI <CI@example.com> 1659355876 +1000	commit: WIP

test/integration/popupFocus/expected/repo/.git_keep/logs/refs/heads/master

@@ -0,0 +1,3 @@
+0000000000000000000000000000000000000000 353cce986c61f361452f43522426c120b4ee9461 CI <CI@example.com> 1659355870 +1000	commit (initial): myfile1
+353cce986c61f361452f43522426c120b4ee9461 6ecdae79ff53548670039abee9008b6bb36cdf4f CI <CI@example.com> 1659355870 +1000	commit: myfile2
+6ecdae79ff53548670039abee9008b6bb36cdf4f 0478d727ea0ebf57ed9ca85acef9e60a324d86f0 CI <CI@example.com> 1659355876 +1000	commit: WIP

test/integration/popupFocus/expected/repo/.git_keep/objects/04/78d727ea0ebf57ed9ca85acef9e60a324d86f0

@@ -0,0 +1,3 @@
+x
��Λ
+Β0E]η+f/Θ¤ΣΌ@Dθ�;w®“ι
+Ζ–ΑΟ7�ΰκΒαpΈΌΦϊl`Ϊ.C±�8z»θH¤y$ΥΌX��Ω�]_³ε]ή
Όπ’%$UGn�> RΚE$!ΖβK!Ο=¥&Ϊcέa�α<ΝWωζΊ½δΔk½€υ.‘s1x8ZD4�φSMώΤΝ}Ύ™‚—9³

test/integration/popupFocus/expected/repo/.git_keep/objects/35/3cce986c61f361452f43522426c120b4ee9461

@@ -0,0 +1,3 @@
+x
�ÍA
+ƒ0@Ñ®sŠÙÊŒN&Š\yŒ˜L¨`ˆH
+ööõÝ~üXKYË­ª€*1c�Å
ê³’—ä©”/Ü'á¢íLø´w=`šá9Í/=CÙ7}ÄZF ±Co­wwBDsÕkÒôOnÊ7¯›’ù
4],Ù

test/integration/popupFocus/expected/repo/.git_keep/objects/6e/cdae79ff53548670039abee9008b6bb36cdf4f

@@ -0,0 +1,2 @@
+x
�ÎM
+Â0@a×9Eö‚d~2퀈ÐU�‘¦,[J½½=‚ÛÇ·xy­ui”Om7ó©#†µ„y6+*j¢e’EfÄ)d·¥Ý^ÍS¤œM{É…8baŠˆŒ’
ÃÄfÊ.½ÛcÝý0úë0Þí“êö´K^ë̓D¥û.ø3„ÜQ�©frW¿eyº\+9>

test/integration/popupFocus/expected/repo/.git_keep/refs/heads/master

@@ -0,0 +1 @@
+0478d727ea0ebf57ed9ca85acef9e60a324d86f0

test/integration/popupFocus/expected/repo/myfile1

@@ -0,0 +1 @@
+test1

test/integration/popupFocus/expected/repo/myfile2

@@ -0,0 +1 @@
+test2

test/integration/popupFocus/expected/repo/myfile3

@@ -0,0 +1 @@
+test3

test/integration/popupFocus/recording.json

@@ -0,0 +1 @@
+{"KeyEvents":[{"Timestamp":607,"Mod":0,"Key":259,"Ch":0},{"Timestamp":745,"Mod":0,"Key":259,"Ch":0},{"Timestamp":1304,"Mod":0,"Key":256,"Ch":82},{"Timestamp":2087,"Mod":2,"Key":18,"Ch":18},{"Timestamp":2894,"Mod":0,"Key":27,"Ch":0},{"Timestamp":3553,"Mod":0,"Key":260,"Ch":0},{"Timestamp":3697,"Mod":0,"Key":260,"Ch":0},{"Timestamp":4064,"Mod":0,"Key":256,"Ch":32},{"Timestamp":4376,"Mod":0,"Key":256,"Ch":119},{"Timestamp":4745,"Mod":0,"Key":13,"Ch":13},{"Timestamp":5200,"Mod":0,"Key":256,"Ch":113}],"ResizeEvents":[{"Timestamp":0,"Width":238,"Height":61}]}

test/integration/popupFocus/setup.sh

@@ -0,0 +1,18 @@
+#!/bin/sh
+
+set -e
+
+cd $1
+
+git init
+
+git config user.email "CI@example.com"
+git config user.name "CI"
+
+echo test1 > myfile1
+git add .
+git commit -am "myfile1"
+echo test2 > myfile2
+git add .
+git commit -am "myfile2"
+echo test3 > myfile3

test/integration/popupFocus/test.json

@@ -0,0 +1 @@
+{ "description": "stage a file and commit the change", "speed": 15 }