From 4b0f84f8c6a9027bd536628f68ac1cb123cff91e Mon Sep 17 00:00:00 2001 From: Kujtim Hoxha Date: Sat, 26 Jul 2025 12:17:36 +0200 Subject: [PATCH] chore: address review requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove Slice() method from csync.Slice and use slices.Collect(s.Seq()) instead - Optimize Seq2() method to avoid calling removed Slice() method - Update Seq() to use Seq2() for better performance - Update all references throughout the codebase to use the new pattern 💖 Generated with Crush Co-Authored-By: Crush --- internal/csync/slices.go | 19 +++++-------------- internal/csync/slices_test.go | 7 ++++--- internal/tui/exp/list/filterable.go | 2 +- internal/tui/exp/list/filterable_group.go | 3 ++- internal/tui/exp/list/grouped.go | 4 +++- internal/tui/exp/list/list.go | 21 +++++++++++---------- 6 files changed, 26 insertions(+), 30 deletions(-) diff --git a/internal/csync/slices.go b/internal/csync/slices.go index ba47062b51de1a038836467f15bd4e9672ab3303..176e53d13585517df72b28969a9ced54383706f9 100644 --- a/internal/csync/slices.go +++ b/internal/csync/slices.go @@ -112,15 +112,6 @@ func (s *Slice[T]) Len() int { return len(s.inner) } -// Slice returns a copy of the underlying slice. -func (s *Slice[T]) Slice() []T { - s.mu.RLock() - defer s.mu.RUnlock() - result := make([]T, len(s.inner)) - copy(result, s.inner) - return result -} - // SetSlice replaces the entire slice with a new one. func (s *Slice[T]) SetSlice(items []T) { s.mu.Lock() @@ -138,10 +129,8 @@ func (s *Slice[T]) Clear() { // Seq returns an iterator that yields elements from the slice. func (s *Slice[T]) Seq() iter.Seq[T] { - // Take a snapshot to avoid holding the lock during iteration - items := s.Slice() return func(yield func(T) bool) { - for _, v := range items { + for _, v := range s.Seq2() { if !yield(v) { return } @@ -151,8 +140,10 @@ func (s *Slice[T]) Seq() iter.Seq[T] { // Seq2 returns an iterator that yields index-value pairs from the slice. func (s *Slice[T]) Seq2() iter.Seq2[int, T] { - // Take a snapshot to avoid holding the lock during iteration - items := s.Slice() + s.mu.RLock() + items := make([]T, len(s.inner)) + copy(items, s.inner) + s.mu.RUnlock() return func(yield func(int, T) bool) { for i, v := range items { if !yield(i, v) { diff --git a/internal/csync/slices_test.go b/internal/csync/slices_test.go index d86b02537fe0534ed60de0e314e1e1d9b7b866b6..ad4d7b408deb55ad68bc62d5327096573df8f8b6 100644 --- a/internal/csync/slices_test.go +++ b/internal/csync/slices_test.go @@ -1,6 +1,7 @@ package csync import ( + "slices" "sync" "sync/atomic" "testing" @@ -145,7 +146,7 @@ func TestSlice(t *testing.T) { assert.Equal(t, 4, s.Len()) expected := []int{1, 2, 4, 5} - actual := s.Slice() + actual := slices.Collect(s.Seq()) assert.Equal(t, expected, actual) // Delete out of bounds @@ -203,7 +204,7 @@ func TestSlice(t *testing.T) { s.SetSlice(newItems) assert.Equal(t, 3, s.Len()) - assert.Equal(t, newItems, s.Slice()) + assert.Equal(t, newItems, slices.Collect(s.Seq())) // Verify it's a copy newItems[0] = 999 @@ -224,7 +225,7 @@ func TestSlice(t *testing.T) { original := []int{1, 2, 3} s := NewSliceFrom(original) - copy := s.Slice() + copy := slices.Collect(s.Seq()) assert.Equal(t, original, copy) // Verify it's a copy diff --git a/internal/tui/exp/list/filterable.go b/internal/tui/exp/list/filterable.go index 909bcf8a42c728aea398bdc16794b63d7d6e725d..b2b7412488d7233236f6d09ac2b7c374a34ef959 100644 --- a/internal/tui/exp/list/filterable.go +++ b/internal/tui/exp/list/filterable.go @@ -97,7 +97,7 @@ func NewFilterableList[T FilterableItem](items []T, opts ...filterableListOption f.list = New[T](items, f.listOptions...).(*list[T]) f.updateKeyMaps() - f.items = f.list.items.Slice() + f.items = slices.Collect(f.list.items.Seq()) if f.inputHidden { return f diff --git a/internal/tui/exp/list/filterable_group.go b/internal/tui/exp/list/filterable_group.go index 2543723b4b74072510c31453441495d27ec05c9d..88b790c678aede588956bce4df002b766646feff 100644 --- a/internal/tui/exp/list/filterable_group.go +++ b/internal/tui/exp/list/filterable_group.go @@ -2,6 +2,7 @@ package list import ( "regexp" + "slices" "sort" "strings" @@ -179,7 +180,7 @@ func (f *filterableGroupList[T]) inputHeight() int { func (f *filterableGroupList[T]) Filter(query string) tea.Cmd { var cmds []tea.Cmd - for _, item := range f.items.Slice() { + for _, item := range slices.Collect(f.items.Seq()) { if i, ok := any(item).(layout.Focusable); ok { cmds = append(cmds, i.Blur()) } diff --git a/internal/tui/exp/list/grouped.go b/internal/tui/exp/list/grouped.go index 4b9124fdd0a0e3335d3b22ef122dc5cd86c5785b..cb54628a70e84cb80eeb162a0d9f836f14271641 100644 --- a/internal/tui/exp/list/grouped.go +++ b/internal/tui/exp/list/grouped.go @@ -1,6 +1,8 @@ package list import ( + "slices" + tea "github.com/charmbracelet/bubbletea/v2" "github.com/charmbracelet/crush/internal/csync" "github.com/charmbracelet/crush/internal/tui/components/core/layout" @@ -89,7 +91,7 @@ func (g *groupedList[T]) convertItems() { func (g *groupedList[T]) SetGroups(groups []Group[T]) tea.Cmd { g.groups = groups g.convertItems() - return g.SetItems(g.items.Slice()) + return g.SetItems(slices.Collect(g.items.Seq())) } func (g *groupedList[T]) Groups() []Group[T] { diff --git a/internal/tui/exp/list/list.go b/internal/tui/exp/list/list.go index 3af90d405382ba5df207774c4f2fba109717034a..8deae123c0df651b21c9f256b2e9ace72a5f7b00 100644 --- a/internal/tui/exp/list/list.go +++ b/internal/tui/exp/list/list.go @@ -1,6 +1,7 @@ package list import ( + "slices" "strings" "github.com/charmbracelet/bubbles/v2/key" @@ -201,7 +202,7 @@ func (l *list[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return l, nil case anim.StepMsg: var cmds []tea.Cmd - for _, item := range l.items.Slice() { + for _, item := range slices.Collect(l.items.Seq()) { if i, ok := any(item).(HasAnim); ok && i.Spinning() { updated, cmd := i.Update(msg) cmds = append(cmds, cmd) @@ -288,7 +289,7 @@ func (l *list[T]) viewPosition() (int, int) { func (l *list[T]) recalculateItemPositions() { currentContentHeight := 0 - for _, item := range l.items.Slice() { + for _, item := range slices.Collect(l.items.Seq()) { rItem, ok := l.renderedItems.Get(item.ID()) if !ok { continue @@ -561,7 +562,7 @@ func (l *list[T]) focusSelectedItem() tea.Cmd { return nil } var cmds []tea.Cmd - for _, item := range l.items.Slice() { + for _, item := range slices.Collect(l.items.Seq()) { if f, ok := any(item).(layout.Focusable); ok { if item.ID() == l.selectedItem && !f.IsFocused() { cmds = append(cmds, f.Focus()) @@ -580,7 +581,7 @@ func (l *list[T]) blurSelectedItem() tea.Cmd { return nil } var cmds []tea.Cmd - for _, item := range l.items.Slice() { + for _, item := range slices.Collect(l.items.Seq()) { if f, ok := any(item).(layout.Focusable); ok { if item.ID() == l.selectedItem && f.IsFocused() { cmds = append(cmds, f.Blur()) @@ -655,7 +656,7 @@ func (l *list[T]) AppendItem(item T) tea.Cmd { l.items.Append(item) l.indexMap = csync.NewMap[string, int]() - for inx, item := range l.items.Slice() { + for inx, item := range slices.Collect(l.items.Seq()) { l.indexMap.Set(item.ID(), inx) } if l.width > 0 && l.height > 0 { @@ -702,7 +703,7 @@ func (l *list[T]) DeleteItem(id string) tea.Cmd { } l.items.Delete(inx) l.renderedItems.Del(id) - for inx, item := range l.items.Slice() { + for inx, item := range slices.Collect(l.items.Seq()) { l.indexMap.Set(item.ID(), inx) } @@ -767,7 +768,7 @@ func (l *list[T]) IsFocused() bool { // Items implements List. func (l *list[T]) Items() []T { - return l.items.Slice() + return slices.Collect(l.items.Seq()) } func (l *list[T]) incrementOffset(n int) { @@ -822,7 +823,7 @@ func (l *list[T]) PrependItem(item T) tea.Cmd { } l.items.Prepend(item) l.indexMap = csync.NewMap[string, int]() - for inx, item := range l.items.Slice() { + for inx, item := range slices.Collect(l.items.Seq()) { l.indexMap.Set(item.ID(), inx) } if l.width > 0 && l.height > 0 { @@ -926,7 +927,7 @@ func (l *list[T]) SelectedItem() *T { func (l *list[T]) SetItems(items []T) tea.Cmd { l.items.SetSlice(items) var cmds []tea.Cmd - for inx, item := range l.items.Slice() { + for inx, item := range slices.Collect(l.items.Seq()) { if i, ok := any(item).(Indexable); ok { i.SetIndex(inx) } @@ -949,7 +950,7 @@ func (l *list[T]) reset(selectedItem string) tea.Cmd { l.selectedItem = selectedItem l.indexMap = csync.NewMap[string, int]() l.renderedItems = csync.NewMap[string, renderedItem]() - for inx, item := range l.items.Slice() { + for inx, item := range slices.Collect(l.items.Seq()) { l.indexMap.Set(item.ID(), inx) if l.width > 0 && l.height > 0 { cmds = append(cmds, item.SetSize(l.width, l.height))