Fix some obscure selection highlighting bugs (#5990)

pkg/gui/context.go

@@ -3,6 +3,7 @@ package gui
 import (
 	"sync"
 
+	"github.com/jesseduffield/generics/set"
 	"github.com/jesseduffield/lazygit/pkg/gui/context"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
 	"github.com/jesseduffield/lazygit/pkg/utils"
@@ -180,10 +181,6 @@ func (self *ContextMgr) Activate(c types.Context, opts types.OnFocusOpts) {
 
 	self.gui.helpers.Window.MoveToTopOfWindow(c)
 	inputViewName := c.GetInputViewName()
-	oldView := self.gui.c.GocuiGui().CurrentView()
-	if oldView != nil && oldView.Name() != inputViewName {
-		oldView.HighlightInactive = true
-	}
 	if _, err := self.gui.c.GocuiGui().SetCurrentView(inputViewName); err != nil {
 		panic(err)
 	}
@@ -199,9 +196,37 @@ func (self *ContextMgr) Activate(c types.Context, opts types.OnFocusOpts) {
 
 	self.gui.c.GocuiGui().Cursor = v.Editable && v.Mask == ""
 
+	self.updateSelectionHighlights()
+
 	c.HandleFocus(opts)
 }
 
+// updateSelectionHighlights re-derives which views draw a selection, and which of
+// them draw theirs as the active one: a view shows a selection while its context is
+// on the stack and has something to select, and the context the user is in shows the
+// active selection while the ones behind it show inactive ones.
+//
+// Both of those can change, so this is called wherever they do: from Activate, which
+// every change to the stack goes through; after a refresh, since that is when the
+// contents of a list change; and from whoever tells a context that its content has
+// gained or lost something to select.
+func (self *ContextMgr) updateSelectionHighlights() {
+	self.RLock()
+	defer self.RUnlock()
+
+	onStack := set.NewFromSlice(lo.Map(self.ContextStack,
+		func(c types.Context, _ int) types.ContextKey { return c.GetKey() }))
+	currentKey := self.currentContextWithoutLock().GetKey()
+
+	for _, c := range self.allContexts.Flatten() {
+		// The global context has no view of its own.
+		if view := c.GetView(); view != nil {
+			view.Highlight = onStack.Includes(c.GetKey()) && c.HasSelectableContent()
+			view.HighlightInactive = c.GetKey() != currentKey
+		}
+	}
+}
+
 func (self *ContextMgr) Current() types.Context {
 	self.RLock()
 	defer self.RUnlock()

pkg/gui/context/base_context.go

@@ -28,7 +28,7 @@ type BaseContext struct {
 	hasControlledBounds         bool
 	needsRerenderOnWidthChange  types.NeedsRerenderOnWidthChangeLevel
 	needsRerenderOnHeightChange bool
-	highlightOnFocus            bool
+	hasSelectableContent        bool
 
 	*ParentContextMgr
 }
@@ -49,7 +49,7 @@ type NewBaseContextOpts struct {
 	Focusable                   bool
 	Transient                   bool
 	HasUncontrolledBounds       bool // negating for the sake of making false the default
-	HighlightOnFocus            bool
+	HasSelectableContent        bool
 	NeedsRerenderOnWidthChange  types.NeedsRerenderOnWidthChangeLevel
 	NeedsRerenderOnHeightChange bool
 
@@ -70,7 +70,7 @@ func NewBaseContext(opts NewBaseContextOpts) *BaseContext {
 		focusable:                   opts.Focusable,
 		transient:                   opts.Transient,
 		hasControlledBounds:         hasControlledBounds,
-		highlightOnFocus:            opts.HighlightOnFocus,
+		hasSelectableContent:        opts.HasSelectableContent,
 		needsRerenderOnWidthChange:  opts.NeedsRerenderOnWidthChange,
 		needsRerenderOnHeightChange: opts.NeedsRerenderOnHeightChange,
 		ParentContextMgr:            &ParentContextMgr{},
@@ -118,6 +118,10 @@ func (self *BaseContext) GetKind() types.ContextKind {
 	return self.kind
 }
 
+func (self *BaseContext) HasSelectableContent() bool {
+	return self.hasSelectableContent
+}
+
 func (self *BaseContext) GetKey() types.ContextKey {
 	return self.key
 }

pkg/gui/context/list_context_trait.go

@@ -37,6 +37,10 @@ type ListContextTrait struct {
 
 func (self *ListContextTrait) IsListContext() {}
 
+func (self *ListContextTrait) HasSelectableContent() bool {
+	return self.list.Len() > 0
+}
+
 func (self *ListContextTrait) FocusLine(scrollIntoView bool) {
 	self.Context.FocusLine(scrollIntoView)
 
@@ -102,8 +106,6 @@ func formatListFooter(selectedLineIdx int, length int) string {
 func (self *ListContextTrait) HandleFocus(opts types.OnFocusOpts) {
 	self.FocusLine(!opts.KeepScrollPosition)
 
-	self.GetViewTrait().SetHighlight(self.list.Len() > 0)
-
 	self.Context.HandleFocus(opts)
 }
 

pkg/gui/context/main_context.go

@@ -21,12 +21,12 @@ func NewMainContext(
 	ctx := &MainContext{
 		SimpleContext: NewSimpleContext(
 			NewBaseContext(NewBaseContextOpts{
-				Kind:             types.MAIN_CONTEXT,
-				View:             view,
-				WindowName:       windowName,
-				Key:              key,
-				Focusable:        true,
-				HighlightOnFocus: false,
+				Kind:                 types.MAIN_CONTEXT,
+				View:                 view,
+				WindowName:           windowName,
+				Key:                  key,
+				Focusable:            true,
+				HasSelectableContent: false,
 			})),
 		SearchTrait: NewSearchTrait(c),
 	}

pkg/gui/context/merge_conflicts_context.go

@@ -35,12 +35,12 @@ func NewMergeConflictsContext(
 		viewModel: viewModel,
 		Context: NewSimpleContext(
 			NewBaseContext(NewBaseContextOpts{
-				Kind:             types.MAIN_CONTEXT,
-				View:             c.Views().MergeConflicts,
-				WindowName:       "main",
-				Key:              MERGE_CONFLICTS_CONTEXT_KEY,
-				Focusable:        true,
-				HighlightOnFocus: true,
+				Kind:                 types.MAIN_CONTEXT,
+				View:                 c.Views().MergeConflicts,
+				WindowName:           "main",
+				Key:                  MERGE_CONFLICTS_CONTEXT_KEY,
+				Focusable:            true,
+				HasSelectableContent: true,
 			}),
 		),
 		c: c,

pkg/gui/context/patch_explorer_context.go

@@ -47,7 +47,7 @@ func NewPatchExplorerContext(
 			Key:                        key,
 			Kind:                       types.MAIN_CONTEXT,
 			Focusable:                  true,
-			HighlightOnFocus:           true,
+			HasSelectableContent:       true,
 			NeedsRerenderOnWidthChange: types.NEEDS_RERENDER_ON_WIDTH_CHANGE_WHEN_WIDTH_CHANGES,
 		})),
 		SearchTrait: NewSearchTrait(c),

pkg/gui/context/simple_context.go

@@ -33,10 +33,6 @@ func NewDisplayContext(key types.ContextKey, view *gocui.View, windowName string
 }
 
 func (self *SimpleContext) HandleFocus(opts types.OnFocusOpts) {
-	if self.highlightOnFocus {
-		self.GetViewTrait().SetHighlight(true)
-	}
-
 	for _, fn := range self.onFocusFns {
 		fn(opts)
 	}
@@ -47,7 +43,6 @@ func (self *SimpleContext) HandleFocus(opts types.OnFocusOpts) {
 }
 
 func (self *SimpleContext) HandleFocusLost(opts types.OnFocusLostOpts) {
-	self.GetViewTrait().SetHighlight(false)
 	self.view.SetOriginX(0)
 	for _, fn := range self.onFocusLostFns {
 		fn(opts)

pkg/gui/context/view_trait.go

@@ -43,11 +43,6 @@ func (self *ViewTrait) SetContent(content string) {
 	self.view.SetContent(content)
 }
 
-func (self *ViewTrait) SetHighlight(highlight bool) {
-	self.view.Highlight = highlight
-	self.view.HighlightInactive = false
-}
-
 func (self *ViewTrait) SetFooter(value string) {
 	self.view.Footer = value
 }

pkg/gui/controllers/suggestions_controller.go

@@ -85,7 +85,6 @@ func (self *SuggestionsController) GetMouseKeybindings(opts types.KeybindingsOpt
 
 func (self *SuggestionsController) switchToPrompt() error {
 	self.c.Views().Suggestions.Subtitle = ""
-	self.c.Views().Suggestions.Highlight = false
 	self.c.Context().Replace(self.c.Contexts().Prompt)
 	return nil
 }

pkg/gui/controllers/toggle_whitespace_action.go

@@ -27,6 +27,6 @@ func (self *ToggleWhitespaceAction) Call() error {
 
 	self.c.UserConfig().Git.IgnoreWhitespaceInDiffView = !self.c.UserConfig().Git.IgnoreWhitespaceInDiffView
 
-	self.c.Context().CurrentSide().HandleFocus(types.OnFocusOpts{})
+	self.c.Context().CurrentSide().HandleRenderToMain()
 	return nil
 }

pkg/gui/gui.go

@@ -598,13 +598,6 @@ func (gui *Gui) resetState(startArgs appTypes.StartArgs) types.Context {
 	// RefreshHelper.onUIThreadUnlessRepoChanged).
 	gui.repoGeneration.Add(1)
 
-	// Un-highlight the current view if there is one. The reason we do this is
-	// that the repo we are switching to might have a different view focused,
-	// and would then show an inactive highlight for the previous view.
-	if oldCurrentView := gui.g.CurrentView(); oldCurrentView != nil {
-		oldCurrentView.Highlight = false
-	}
-
 	worktreePath := gui.git.RepoPaths.WorktreePath()
 
 	if state := gui.RepoStateMap[Repo(worktreePath)]; state != nil {

pkg/gui/types/context.go

@@ -75,6 +75,10 @@ type IBaseContext interface {
 	// determined independently.
 	HasControlledBounds() bool
 
+	// true if the context holds something for a selection to sit on. Contexts that
+	// don't show a selection at all say false, and so do lists with nothing in them.
+	HasSelectableContent() bool
+
 	// the total height of the content that the view is currently showing
 	TotalContentHeight() int
 
@@ -225,7 +229,6 @@ type IViewTrait interface {
 	ScrollDown(value int)
 	PageDelta() int
 	SelectedLineIdx() int
-	SetHighlight(bool)
 }
 
 type OnFocusOpts struct {

pkg/gui/view_helpers.go

@@ -140,6 +140,10 @@ func (gui *Gui) postRefreshUpdate(c types.Context, opts types.OnFocusOpts) {
 
 	c.HandleRender()
 
+	// The render may have given the context its first item, or taken its last one
+	// away, which decides whether its view draws a selection at all.
+	gui.State.ContextMgr.updateSelectionHighlights()
+
 	if gui.currentViewName() == c.GetInputViewName() {
 		c.HandleFocus(opts)
 	} else {

pkg/integration/components/view_driver.go

@@ -360,6 +360,42 @@ func (self *ViewDriver) Content(matcher *TextMatcher) *ViewDriver {
 	return self
 }
 
+// SelectionIsActive asserts that the view draws its selection as the one the user
+// is working in. These three assertions read the highlight flags rather than the
+// selected lines, which say nothing about whether the selection is drawn at all.
+func (self *ViewDriver) SelectionIsActive() *ViewDriver {
+	self.t.assertWithRetries(func() (bool, string) {
+		view := self.getView()
+		ok := view.Highlight && !view.HighlightInactive
+		return ok, fmt.Sprintf("%s: expected an active selection to be shown, but it wasn't", self.context)
+	})
+
+	return self
+}
+
+// SelectionIsInactive asserts that the view draws its selection dimmed, as a panel
+// does while the focus is somewhere else.
+func (self *ViewDriver) SelectionIsInactive() *ViewDriver {
+	self.t.assertWithRetries(func() (bool, string) {
+		view := self.getView()
+		ok := view.Highlight && view.HighlightInactive
+		return ok, fmt.Sprintf("%s: expected an inactive selection to be shown, but it wasn't", self.context)
+	})
+
+	return self
+}
+
+// SelectionIsHidden asserts that the view draws no selection at all, e.g. a list
+// with nothing in it, where there is nothing to select.
+func (self *ViewDriver) SelectionIsHidden() *ViewDriver {
+	self.t.assertWithRetries(func() (bool, string) {
+		ok := !self.getView().Highlight
+		return ok, fmt.Sprintf("%s: expected no selection to be shown, but one was", self.context)
+	})
+
+	return self
+}
+
 // asserts on the selected line of the view. If you are selecting a range,
 // you should use the SelectedLines method instead.
 func (self *ViewDriver) SelectedLine(matcher *TextMatcher) *ViewDriver {

pkg/integration/tests/test_list.go

@@ -526,8 +526,13 @@ var tests = []*components.IntegrationTest{
 	ui.ReloadSidePanels,
 	ui.ReorderSidePanels,
 	ui.SubCommitsScrollPositionIsReset,
+	ui.SuggestionsSelectionFollowsTheFocus,
+	ui.SwitchRepoMovesTheSelection,
 	ui.SwitchTabFromMenu,
 	ui.SwitchTabWithPanelJumpKeys,
+	ui.ToggleWhitespaceKeepsUnfocusedSelectionDimmed,
+	ui.UnfocusedListHidesSelectionWhenEmptied,
+	ui.UnfocusedListShowsSelectionWhenFilled,
 	undo.UndoCheckoutAndDrop,
 	undo.UndoCommit,
 	undo.UndoDrop,

pkg/integration/tests/ui/suggestions_selection_follows_the_focus.go

@@ -0,0 +1,42 @@
+package ui
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SuggestionsSelectionFollowsTheFocus = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "The suggestions list only shows a selection while it, rather than the prompt, has the focus",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.
+			EmptyCommit("one").
+			NewBranch("branch-to-checkout")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Branches().
+			Focus().
+			Press(keys.Branches.CheckoutBranchByName)
+
+		t.ExpectPopup().Prompt().
+			Title(Equals("Branch name:")).
+			Type("branch-to").
+			SuggestionTopLines(Contains("branch-to-checkout"))
+
+		t.Views().Suggestions().SelectionIsHidden()
+
+		t.Views().Prompt().Press(keys.Universal.TogglePanel)
+		t.Views().Suggestions().
+			IsFocused().
+			SelectionIsActive().
+			Press(keys.Universal.TogglePanel)
+
+		t.Views().Prompt().IsFocused()
+		t.Views().Suggestions().SelectionIsHidden()
+
+		t.Views().Prompt().Press(keys.Universal.Return)
+		t.Views().Branches().IsFocused()
+	},
+})

pkg/integration/tests/ui/switch_repo_moves_the_selection.go

@@ -0,0 +1,48 @@
+package ui
+
+import (
+	"path/filepath"
+
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SwitchRepoMovesTheSelection = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "The selection follows the focus of the repo being switched to, rather than the one being left",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig: func(config *config.AppConfig) {
+		otherRepo, _ := filepath.Abs("../other")
+		config.GetAppState().RecentRepos = []string{otherRepo}
+	},
+	SetupRepo: func(shell *Shell) {
+		shell.CloneNonBare("other")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		switchToRepo := func(repo string) {
+			t.GlobalPress(keys.Universal.OpenRecentRepos)
+			t.ExpectPopup().Menu().Title(Equals("Recent repositories")).
+				Lines(
+					Contains(repo).IsSelected(),
+					Contains("Cancel"),
+				).Confirm()
+			t.Views().Status().Content(Contains(repo + " → master"))
+		}
+
+		t.Views().Branches().
+			Focus().
+			SelectionIsActive()
+
+		// The other repo has its own focus, which is the files panel it starts in
+		switchToRepo("other")
+		t.Views().Files().IsFocused()
+		t.Views().Branches().SelectionIsHidden()
+
+		// And coming back, this repo still has the focus we left it with
+		switchToRepo("repo")
+		t.Views().Branches().
+			IsFocused().
+			SelectionIsActive()
+		t.Views().Files().SelectionIsHidden()
+	},
+})

pkg/integration/tests/ui/toggle_whitespace_keeps_unfocused_selection_dimmed.go

@@ -0,0 +1,31 @@
+package ui
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var ToggleWhitespaceKeepsUnfocusedSelectionDimmed = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Toggling whitespace from the main view leaves the panel beneath it showing a dimmed selection",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "one\n")
+		shell.Commit("one")
+		shell.UpdateFile("file1", "  one\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Lines(Contains("file1")).
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().
+			IsFocused().
+			Press(keys.Universal.ToggleWhitespaceInDiffView)
+
+		t.Views().Files().
+			SelectionIsInactive()
+	},
+})

pkg/integration/tests/ui/unfocused_list_hides_selection_when_emptied.go

@@ -0,0 +1,36 @@
+package ui
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var UnfocusedListHidesSelectionWhenEmptied = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "A list that loses its last item while the focus is elsewhere stops showing a selection",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "one\n")
+		shell.Commit("one")
+		shell.UpdateFile("file1", "two\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Lines(Contains("file1")).
+			SelectionIsActive().
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().IsFocused()
+
+		t.Views().Files().
+			SelectionIsInactive().
+			Tap(func() {
+				t.Shell().RunCommand([]string{"git", "checkout", "--", "file1"})
+				t.RefreshInBackground()
+			}).
+			IsEmpty().
+			SelectionIsHidden()
+	},
+})

pkg/integration/tests/ui/unfocused_list_shows_selection_when_filled.go

@@ -0,0 +1,34 @@
+package ui
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var UnfocusedListShowsSelectionWhenFilled = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "A list that gets its first item while the focus is elsewhere starts showing a selection",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "one\n")
+		shell.Commit("one")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			IsEmpty().
+			SelectionIsHidden().
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().IsFocused()
+
+		t.Views().Files().
+			Tap(func() {
+				t.Shell().CreateFile("file2", "two\n")
+				t.RefreshInBackground()
+			}).
+			Lines(Contains("file2")).
+			SelectionIsInactive()
+	},
+})