Section headers in keybindings menu

[stefanhaller]

• PR Description

This PR lays the groundwork for displaying non-model things (e.g. section headers or divider lines) in list views in general, then adds a Section field to MenuItem so that menu items can easily be grouped into sections, and uses this mechanism in the keybindings menu to group bindings into local and global ones.

Draft because the visual design is still ugly.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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 (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

When clicking on a section header, the next model item is selected instead. I think that's fine. You might prefer that nothing happens, but I think it doesn't matter much.

[jesseduffield]

Only question is: what happens when there's no items under a given heading (which I expect to happen sometimes e.g. with the symmetric diff thing)? If it's not easy to make that a no-op I'd just make clicking the heading itself a no-op

[stefanhaller]

If you really have two consecutive section headers and you click on the first one, it will instead select the first model item under the second header. I'd still say that's acceptable; I don't expect anybody to click on section headers, except by accident.

For menus this can never happen because section headers are derived from menu items, so there can't be a section without items.

For other lists, it's up to the client code to decide whether they want to include empty sections or not. For the upstream divergence view, I'm still undecided whether I always want to include both section headers even if one is empty; I'm also considering maybe not having headers at all, but only a divider line that is only visible when both sections are non-empty.

[jesseduffield]

I think for symmetric diff headings would be great: it removes any ambiguity about what the items represent. And given that your cursor would start on the first item, it wouldn't affect keypress counts

[stefanhaller]

I'm not so sure the ambiguity is a problem. After all, you enter the view after seeing either mybranch ↑3, or mybranch ↓14, so you know exactly what to expect in these cases. It's only for the case of mybranch ↑4↓9 that you need some visual guidance to tell the sections apart, and for that a simple divider line is sufficient.

[jesseduffield]

My take is that, given that in all other views where we show commits, there's a logical continuity to them, it's worth spelling out what the two sections represent. Using a separator tells you that the two sets are different but using headers instead means you've only got one extra line there which imo is a very low price to pay to be explicit.

[stefanhaller]

OK, happy to try that once we have the functionality in place.

[jesseduffield]

One issue I've noticed is that if you scroll all the way down and then scroll all the way up again, you can't see the header. Wonder how easy that is to fix

[stefanhaller]

One issue I've noticed is that if you scroll all the way down and then scroll all the way up again, you can't see the header. Wonder how easy that is to fix

Yes, I noticed this myself. I have another very old branch that would (mostly) take care of this; let me brush it up and post it as a draft PR to see if that solves it enough. I'll be back here in a minute.

[stefanhaller]

One issue I've noticed is that if you scroll all the way down and then scroll all the way up again, you can't see the header. Wonder how easy that is to fix

Yes, I noticed this myself. I have another very old branch that would (mostly) take care of this; let me brush it up and post it as a draft PR to see if that solves it enough. I'll be back here in a minute.

All right, it's #2915. BTW, it's one of the changes that I wouldn't want to miss any more, I realize this whenever I work on a master-based branch that doesn't have it. Makes a huge difference to me.

This wouldn't solve it in 100% of the cases, but it will hopefully be good enough in practice. It only solves it if the first real model item that you're moving to is out of view. So one example where it wouldn't kick in is if you use the scroll wheel to scroll by just one line, so that only the section header is out of view. Since the default scroll height is 2, this is not possible by default.

But it would also be possible if you open the upstream divergence view for a branch that has only outgoing changes, so there are two consecutive section headers at the top. In that case, scroll the two section headers out of view with one tick of the scroll wheel, and then you can't get them back by pressing up arrow.

I find this obscure enough that I would be willing to accept it like this.

[jesseduffield]

Just confirming this PR doesn't need any more input from me atm

[stefanhaller]

I'm in the process of heavily rewriting everything here to support section headers in the left/right view. This is going to require a complete re-review, I'm afraid... I hope to have something ready Any Day Now (tm).

[jesseduffield]

haha all good

[stefanhaller]

I pushed a reworked version of the branch. It's much longer than before because I wanted to lay the ground for being able to use it in the left/right view. I tried hard to make the git history readable; do use the commit messages as a guide. 😄

[jesseduffield]

Looking great. Left some comments.

I've got a patch to propose which unfortunately touches on both this PR and #2871. The idea is to avoid the type-cast by storing both the desired column and the content on the NonModelItem. Lemme know your thoughts.

diff --git a/pkg/gui/context/list_renderer.go b/pkg/gui/context/list_renderer.go
index 6c63b1a08..a6302eb53 100644
--- a/pkg/gui/context/list_renderer.go
+++ b/pkg/gui/context/list_renderer.go
@@ -12,10 +12,10 @@ import (
 type NonModelItem struct {
 	// Where in the model this should be inserted
 	Index int
-
-	// Whatever data is needed to render the item. In many cases a simple string
-	// will suffice
-	ClientData any
+	// Content to render
+	Content string
+	// The column from which to render the item
+	Column int
 }
 
 type ListRenderer struct {
@@ -30,11 +30,6 @@ type ListRenderer struct {
 	// Function to insert non-model items (e.g. section headers). If nil, no
 	// such items are inserted
 	getNonModelItems func() []*NonModelItem
-	// Function to render a non-model item. The column positions of the display
-	// strings are passed in so that it's possible to align section headers with
-	// columns. Only used if getNonModelItems is provided, can be left nil
-	// otherwise.
-	renderNonModelItem func(item *NonModelItem, columnPositions []int) string
 
 	// The remaining fields are private and shouldn't be initialized by clients
 	numNonModelItems        int
@@ -120,7 +115,11 @@ func (self *ListRenderer) insertNonModelItems(
 			break
 		}
 		if item.Index+offset >= startIdx {
-			lines = slices.Insert(lines, item.Index+offset-startIdx, self.renderNonModelItem(item, columnPositions))
+			padding := ""
+			if item.Column != 0 {
+				padding = strings.Repeat(" ", columnPositions[item.Column])
+			}
+			lines = slices.Insert(lines, item.Index+offset-startIdx, padding+item.Content)
 		}
 		offset++
 	}
diff --git a/pkg/gui/context/list_renderer_test.go b/pkg/gui/context/list_renderer_test.go
index 96cfbb725..98e3f60aa 100644
--- a/pkg/gui/context/list_renderer_test.go
+++ b/pkg/gui/context/list_renderer_test.go
@@ -110,16 +110,15 @@ func TestListRenderer_renderLines(t *testing.T) {
 		t.Run(s.name, func(t *testing.T) {
 			viewModel := NewListViewModel[string](func() []string { return s.modelStrings })
 			var getNonModelItems func() []*NonModelItem
-			var renderNonModelItem func(item *NonModelItem, columnPositions []int) string
 			if s.nonModelIndices != nil {
 				getNonModelItems = func() []*NonModelItem {
 					return lo.Map(s.nonModelIndices, func(modelIndex int, nonModelIndex int) *NonModelItem {
-						return &NonModelItem{Index: modelIndex, ClientData: fmt.Sprintf("--- %d (%d) ---", modelIndex, nonModelIndex)}
+						return &NonModelItem{
+							Index:   modelIndex,
+							Content: fmt.Sprintf("--- %d (%d) ---", modelIndex, nonModelIndex),
+						}
 					})
 				}
-				renderNonModelItem = func(item *NonModelItem, columnPositions []int) string {
-					return item.ClientData.(string)
-				}
 			}
 			self := &ListRenderer{
 				list: viewModel,
@@ -127,8 +126,7 @@ func TestListRenderer_renderLines(t *testing.T) {
 					return lo.Map(s.modelStrings[startIdx:endIdx],
 						func(s string, _ int) []string { return []string{s} })
 				},
-				getNonModelItems:   getNonModelItems,
-				renderNonModelItem: renderNonModelItem,
+				getNonModelItems: getNonModelItems,
 			}
 
 			expectedOutput := strings.Join(lo.Map(
@@ -230,7 +228,7 @@ func TestListRenderer_ModelIndexToViewIndex_and_back(t *testing.T) {
 			if s.nonModelIndices != nil {
 				getNonModelItems = func() []*NonModelItem {
 					return lo.Map(s.nonModelIndices, func(modelIndex int, _ int) *NonModelItem {
-						return &NonModelItem{Index: modelIndex}
+						return &NonModelItem{Index: modelIndex, Content: ""}
 					})
 				}
 			}
@@ -240,8 +238,7 @@ func TestListRenderer_ModelIndexToViewIndex_and_back(t *testing.T) {
 					return lo.Map(modelInts[startIdx:endIdx],
 						func(i int, _ int) []string { return []string{fmt.Sprint(i)} })
 				},
-				getNonModelItems:   getNonModelItems,
-				renderNonModelItem: func(_ *NonModelItem, _ []int) string { return "" },
+				getNonModelItems: getNonModelItems,
 			}
 
 			// Need to render first so that it knows the non-model items
diff --git a/pkg/gui/context/menu_context.go b/pkg/gui/context/menu_context.go
index affe058ad..cd425f6a4 100644
--- a/pkg/gui/context/menu_context.go
+++ b/pkg/gui/context/menu_context.go
@@ -1,8 +1,6 @@
 package context
 
 import (
-	"strings"
-
 	"github.com/jesseduffield/lazygit/pkg/gui/keybindings"
 	"github.com/jesseduffield/lazygit/pkg/gui/style"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
@@ -41,7 +39,6 @@ func NewMenuContext(
 				getDisplayStrings:   viewModel.GetDisplayStrings,
 				getColumnAlignments: func() []utils.Alignment { return viewModel.columnAlignment },
 				getNonModelItems:    viewModel.GetNonModelItems,
-				renderNonModelItem:  viewModel.RenderNonModelItem,
 			},
 			c: c,
 		},
@@ -134,20 +131,20 @@ func (self *MenuViewModel) GetNonModelItems() []*NonModelItem {
 	menuItems := self.FilteredListViewModel.GetItems()
 	var prevSection *types.MenuSection = nil
 	for i, menuItem := range menuItems {
+		menuItem := menuItem
 		if menuItem.Section != nil && menuItem.Section != prevSection {
 			if prevSection != nil {
 				result = append(result, &NonModelItem{
-					Index:      i,
-					ClientData: MenuSectionRenderInfo{},
+					Index:   i,
+					Column:  1,
+					Content: "",
 				})
 			}
 
 			result = append(result, &NonModelItem{
-				Index: i,
-				ClientData: MenuSectionRenderInfo{
-					text:   menuItem.Section.Title,
-					column: menuItem.Section.Column,
-				},
+				Index:   i,
+				Column:  1,
+				Content: style.FgGreen.SetBold().Sprintf("--- %s ---", menuItem.Section.Title),
 			})
 			prevSection = menuItem.Section
 		}
@@ -156,17 +153,6 @@ func (self *MenuViewModel) GetNonModelItems() []*NonModelItem {
 	return result
 }
 
-func (self *MenuViewModel) RenderNonModelItem(item *NonModelItem, columnPositions []int) string {
-	renderInfo := item.ClientData.(MenuSectionRenderInfo)
-
-	if renderInfo.text == "" {
-		return ""
-	}
-
-	padding := strings.Repeat(" ", columnPositions[renderInfo.column])
-	return padding + style.FgGreen.SetBold().Sprintf("--- %s ---", renderInfo.text)
-}
-
 func (self *MenuContext) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding {
 	basicBindings := self.ListContextTrait.GetKeybindings(opts)
 	menuItemsWithKeys := lo.Filter(self.menuItems, func(item *types.MenuItem, _ int) bool {
diff --git a/pkg/gui/context/sub_commits_context.go b/pkg/gui/context/sub_commits_context.go
index 20c5fb21e..7bf34d190 100644
--- a/pkg/gui/context/sub_commits_context.go
+++ b/pkg/gui/context/sub_commits_context.go
@@ -90,8 +90,8 @@ func NewSubCommitsContext(
 				upstreamIdx = 0
 			}
 			result = append(result, &NonModelItem{
-				Index:      upstreamIdx,
-				ClientData: fmt.Sprintf("--- %s ---", c.Tr.DivergenceSectionHeaderRemote),
+				Index:   upstreamIdx,
+				Content: fmt.Sprintf("--- %s ---", c.Tr.DivergenceSectionHeaderRemote),
 			})
 
 			_, localIdx, found := lo.FindIndexOf(
@@ -100,18 +100,14 @@ func NewSubCommitsContext(
 				localIdx = len(c.Model().SubCommits)
 			}
 			result = append(result, &NonModelItem{
-				Index:      localIdx,
-				ClientData: fmt.Sprintf("--- %s ---", c.Tr.DivergenceSectionHeaderLocal),
+				Index:   localIdx,
+				Content: fmt.Sprintf("--- %s ---", c.Tr.DivergenceSectionHeaderLocal),
 			})
 		}
 
 		return result
 	}
 
-	renderNonModelItem := func(item *NonModelItem, columnPositions []int) string {
-		return item.ClientData.(string)
-	}
-
 	ctx := &SubCommitsContext{
 		c:                   c,
 		SubCommitsViewModel: viewModel,
@@ -127,10 +123,9 @@ func NewSubCommitsContext(
 				Transient:  true,
 			})),
 			ListRenderer: ListRenderer{
-				list:               viewModel,
-				getDisplayStrings:  getDisplayStrings,
-				getNonModelItems:   getNonModelItems,
-				renderNonModelItem: renderNonModelItem,
+				list:              viewModel,
+				getDisplayStrings: getDisplayStrings,
+				getNonModelItems:  getNonModelItems,
 			},
 			c:                       c,
 			refreshViewportOnChange: true,

[jesseduffield]

I would like to have many more headings than just these, where each binding specifies which group it belongs to, and in that scheme it won't do to have a tuple being returned by getBindings, but I'm fine with this for now.

[jesseduffield]

I have a heuristic that says if we do something and then revert what we did, there exists a simpler solution where we don't need to revert anything. I've spent some time seeing if I can simplify things by adding a 'Hide' field to ColumnConfig and using column configs for everything rather than having a separate padWidths variable, but I haven't managed to get tests passing and the code seems to be more complicated, so I'm happy to leave it

[stefanhaller]

I agree, and I didn't like having to put things back that we removed earlier. However, I spent a lot of time on this, and tried several other approaches (e.g. reordering code so that we calculate the pad widths first), but none of them really worked out, so I settled on this in the end, not feeling great about it.

[stefanhaller]

I'm a bit torn on the patch. On the one hand it's nice that we don't need the extra render function. However, it limits the flexibility; right now you can only have one string in the header and left-align it with one column. If we ever have a more involved list view with section headers that have multiple strings, each of which needs to be aligned with a certain column, then the old approach would allow this, but the new one doesn't.

Maybe that's ok and we'll never have that use case, in which case the simpler design is good. I'm undecided, can you just make a call? I'm happy to work the changes back into the two PRs, that shouldn't be the problem.

[jesseduffield]

I say we go with the simple design. As you say, we can deal with more complex use cases as they come up

[stefanhaller]

I worked your changes into the branches (it was pretty easy), and force-pushed both PRs. I added one more fixup to this one.

[stefanhaller]

strings.Repeat works with a zero count, and columnPositions[0] is always 0, so we can simplify this.

[jesseduffield]

LGTM, great work!