feat(list): implement GlobalIndex helper

[nobe4]

This commit introduces a new filteredItem field called index which stores the index of the filtered item in the unfiltered list. This allows to get at runtime the unfiltered list index for the selected (and possibly filtered) item.

This is the only solution I found to use SetItem with a filtered list.

The name GlobalIndex might not be ideal, I'm happy to change it to something else. (UnfilteredIndex?)

Fixes: #550

Example

Consider the following code:

package main

import (
	"fmt"
	"io"
	"log"
	"os"

	"github.com/charmbracelet/bubbles/list"
	tea "github.com/charmbracelet/bubbletea"
)

type item struct {
	title   string
	checked bool
}

func (i item) Title() string       { return i.title }
func (_ item) Description() string { return "" }
func (i item) FilterValue() string { return i.title }

type itemDelegate struct{}

func (_ itemDelegate) Height() int                             { return 1 }
func (_ itemDelegate) Spacing() int                            { return 0 }
func (_ itemDelegate) Update(_ tea.Msg, _ *list.Model) tea.Cmd { return nil }
func (_ itemDelegate) Render(w io.Writer, m list.Model, index int, listItem list.Item) {
	i, ok := listItem.(item)
	if !ok {
		return
	}

	cursor := " "
	if index == m.Index() {
		cursor = ">"
	}

	checked := " "
	if i.checked {
		checked = "x"
	}

	fmt.Fprint(w, cursor+checked+i.title)
}

type model struct {
	list list.Model
}

func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
	switch msg := msg.(type) {

	case tea.KeyMsg:
		switch msg.String() {
		case "q":
			return m, tea.Quit

		case " ":
			log.Print("using global index")
			i := m.list.SelectedItem().(item)
			i.checked = !i.checked
			return m, m.list.SetItem(m.list.GlobalIndex(), i)

		case "x":
			log.Print("using index")
			i := m.list.SelectedItem().(item)
			i.checked = !i.checked
			return m, m.list.SetItem(m.list.Index(), i)
		}
	}

	var cmd tea.Cmd
	m.list, cmd = m.list.Update(msg)
	return m, cmd
}

func (_ model) Init() tea.Cmd { return nil }
func (m model) View() string  { return m.list.View() }

func main() {
	choices := []string{"a", "b1", "b2", "b3", "c"}

	items := make([]list.Item, 0, len(choices))
	for _, i := range choices {
		items = append(items, item{title: i})
	}

	l := list.New(items, itemDelegate{}, 0, 0)
	l.SetHeight(20)

	f, err := tea.LogToFile("debug.log", "debug")
	if err != nil {
		fmt.Println("fatal:", err)
		os.Exit(1)
	}
	defer f.Close()

	model := model{list: l}
	p := tea.NewProgram(model)
	if _, err := p.Run(); err != nil {
		panic(err)
	}

	fmt.Printf("%#v\n", items)
}

Selecting with " " and "x" behaves similarly before filtering. Upon filtering, "x" behaves as described in #550 (i.e. wrongly) and " " behaves correctly: it updates the correct item in place.

[bashbunni]

Ah this is such a good catch! Thank you so much for working on this. I'm thinking it might make sense to integrate your change directly into Index rather than creating a new function. It seems that Index and Cursor tend to be the same value once the cursor updates while in reality we should be preserving the item's original index as you've done here.

I just wrote a test that showcases the behaviour you've added with this PR + the Index/Cursor dynamic https://github.com/charmbracelet/bubbles/blob/testindex/list/list_test.go#L76-L142

Let me know if you're cool with me adding this test to your PR. If you can think of a case where you need Index to remain as-is where GlobalIndex wouldn't fit the bill, please let me know!

[bashbunni]

I'll want to refactor the test before merging so there are more conditions to satisfy. Right now it's more of a walkthrough on the behaviour of this change. If you have any suggestions to improve it as well, I'm all ears :)

[nobe4]

Thanks for the review @bashbunni ❤️

I'm thinking it might make sense to integrate your change directly into Index [...] we should be preserving the item's original index as you've done here.

I explicitly added a new function/field because my understanding of Index and Cursor differed from what I wanted:

• Cursor: The current item position in the visible list
• Index: The item position in the filtered list
• GlobalIndex: The item position in the unfiltered list

So if I want to get the visible position of the item, only Cursor can currently give me this information. And if I want the item position in the filtered list, only Index can give me this information.

E.g. (assuming the cursor is on the item: GId = GlobalIndex, Id = Index, C = Cursor)

Filter: A / Item per Page: 2 / Page: 1
GId  Id  C   Item  Comment
0    0   0   A     Hidden by page
1    x   x   B     Hidden by filter
2    1   1   A     Hidden by page
3    2   0   A     Visible
4    3   1   A     Visible
5    x   x   B     Hidden by filter
6    4   0   A     Hidden by page
7    x   x   B     Hidden by filter
8    5   1   A     Hidden by page
9    6   0   A     Hidden by page

It seems to me that the Index naming should be reversed to be more explicit and less confusing:

• Index: The position of the item in the list
• FilteredIndex: The position of the item in the filtered list

But that might require a bigger codechange and introduce a breaking change, so I understand if this is not ideal.

Alternatively, Index could become a struct to erase all confusion:

type Index struct {
   Filtered    int
   Unfiltered  int
}

func (m Model) Index() int {
        filteredIndex := m.Paginator.Page*m.Paginator.PerPage + m.cursor
        unfilteredIndex := filteredIndex

	if m.filteredItems != nil && index < len(m.filteredItems) {
		unfilteredIndex = m.filteredItems[index].index
	}

        return Index {
            Filtered: filteredIndex,
            Unfiltered: unfilteredIndex,
        }
}

But this would be even more disruptive to existing codebase.

Let me know if you're cool with me adding this test to your PR. If you can think of a case where you need Index to remain as-is where GlobalIndex wouldn't fit the bill, please let me know! + I'll want to refactor the test before merging so there are more conditions to satisfy.

Go ahead, and I'll have a look once you've done so :) Feel free to ping me!
Thanks for writing this test!

[bashbunni]

Thank you!! I will take another look when I have a moment. I do think this is a valuable fix here 🙏

[nobe4]

Hi @bashbunni 👋
Any updates here? I have another issue coming up that would benefits from this as well 😬

Let me know if / how I can help push this forward 🙏

[g026r]

Could I ask for an update the godoc for Index() at the same time?

It currently states the following, which implies that it operates across the unfiltered list:

// Index returns the index of the currently selected item as it appears in the
// entire slice of items.

[nobe4]

@g026r done in 11d4e11

[bashbunni]

@nobe4 thank you so much for bringing this to our attention and for your patience!