fix(daemon): redraw committed floating geometry
- commit
- 789f8b138e20cdf7643e332774f1570e2dc4485d
- author
- brice · · 2026-07-10 11:22 UTC
- parent
- 1169241
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +88 -3
M internal/usecase/daemon/floating_render.go +12 -3
| @@ -92,8 +92,6 @@ func composeFloatingFrame(base renderer.Frame, baseDamage []renderer.Damage, p * | |||
| 92 | 92 | (overlayBackdrop{DimPaneContents: true}).apply(frame, content, layoutSnap, theme) | |
| 93 | 93 | desiredGeometry := calculateFloatingGeometry(content, cfg) | |
| 94 | 94 | ||
| 95 | - | popupChanged := cache == nil || cache.floating != p || cache.floatingGeneration != generation | |
| 96 | - | ||
| 97 | 95 | // The PTY reader mutates both the frame and its damage under p.mu. Keep | |
| 98 | 96 | // every read of them, including the blit, in the same critical section. | |
| 99 | 97 | // In particular, Frame is not safe to retain as an alias after unlocking. | |
| @@ -110,11 +108,16 @@ func composeFloatingFrame(base renderer.Frame, baseDamage []renderer.Damage, p * | |||
| 110 | 108 | p.screen.ClearDamage() | |
| 111 | 109 | p.mu.Unlock() | |
| 112 | 110 | ||
| 111 | + | // The committed geometry, rather than the requested config geometry, is | |
| 112 | + | // what was actually rendered. Track it so movement (including a same-size | |
| 113 | + | // resize) cannot reuse a cached popup at its former position. | |
| 114 | + | popupChanged := cache == nil || cache.floating != p || cache.floatingGeneration != generation || cache.floatingGeometry != geometry | |
| 113 | 115 | titleChanged := popupChanged || cache == nil || cache.floatingTitleGeneration != titleGeneration | |
| 114 | 116 | drawFloatingBorder(frame, geometry.Bounds, title, newThemeStyles(theme).border) | |
| 115 | 117 | if cache != nil { | |
| 116 | 118 | cache.floating = p | |
| 117 | 119 | cache.floatingGeneration = generation | |
| 120 | + | cache.floatingGeometry = geometry | |
| 118 | 121 | cache.floatingTitleGeneration = titleGeneration | |
| 119 | 122 | } | |
| 120 | 123 | if full || popupChanged { | |
| @@ -167,7 +170,13 @@ func (d *Daemon) resizeFloatingPane(p *pane, geometry floatingGeometry) bool { | |||
| 167 | 170 | old := p.rect | |
| 168 | 171 | pty := p.pty | |
| 169 | 172 | p.mu.Unlock() | |
| 170 | - | if old == inner { | |
| 173 | + | if old.Width == inner.Width && old.Height == inner.Height { | |
| 174 | + | // Position and outer bounds can change while the terminal dimensions do | |
| 175 | + | // not. Commit that geometry without needlessly resetting the PTY/screen. | |
| 176 | + | p.mu.Lock() | |
| 177 | + | p.rect = inner | |
| 178 | + | p.popupGeometry = geometry | |
| 179 | + | p.mu.Unlock() | |
| 171 | 180 | return true | |
| 172 | 181 | } | |
| 173 | 182 | if pty != nil { | |
| @@ -92,8 +92,6 @@ func composeFloatingFrame(base renderer.Frame, baseDamage []renderer.Damage, p * | |||||
| 92 | (overlayBackdrop{DimPaneContents: true}).apply(frame, content, layoutSnap, theme) | 92 | (overlayBackdrop{DimPaneContents: true}).apply(frame, content, layoutSnap, theme) | ||
| 93 | desiredGeometry := calculateFloatingGeometry(content, cfg) | 93 | desiredGeometry := calculateFloatingGeometry(content, cfg) | ||
| 94 | 94 | ||||
| 95 | - | popupChanged := cache == nil || cache.floating != p || cache.floatingGeneration != generation | |||
| 96 | - | ||||
| 97 | // The PTY reader mutates both the frame and its damage under p.mu. Keep | 95 | // The PTY reader mutates both the frame and its damage under p.mu. Keep | ||
| 98 | // every read of them, including the blit, in the same critical section. | 96 | // every read of them, including the blit, in the same critical section. | ||
| 99 | // In particular, Frame is not safe to retain as an alias after unlocking. | 97 | // In particular, Frame is not safe to retain as an alias after unlocking. | ||
| @@ -110,11 +108,16 @@ func composeFloatingFrame(base renderer.Frame, baseDamage []renderer.Damage, p * | |||||
| 110 | p.screen.ClearDamage() | 108 | p.screen.ClearDamage() | ||
| 111 | p.mu.Unlock() | 109 | p.mu.Unlock() | ||
| 112 | 110 | ||||
| 111 | + | // The committed geometry, rather than the requested config geometry, is | |||
| 112 | + | // what was actually rendered. Track it so movement (including a same-size | |||
| 113 | + | // resize) cannot reuse a cached popup at its former position. | |||
| 114 | + | popupChanged := cache == nil || cache.floating != p || cache.floatingGeneration != generation || cache.floatingGeometry != geometry | |||
| 113 | titleChanged := popupChanged || cache == nil || cache.floatingTitleGeneration != titleGeneration | 115 | titleChanged := popupChanged || cache == nil || cache.floatingTitleGeneration != titleGeneration | ||
| 114 | drawFloatingBorder(frame, geometry.Bounds, title, newThemeStyles(theme).border) | 116 | drawFloatingBorder(frame, geometry.Bounds, title, newThemeStyles(theme).border) | ||
| 115 | if cache != nil { | 117 | if cache != nil { | ||
| 116 | cache.floating = p | 118 | cache.floating = p | ||
| 117 | cache.floatingGeneration = generation | 119 | cache.floatingGeneration = generation | ||
| 120 | + | cache.floatingGeometry = geometry | |||
| 118 | cache.floatingTitleGeneration = titleGeneration | 121 | cache.floatingTitleGeneration = titleGeneration | ||
| 119 | } | 122 | } | ||
| 120 | if full || popupChanged { | 123 | if full || popupChanged { | ||
| @@ -167,7 +170,13 @@ func (d *Daemon) resizeFloatingPane(p *pane, geometry floatingGeometry) bool { | |||||
| 167 | old := p.rect | 170 | old := p.rect | ||
| 168 | pty := p.pty | 171 | pty := p.pty | ||
| 169 | p.mu.Unlock() | 172 | p.mu.Unlock() | ||
| 170 | - | if old == inner { | 173 | + | if old.Width == inner.Width && old.Height == inner.Height { |
| 174 | + | // Position and outer bounds can change while the terminal dimensions do | |||
| 175 | + | // not. Commit that geometry without needlessly resetting the PTY/screen. | |||
| 176 | + | p.mu.Lock() | |||
| 177 | + | p.rect = inner | |||
| 178 | + | p.popupGeometry = geometry | |||
| 179 | + | p.mu.Unlock() | |||
| 171 | return true | 180 | return true | ||
| 172 | } | 181 | } | ||
| 173 | if pty != nil { | 182 | if pty != nil { | ||
M internal/usecase/daemon/floating_render_test.go +74 -0
| @@ -81,6 +81,79 @@ func TestComposeFloatingFrameDoesNotMutateSourceAndDamagesTitle(t *testing.T) { | |||
| 81 | 81 | require.Equal(t, 1, damage[0].Height) | |
| 82 | 82 | } | |
| 83 | 83 | ||
| 84 | + | func TestComposeFloatingFrameGeometryChangesInvalidateCache(t *testing.T) { | |
| 85 | + | initial := floatingGeometry{ | |
| 86 | + | Bounds: domain.Rect{X: 4, Y: 2, Width: 8, Height: 5}, | |
| 87 | + | Inner: domain.Rect{X: 5, Y: 3, Width: 6, Height: 3}, | |
| 88 | + | } | |
| 89 | + | tests := []struct { | |
| 90 | + | name string | |
| 91 | + | next floatingGeometry | |
| 92 | + | }{ | |
| 93 | + | { | |
| 94 | + | name: "position change", | |
| 95 | + | next: floatingGeometry{Bounds: domain.Rect{X: 12, Y: 4, Width: 8, Height: 5}, Inner: domain.Rect{X: 13, Y: 5, Width: 6, Height: 3}}, | |
| 96 | + | }, | |
| 97 | + | { | |
| 98 | + | name: "bounds change with same inner size", | |
| 99 | + | next: floatingGeometry{Bounds: domain.Rect{X: 10, Y: 2, Width: 10, Height: 7}, Inner: domain.Rect{X: 12, Y: 4, Width: 6, Height: 3}}, | |
| 100 | + | }, | |
| 101 | + | } | |
| 102 | + | for _, tt := range tests { | |
| 103 | + | t.Run(tt.name, func(t *testing.T) { | |
| 104 | + | p := newPane("floating", nil, domain.Size{Cols: 6, Rows: 3}) | |
| 105 | + | p.rect, p.popupGeometry = initial.Inner, initial | |
| 106 | + | cache := &composedFrameCache{} | |
| 107 | + | base := renderer.NewFrame(30, 12) | |
| 108 | + | content := domain.Rect{Y: 1, Width: 30, Height: 10} | |
| 109 | + | _, damage := composeFloatingFrame(base, nil, p, 1, content, domain.FloatingConfig{}, tabLayoutSnapshot{}, themeui.Theme{}, cache, false) | |
| 110 | + | require.Equal(t, []renderer.Damage{renderer.FullRedraw()}, damage) | |
| 111 | + | ||
| 112 | + | p.mu.Lock() | |
| 113 | + | p.rect, p.popupGeometry = tt.next.Inner, tt.next | |
| 114 | + | p.mu.Unlock() | |
| 115 | + | _, damage = composeFloatingFrame(base, nil, p, 1, content, domain.FloatingConfig{}, tabLayoutSnapshot{}, themeui.Theme{}, cache, false) | |
| 116 | + | require.Equal(t, []renderer.Damage{renderer.FullRedraw()}, damage) | |
| 117 | + | require.Equal(t, tt.next, cache.floatingGeometry) | |
| 118 | + | }) | |
| 119 | + | } | |
| 120 | + | } | |
| 121 | + | ||
| 122 | + | func TestResizeFloatingPaneCommitsSameSizeGeometryWithoutPTYResize(t *testing.T) { | |
| 123 | + | initial := floatingGeometry{ | |
| 124 | + | Bounds: domain.Rect{X: 1, Y: 1, Width: 8, Height: 5}, | |
| 125 | + | Inner: domain.Rect{X: 2, Y: 2, Width: 6, Height: 3}, | |
| 126 | + | } | |
| 127 | + | tests := []struct { | |
| 128 | + | name string | |
| 129 | + | next floatingGeometry | |
| 130 | + | }{ | |
| 131 | + | { | |
| 132 | + | name: "position change", | |
| 133 | + | next: floatingGeometry{Bounds: domain.Rect{X: 9, Y: 4, Width: 8, Height: 5}, Inner: domain.Rect{X: 10, Y: 5, Width: 6, Height: 3}}, | |
| 134 | + | }, | |
| 135 | + | { | |
| 136 | + | name: "bounds change", | |
| 137 | + | next: floatingGeometry{Bounds: domain.Rect{X: 3, Y: 1, Width: 10, Height: 7}, Inner: domain.Rect{X: 5, Y: 3, Width: 6, Height: 3}}, | |
| 138 | + | }, | |
| 139 | + | } | |
| 140 | + | for _, tt := range tests { | |
| 141 | + | t.Run(tt.name, func(t *testing.T) { | |
| 142 | + | pty := &resizePTY{} | |
| 143 | + | p := newPane("floating", pty, rectSize(initial.Inner)) | |
| 144 | + | p.rect, p.popupGeometry = initial.Inner, initial | |
| 145 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 146 | + | ||
| 147 | + | require.True(t, d.resizeFloatingPane(p, tt.next)) | |
| 148 | + | require.Empty(t, pty.sizes(), "same-size geometry must not resize the PTY") | |
| 149 | + | require.Equal(t, tt.next.Inner, p.rect) | |
| 150 | + | require.Equal(t, tt.next, p.popupGeometry) | |
| 151 | + | require.Equal(t, 6, p.screen.Frame.Width) | |
| 152 | + | require.Equal(t, 3, p.screen.Frame.Height) | |
| 153 | + | }) | |
| 154 | + | } | |
| 155 | + | } | |
| 156 | + | ||
| 84 | 157 | func TestComposeFloatingFrameRendersPaneOwnedCommandFallback(t *testing.T) { | |
| 85 | 158 | d := newTestDaemon(t, nil, stubClock{}) | |
| 86 | 159 | d.shell = "/usr/bin/fish" | |
| @@ -292,6 +365,7 @@ func TestResizeFloatingPaneFailureAndSerialization(t *testing.T) { | |||
| 292 | 365 | requested := floatingGeometry{Bounds: domain.Rect{X: 1, Y: 1, Width: 11, Height: 9}, Inner: domain.Rect{X: 2, Y: 2, Width: 9, Height: 7}} | |
| 293 | 366 | require.False(t, d.resizeFloatingPane(p, requested)) | |
| 294 | 367 | require.Equal(t, domain.Rect{X: 8, Y: 3, Width: 5, Height: 4}, p.rect) | |
| 368 | + | require.Equal(t, floatingGeometry{}, p.popupGeometry) | |
| 295 | 369 | require.Equal(t, 5, p.screen.Frame.Width) | |
| 296 | 370 | require.Equal(t, 4, p.screen.Frame.Height) | |
| 297 | 371 | }) | |
| @@ -81,6 +81,79 @@ func TestComposeFloatingFrameDoesNotMutateSourceAndDamagesTitle(t *testing.T) { | |||||
| 81 | require.Equal(t, 1, damage[0].Height) | 81 | require.Equal(t, 1, damage[0].Height) | ||
| 82 | } | 82 | } | ||
| 83 | 83 | ||||
| 84 | + | func TestComposeFloatingFrameGeometryChangesInvalidateCache(t *testing.T) { | |||
| 85 | + | initial := floatingGeometry{ | |||
| 86 | + | Bounds: domain.Rect{X: 4, Y: 2, Width: 8, Height: 5}, | |||
| 87 | + | Inner: domain.Rect{X: 5, Y: 3, Width: 6, Height: 3}, | |||
| 88 | + | } | |||
| 89 | + | tests := []struct { | |||
| 90 | + | name string | |||
| 91 | + | next floatingGeometry | |||
| 92 | + | }{ | |||
| 93 | + | { | |||
| 94 | + | name: "position change", | |||
| 95 | + | next: floatingGeometry{Bounds: domain.Rect{X: 12, Y: 4, Width: 8, Height: 5}, Inner: domain.Rect{X: 13, Y: 5, Width: 6, Height: 3}}, | |||
| 96 | + | }, | |||
| 97 | + | { | |||
| 98 | + | name: "bounds change with same inner size", | |||
| 99 | + | next: floatingGeometry{Bounds: domain.Rect{X: 10, Y: 2, Width: 10, Height: 7}, Inner: domain.Rect{X: 12, Y: 4, Width: 6, Height: 3}}, | |||
| 100 | + | }, | |||
| 101 | + | } | |||
| 102 | + | for _, tt := range tests { | |||
| 103 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 104 | + | p := newPane("floating", nil, domain.Size{Cols: 6, Rows: 3}) | |||
| 105 | + | p.rect, p.popupGeometry = initial.Inner, initial | |||
| 106 | + | cache := &composedFrameCache{} | |||
| 107 | + | base := renderer.NewFrame(30, 12) | |||
| 108 | + | content := domain.Rect{Y: 1, Width: 30, Height: 10} | |||
| 109 | + | _, damage := composeFloatingFrame(base, nil, p, 1, content, domain.FloatingConfig{}, tabLayoutSnapshot{}, themeui.Theme{}, cache, false) | |||
| 110 | + | require.Equal(t, []renderer.Damage{renderer.FullRedraw()}, damage) | |||
| 111 | + | ||||
| 112 | + | p.mu.Lock() | |||
| 113 | + | p.rect, p.popupGeometry = tt.next.Inner, tt.next | |||
| 114 | + | p.mu.Unlock() | |||
| 115 | + | _, damage = composeFloatingFrame(base, nil, p, 1, content, domain.FloatingConfig{}, tabLayoutSnapshot{}, themeui.Theme{}, cache, false) | |||
| 116 | + | require.Equal(t, []renderer.Damage{renderer.FullRedraw()}, damage) | |||
| 117 | + | require.Equal(t, tt.next, cache.floatingGeometry) | |||
| 118 | + | }) | |||
| 119 | + | } | |||
| 120 | + | } | |||
| 121 | + | ||||
| 122 | + | func TestResizeFloatingPaneCommitsSameSizeGeometryWithoutPTYResize(t *testing.T) { | |||
| 123 | + | initial := floatingGeometry{ | |||
| 124 | + | Bounds: domain.Rect{X: 1, Y: 1, Width: 8, Height: 5}, | |||
| 125 | + | Inner: domain.Rect{X: 2, Y: 2, Width: 6, Height: 3}, | |||
| 126 | + | } | |||
| 127 | + | tests := []struct { | |||
| 128 | + | name string | |||
| 129 | + | next floatingGeometry | |||
| 130 | + | }{ | |||
| 131 | + | { | |||
| 132 | + | name: "position change", | |||
| 133 | + | next: floatingGeometry{Bounds: domain.Rect{X: 9, Y: 4, Width: 8, Height: 5}, Inner: domain.Rect{X: 10, Y: 5, Width: 6, Height: 3}}, | |||
| 134 | + | }, | |||
| 135 | + | { | |||
| 136 | + | name: "bounds change", | |||
| 137 | + | next: floatingGeometry{Bounds: domain.Rect{X: 3, Y: 1, Width: 10, Height: 7}, Inner: domain.Rect{X: 5, Y: 3, Width: 6, Height: 3}}, | |||
| 138 | + | }, | |||
| 139 | + | } | |||
| 140 | + | for _, tt := range tests { | |||
| 141 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 142 | + | pty := &resizePTY{} | |||
| 143 | + | p := newPane("floating", pty, rectSize(initial.Inner)) | |||
| 144 | + | p.rect, p.popupGeometry = initial.Inner, initial | |||
| 145 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 146 | + | ||||
| 147 | + | require.True(t, d.resizeFloatingPane(p, tt.next)) | |||
| 148 | + | require.Empty(t, pty.sizes(), "same-size geometry must not resize the PTY") | |||
| 149 | + | require.Equal(t, tt.next.Inner, p.rect) | |||
| 150 | + | require.Equal(t, tt.next, p.popupGeometry) | |||
| 151 | + | require.Equal(t, 6, p.screen.Frame.Width) | |||
| 152 | + | require.Equal(t, 3, p.screen.Frame.Height) | |||
| 153 | + | }) | |||
| 154 | + | } | |||
| 155 | + | } | |||
| 156 | + | ||||
| 84 | func TestComposeFloatingFrameRendersPaneOwnedCommandFallback(t *testing.T) { | 157 | func TestComposeFloatingFrameRendersPaneOwnedCommandFallback(t *testing.T) { | ||
| 85 | d := newTestDaemon(t, nil, stubClock{}) | 158 | d := newTestDaemon(t, nil, stubClock{}) | ||
| 86 | d.shell = "/usr/bin/fish" | 159 | d.shell = "/usr/bin/fish" | ||
| @@ -292,6 +365,7 @@ func TestResizeFloatingPaneFailureAndSerialization(t *testing.T) { | |||||
| 292 | requested := floatingGeometry{Bounds: domain.Rect{X: 1, Y: 1, Width: 11, Height: 9}, Inner: domain.Rect{X: 2, Y: 2, Width: 9, Height: 7}} | 365 | requested := floatingGeometry{Bounds: domain.Rect{X: 1, Y: 1, Width: 11, Height: 9}, Inner: domain.Rect{X: 2, Y: 2, Width: 9, Height: 7}} | ||
| 293 | require.False(t, d.resizeFloatingPane(p, requested)) | 366 | require.False(t, d.resizeFloatingPane(p, requested)) | ||
| 294 | require.Equal(t, domain.Rect{X: 8, Y: 3, Width: 5, Height: 4}, p.rect) | 367 | require.Equal(t, domain.Rect{X: 8, Y: 3, Width: 5, Height: 4}, p.rect) | ||
| 368 | + | require.Equal(t, floatingGeometry{}, p.popupGeometry) | |||
| 295 | require.Equal(t, 5, p.screen.Frame.Width) | 369 | require.Equal(t, 5, p.screen.Frame.Width) | ||
| 296 | require.Equal(t, 4, p.screen.Frame.Height) | 370 | require.Equal(t, 4, p.screen.Frame.Height) | ||
| 297 | }) | 371 | }) | ||
M internal/usecase/daemon/render.go +2 -0
| @@ -593,6 +593,7 @@ type composedFrameCache struct { | |||
| 593 | 593 | floating *pane | |
| 594 | 594 | floatingFrame renderer.Frame | |
| 595 | 595 | floatingGeneration uint64 | |
| 596 | + | floatingGeometry floatingGeometry | |
| 596 | 597 | floatingTitleGeneration uint64 | |
| 597 | 598 | } | |
| 598 | 599 | ||
| @@ -600,6 +601,7 @@ func (c *composedFrameCache) invalidate() { | |||
| 600 | 601 | c.valid = false | |
| 601 | 602 | c.floating = nil | |
| 602 | 603 | c.floatingGeneration = 0 | |
| 604 | + | c.floatingGeometry = floatingGeometry{} | |
| 603 | 605 | c.floatingTitleGeneration = 0 | |
| 604 | 606 | } | |
| 605 | 607 | ||
| @@ -593,6 +593,7 @@ type composedFrameCache struct { | |||||
| 593 | floating *pane | 593 | floating *pane | ||
| 594 | floatingFrame renderer.Frame | 594 | floatingFrame renderer.Frame | ||
| 595 | floatingGeneration uint64 | 595 | floatingGeneration uint64 | ||
| 596 | + | floatingGeometry floatingGeometry | |||
| 596 | floatingTitleGeneration uint64 | 597 | floatingTitleGeneration uint64 | ||
| 597 | } | 598 | } | ||
| 598 | 599 | ||||
| @@ -600,6 +601,7 @@ func (c *composedFrameCache) invalidate() { | |||||
| 600 | c.valid = false | 601 | c.valid = false | ||
| 601 | c.floating = nil | 602 | c.floating = nil | ||
| 602 | c.floatingGeneration = 0 | 603 | c.floatingGeneration = 0 | ||
| 604 | + | c.floatingGeometry = floatingGeometry{} | |||
| 603 | c.floatingTitleGeneration = 0 | 605 | c.floatingTitleGeneration = 0 | ||
| 604 | } | 606 | } | ||
| 605 | 607 | ||||