From ad56140a04ce2c9d25a5c1cf713cdd894490c015 Mon Sep 17 00:00:00 2001 From: Raphael Amorim Date: Wed, 27 Aug 2025 18:03:55 +0200 Subject: [PATCH] fix: scroll viewport (need to remove logs yet --- internal/tui/exp/list/list.go | 83 ++++++-- internal/tui/exp/list/list_test.go | 305 +++++++++++++++++++++++++++++ 2 files changed, 376 insertions(+), 12 deletions(-) diff --git a/internal/tui/exp/list/list.go b/internal/tui/exp/list/list.go index eb1c9da9a9c78811a3fc43d763c85078b40d729e..291e783dfe10cd9f566b9805205f4ce0f9fa60d5 100644 --- a/internal/tui/exp/list/list.go +++ b/internal/tui/exp/list/list.go @@ -1,6 +1,7 @@ package list import ( + "log" "slices" "strings" "sync" @@ -552,22 +553,32 @@ func (l *list[T]) viewPosition() (int, int) { // View position in the virtual space start, end := 0, 0 if l.direction == DirectionForward { - start = l.offset + // Ensure offset doesn't exceed the maximum valid offset + maxOffset := max(0, l.virtualHeight-l.height) + actualOffset := min(l.offset, maxOffset) + + start = actualOffset if l.virtualHeight > 0 { - end = min(l.offset+l.height-1, l.virtualHeight-1) + end = min(actualOffset+l.height-1, l.virtualHeight-1) } else { - end = l.offset + l.height - 1 + end = actualOffset + l.height - 1 } } else { // For backward direction if l.virtualHeight > 0 { - end = l.virtualHeight - l.offset - 1 + // Ensure offset doesn't exceed the maximum valid offset + maxOffset := max(0, l.virtualHeight-l.height) + actualOffset := min(l.offset, maxOffset) + + end = l.virtualHeight - actualOffset - 1 start = max(0, end-l.height+1) } else { end = 0 start = 0 } } + log.Printf("[viewPosition] direction=%v, offset=%d, height=%d, virtualHeight=%d -> start=%d, end=%d", + l.direction, l.offset, l.height, l.virtualHeight, start, end) return start, end } @@ -943,10 +954,13 @@ func (l *list[T]) renderVirtualScrolling() string { // Calculate viewport bounds viewStart, viewEnd := l.viewPosition() + log.Printf("[renderVirtualScrolling] START: viewStart=%d, viewEnd=%d, offset=%d, height=%d, virtualHeight=%d, direction=%v, selectedIndex=%d", + viewStart, viewEnd, l.offset, l.height, l.virtualHeight, l.direction, l.selectedIndex) // Check if we have any positions calculated if len(l.itemPositions) == 0 { // No positions calculated yet, return empty viewport + log.Printf("[renderVirtualScrolling] No item positions calculated yet") return "" } @@ -958,8 +972,10 @@ func (l *list[T]) renderVirtualScrolling() string { } itemsLen := l.items.Len() + log.Printf("[renderVirtualScrolling] Checking %d items for visibility", itemsLen) for i := 0; i < itemsLen; i++ { if i >= len(l.itemPositions) { + log.Printf("[renderVirtualScrolling] Skipping item %d: no position data", i) continue } @@ -969,44 +985,56 @@ func (l *list[T]) renderVirtualScrolling() string { if pos.end >= viewStart && pos.start <= viewEnd { item, ok := l.items.Get(i) if !ok { + log.Printf("[renderVirtualScrolling] Item %d not found in items slice", i) continue } + log.Printf("[renderVirtualScrolling] Item %d (id=%s) is visible: pos.start=%d, pos.end=%d, height=%d", + i, item.ID(), pos.start, pos.end, pos.height) visibleItems = append(visibleItems, struct { item T pos itemPosition index int }{item, pos, i}) + } else { + log.Printf("[renderVirtualScrolling] Item %d not visible: pos.start=%d, pos.end=%d (viewport: %d-%d)", + i, pos.start, pos.end, viewStart, viewEnd) } // Early exit if we've passed the viewport if pos.start > viewEnd { + log.Printf("[renderVirtualScrolling] Early exit at item %d: pos.start=%d > viewEnd=%d", i, pos.start, viewEnd) break } } // Build the rendered output + log.Printf("[renderVirtualScrolling] Found %d visible items", len(visibleItems)) var lines []string currentLine := viewStart - for _, vis := range visibleItems { + for idx, vis := range visibleItems { // Get or render the item's view var view string if cached, ok := l.viewCache.Get(vis.item.ID()); ok { view = cached + log.Printf("[renderVirtualScrolling] Using cached view for item %d (id=%s)", vis.index, vis.item.ID()) } else { view = vis.item.View() l.viewCache.Set(vis.item.ID(), view) + log.Printf("[renderVirtualScrolling] Rendered new view for item %d (id=%s)", vis.index, vis.item.ID()) } itemLines := strings.Split(view, "\n") + log.Printf("[renderVirtualScrolling] Item %d has %d lines, currentLine=%d", vis.index, len(itemLines), currentLine) - // Add gap lines before item if needed (except for first item) - if vis.index > 0 && currentLine < vis.pos.start { + // Add gap lines before item if needed (except for first visible item) + if idx > 0 && currentLine < vis.pos.start { gapLines := vis.pos.start - currentLine + log.Printf("[renderVirtualScrolling] Adding %d gap lines before item %d", gapLines, vis.index) for i := 0; i < gapLines; i++ { lines = append(lines, "") - currentLine++ } + currentLine = vis.pos.start } // Determine which lines of this item to include @@ -1014,30 +1042,61 @@ func (l *list[T]) renderVirtualScrolling() string { if vis.pos.start < viewStart { // Item starts before viewport, skip some lines startLine = viewStart - vis.pos.start + log.Printf("[renderVirtualScrolling] Item %d starts before viewport, skipping %d lines", vis.index, startLine) } // Add the item's visible lines - for i := startLine; i < len(itemLines) && currentLine <= viewEnd; i++ { - lines = append(lines, itemLines[i]) - currentLine++ + linesAdded := 0 + maxLinesToAdd := len(itemLines) - startLine + for i := 0; i < maxLinesToAdd && len(lines) < l.height; i++ { + lines = append(lines, itemLines[startLine + i]) + linesAdded++ + } + + // Update currentLine to track our position in virtual space + if vis.pos.start < viewStart { + // Item started before viewport, we're now at viewStart + linesAdded + currentLine = viewStart + linesAdded + } else { + // Normal case: we're at the item's start position + lines added + currentLine = vis.pos.start + linesAdded } + + log.Printf("[renderVirtualScrolling] Added %d lines from item %d (visible item #%d), currentLine now=%d", + linesAdded, vis.index, idx, currentLine) } // For content that fits entirely in viewport, don't pad with empty lines // Only pad if we have scrolled or if content is larger than viewport + log.Printf("[renderVirtualScrolling] Before padding: %d lines, virtualHeight=%d, height=%d, offset=%d", + len(lines), l.virtualHeight, l.height, l.offset) + if l.virtualHeight > l.height || l.offset > 0 { // Fill remaining viewport with empty lines if needed + initialLen := len(lines) for len(lines) < l.height { lines = append(lines, "") } + if len(lines) > initialLen { + log.Printf("[renderVirtualScrolling] Added %d padding lines", len(lines)-initialLen) + } // Trim to viewport height if len(lines) > l.height { + log.Printf("[renderVirtualScrolling] Trimming from %d to %d lines", len(lines), l.height) lines = lines[:l.height] } } - return strings.Join(lines, "\n") + result := strings.Join(lines, "\n") + resultHeight := lipgloss.Height(result) + log.Printf("[renderVirtualScrolling] FINAL: Returning %d lines (height=%d), expected viewport height=%d", + len(lines), resultHeight, l.height) + if resultHeight < l.height && len(visibleItems) > 0 { + log.Printf("[renderVirtualScrolling] WARNING: Rendered fewer lines than viewport! Missing %d lines", + l.height-resultHeight) + } + return result } // AppendItem implements List. diff --git a/internal/tui/exp/list/list_test.go b/internal/tui/exp/list/list_test.go index 3381adf249c725d4b8de560c0ea5bf706adbfa62..982ccee270149950b6bcbeb0dc5690a49adc14d9 100644 --- a/internal/tui/exp/list/list_test.go +++ b/internal/tui/exp/list/list_test.go @@ -14,6 +14,311 @@ import ( "github.com/stretchr/testify/require" ) +func TestViewPosition(t *testing.T) { + t.Parallel() + + t.Run("forward direction - normal scrolling", func(t *testing.T) { + t.Parallel() + items := []Item{createItem("test", 1)} + l := New(items, WithDirectionForward(), WithSize(20, 10)).(*list[Item]) + l.virtualHeight = 50 + + // At the top + l.offset = 0 + start, end := l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 9, end) + + // In the middle + l.offset = 20 + start, end = l.viewPosition() + assert.Equal(t, 20, start) + assert.Equal(t, 29, end) + + // Near the bottom + l.offset = 40 + start, end = l.viewPosition() + assert.Equal(t, 40, start) + assert.Equal(t, 49, end) + + // Past the maximum valid offset (should be clamped) + l.offset = 45 + start, end = l.viewPosition() + assert.Equal(t, 40, start) // Clamped to max valid offset + assert.Equal(t, 49, end) + + // Way past the end (should be clamped) + l.offset = 100 + start, end = l.viewPosition() + assert.Equal(t, 40, start) // Clamped to max valid offset + assert.Equal(t, 49, end) + }) + + t.Run("forward direction - edge case with exact fit", func(t *testing.T) { + t.Parallel() + items := []Item{createItem("test", 1)} + l := New(items, WithDirectionForward(), WithSize(20, 10)).(*list[Item]) + l.virtualHeight = 10 + + l.offset = 0 + start, end := l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 9, end) + + // Offset beyond valid range should be clamped + l.offset = 5 + start, end = l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 9, end) + }) + + t.Run("forward direction - content smaller than viewport", func(t *testing.T) { + t.Parallel() + items := []Item{createItem("test", 1)} + l := New(items, WithDirectionForward(), WithSize(20, 10)).(*list[Item]) + l.virtualHeight = 5 + + l.offset = 0 + start, end := l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 4, end) + + // Any offset should be clamped to 0 + l.offset = 10 + start, end = l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 4, end) + }) + + t.Run("backward direction - normal scrolling", func(t *testing.T) { + t.Parallel() + items := []Item{createItem("test", 1)} + l := New(items, WithDirectionBackward(), WithSize(20, 10)).(*list[Item]) + l.virtualHeight = 50 + + // At the bottom (offset 0 in backward mode) + l.offset = 0 + start, end := l.viewPosition() + assert.Equal(t, 40, start) + assert.Equal(t, 49, end) + + // In the middle + l.offset = 20 + start, end = l.viewPosition() + assert.Equal(t, 20, start) + assert.Equal(t, 29, end) + + // Near the top + l.offset = 40 + start, end = l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 9, end) + + // Past the maximum valid offset (should be clamped) + l.offset = 45 + start, end = l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 9, end) + }) + + t.Run("backward direction - edge cases", func(t *testing.T) { + t.Parallel() + items := []Item{createItem("test", 1)} + l := New(items, WithDirectionBackward(), WithSize(20, 10)).(*list[Item]) + l.virtualHeight = 5 + + // Content smaller than viewport + l.offset = 0 + start, end := l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 4, end) + + // Any offset should show all content + l.offset = 10 + start, end = l.viewPosition() + assert.Equal(t, 0, start) + assert.Equal(t, 4, end) + }) +} + +// Helper to create a test item with specific height +func createItem(id string, height int) Item { + content := strings.Repeat(id+"\n", height) + if height > 0 { + content = strings.TrimSuffix(content, "\n") + } + item := &testItem{ + id: id, + content: content, + } + return item +} + +func TestRenderVirtualScrolling(t *testing.T) { + t.Parallel() + + t.Run("should handle partially visible items at top", func(t *testing.T) { + t.Parallel() + items := []Item{ + createItem("A", 1), + createItem("B", 5), + createItem("C", 1), + createItem("D", 3), + } + + l := New(items, WithDirectionForward(), WithSize(20, 3)).(*list[Item]) + execCmd(l, l.Init()) + + // Position B partially visible at top + l.offset = 2 // Start viewing from line 2 (middle of B) + l.calculateItemPositions() + + // Item positions: A(0-0), B(1-5), C(6-6), D(7-9) + // Viewport: lines 2-4 (height=3) + // Should show: lines 2-4 of B (3 lines from B) + + rendered := l.renderVirtualScrolling() + lines := strings.Split(rendered, "\n") + assert.Equal(t, 3, len(lines)) + assert.Equal(t, "B", lines[0]) + assert.Equal(t, "B", lines[1]) + assert.Equal(t, "B", lines[2]) + }) + + t.Run("should handle gaps between items correctly", func(t *testing.T) { + t.Parallel() + items := []Item{ + createItem("A", 1), + createItem("B", 1), + createItem("C", 1), + } + + l := New(items, WithDirectionForward(), WithSize(5, 20), WithGap(1)).(*list[Item]) + execCmd(l, l.Init()) + + // Item positions: A(0-0), gap(1), B(2-2), gap(3), C(4-4) + // Viewport: lines 0-4 (height=5) + // Should show all items with gaps + + rendered := l.renderVirtualScrolling() + lines := strings.Split(rendered, "\n") + assert.Equal(t, 5, len(lines)) + assert.Equal(t, "A", lines[0]) + assert.Equal(t, "", lines[1]) // gap + assert.Equal(t, "B", lines[2]) + assert.Equal(t, "", lines[3]) // gap + assert.Equal(t, "C", lines[4]) + }) + + t.Run("should not show empty lines when scrolled to bottom", func(t *testing.T) { + t.Parallel() + items := []Item{ + createItem("A", 2), + createItem("B", 2), + createItem("C", 2), + createItem("D", 2), + createItem("E", 2), + } + + l := New(items, WithDirectionForward(), WithSize(4, 20)).(*list[Item]) + execCmd(l, l.Init()) + l.calculateItemPositions() + + // Total height: 10 lines (5 items * 2 lines each) + // Scroll to show last 4 lines + l.offset = 6 + + rendered := l.renderVirtualScrolling() + lines := strings.Split(rendered, "\n") + assert.Equal(t, 4, len(lines)) + // Should show last 2 items completely + assert.Equal(t, "D", lines[0]) + assert.Equal(t, "D", lines[1]) + assert.Equal(t, "E", lines[2]) + assert.Equal(t, "E", lines[3]) + }) + + t.Run("should handle offset at maximum boundary", func(t *testing.T) { + t.Parallel() + items := []Item{ + createItem("A", 3), + createItem("B", 3), + createItem("C", 3), + createItem("D", 3), + } + + l := New(items, WithDirectionForward(), WithSize(5, 20)).(*list[Item]) + execCmd(l, l.Init()) + l.calculateItemPositions() + + // Total height: 12 lines + // Max valid offset: 12 - 5 = 7 + l.offset = 7 + + rendered := l.renderVirtualScrolling() + lines := strings.Split(rendered, "\n") + assert.Equal(t, 5, len(lines)) + // Should show from line 7 to 11 + assert.Contains(t, rendered, "C") + assert.Contains(t, rendered, "D") + + // Try setting offset beyond max - should be clamped + l.offset = 20 + rendered = l.renderVirtualScrolling() + lines = strings.Split(rendered, "\n") + assert.Equal(t, 5, len(lines)) + // Should still show the same content as offset=7 + assert.Contains(t, rendered, "C") + assert.Contains(t, rendered, "D") + }) +} + +// testItem is a simple implementation of Item for testing +type testItem struct { + id string + content string +} + +func (t *testItem) ID() string { + return t.id +} + +func (t *testItem) View() string { + return t.content +} + +func (t *testItem) Selectable() bool { + return true +} + +func (t *testItem) Height() int { + return lipgloss.Height(t.content) +} + +func (t *testItem) Init() tea.Cmd { + return nil +} + +func (t *testItem) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + return t, nil +} + +func (t *testItem) SetSize(width, height int) tea.Cmd { + return nil +} + +func (t *testItem) GetSize() (int, int) { + return 0, lipgloss.Height(t.content) +} + +func (t *testItem) SetFocused(focused bool) tea.Cmd { + return nil +} + +func (t *testItem) Focused() bool { + return false +} + func TestList(t *testing.T) { t.Parallel() t.Run("should have correct positions in list that fits the items", func(t *testing.T) {