detached 7137fd1

fix: invalidate stale split ratio callbacks

commit
7137fd10fe86097f230324272cd8171eda5cb602
author
brice · · 2026-07-15 07:26 UTC
parent
6903d03
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +139 -8
Unified
UnifiedSplit
M internal/ui/layout/split.go +34 -8
@@ -23,6 +23,7 @@ type SplitView struct {
2323 onRatioChanged func(ratio float64)
2424 pendingNotifyRatio float64
2525 notifyDebounceTimer *time.Timer
26+ notifyGeneration uint64
2627 tickCallbackID uint
2728 cleanedUp bool
2829
@@ -40,6 +41,9 @@ const notifyPositionDebounceDelay = 100 * time.Millisecond
4041 const notifyPositionSuppressAfterApplyDelay = 50 * time.Millisecond
4142 const splitAllocationSettledTolerance = 2
4243
44+// idleAdd is a seam for scheduling work on GLib's main loop.
45+var idleAdd = glib.IdleAdd
46+
4347 func childAllocation(widget Widget) (int, int) {
4448 if widget == nil {
4549 return 0, 0
@@ -153,25 +157,42 @@ func NewSplitView(
153157 ratio := clampRatio(float64(position) / float64(totalSize))
154158
155159 sv.mu.Lock()
160+ if sv.cleanedUp {
161+ sv.mu.Unlock()
162+ return
163+ }
156164 sv.ratio = ratio
157165 sv.pendingNotifyRatio = ratio
158- onRatioChanged := sv.onRatioChanged
166+ sv.notifyGeneration++
167+ generation := sv.notifyGeneration
159168
160169 if sv.notifyDebounceTimer != nil {
161170 sv.notifyDebounceTimer.Stop()
162171 }
163172 sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {
164- if onRatioChanged == nil {
173+ sv.mu.RLock()
174+ valid := !sv.cleanedUp && generation == sv.notifyGeneration
175+ sv.mu.RUnlock()
176+ if !valid {
165177 return
166178 }
179+
167180 cb := glib.SourceFunc(func(_ uintptr) bool {
168181 sv.mu.RLock()
182+ if sv.cleanedUp || generation != sv.notifyGeneration {
183+ sv.mu.RUnlock()
184+ return false
185+ }
186+ onRatioChanged := sv.onRatioChanged
169187 pending := sv.pendingNotifyRatio
170188 sv.mu.RUnlock()
171- onRatioChanged(pending)
189+
190+ if onRatioChanged != nil {
191+ onRatioChanged(pending)
192+ }
172193 return false
173194 })
174- glib.IdleAdd(&cb, 0)
195+ idleAdd(&cb, 0)
175196 })
176197
177198 sv.mu.Unlock()
@@ -219,18 +240,23 @@ func NewSplitView(
219240 // tree rebuild and GTK teardown can both reach the same view.
220241 func (sv *SplitView) Cleanup() {
221242 sv.mu.Lock()
222- defer sv.mu.Unlock()
223243 if sv.cleanedUp {
244+ sv.mu.Unlock()
224245 return
225246 }
226247 sv.cleanedUp = true
248+ sv.notifyGeneration++
249+ sv.onRatioChanged = nil
227250 if sv.notifyDebounceTimer != nil {
228251 sv.notifyDebounceTimer.Stop()
229252 sv.notifyDebounceTimer = nil
230253 }
231- if sv.tickCallbackID != 0 {
232- sv.paned.RemoveTickCallback(sv.tickCallbackID)
233- sv.tickCallbackID = 0
254+ tickCallbackID := sv.tickCallbackID
255+ sv.tickCallbackID = 0
256+ sv.mu.Unlock()
257+
258+ if tickCallbackID != 0 {
259+ sv.paned.RemoveTickCallback(tickCallbackID)
234260 }
235261 }
236262
@@ -23,6 +23,7 @@ type SplitView struct {
23 onRatioChanged func(ratio float64)23 onRatioChanged func(ratio float64)
24 pendingNotifyRatio float6424 pendingNotifyRatio float64
25 notifyDebounceTimer *time.Timer25 notifyDebounceTimer *time.Timer
26+ notifyGeneration uint64
26 tickCallbackID uint27 tickCallbackID uint
27 cleanedUp bool28 cleanedUp bool
28 29
@@ -40,6 +41,9 @@ const notifyPositionDebounceDelay = 100 * time.Millisecond
40 const notifyPositionSuppressAfterApplyDelay = 50 * time.Millisecond41 const notifyPositionSuppressAfterApplyDelay = 50 * time.Millisecond
41 const splitAllocationSettledTolerance = 242 const splitAllocationSettledTolerance = 2
42 43
44+// idleAdd is a seam for scheduling work on GLib's main loop.
45+var idleAdd = glib.IdleAdd
46+
43 func childAllocation(widget Widget) (int, int) {47 func childAllocation(widget Widget) (int, int) {
44 if widget == nil {48 if widget == nil {
45 return 0, 049 return 0, 0
@@ -153,25 +157,42 @@ func NewSplitView(
153 ratio := clampRatio(float64(position) / float64(totalSize))157 ratio := clampRatio(float64(position) / float64(totalSize))
154 158
155 sv.mu.Lock()159 sv.mu.Lock()
160+ if sv.cleanedUp {
161+ sv.mu.Unlock()
162+ return
163+ }
156 sv.ratio = ratio164 sv.ratio = ratio
157 sv.pendingNotifyRatio = ratio165 sv.pendingNotifyRatio = ratio
158- onRatioChanged := sv.onRatioChanged166+ sv.notifyGeneration++
167+ generation := sv.notifyGeneration
159 168
160 if sv.notifyDebounceTimer != nil {169 if sv.notifyDebounceTimer != nil {
161 sv.notifyDebounceTimer.Stop()170 sv.notifyDebounceTimer.Stop()
162 }171 }
163 sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {172 sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {
164- if onRatioChanged == nil {173+ sv.mu.RLock()
174+ valid := !sv.cleanedUp && generation == sv.notifyGeneration
175+ sv.mu.RUnlock()
176+ if !valid {
165 return177 return
166 }178 }
179+
167 cb := glib.SourceFunc(func(_ uintptr) bool {180 cb := glib.SourceFunc(func(_ uintptr) bool {
168 sv.mu.RLock()181 sv.mu.RLock()
182+ if sv.cleanedUp || generation != sv.notifyGeneration {
183+ sv.mu.RUnlock()
184+ return false
185+ }
186+ onRatioChanged := sv.onRatioChanged
169 pending := sv.pendingNotifyRatio187 pending := sv.pendingNotifyRatio
170 sv.mu.RUnlock()188 sv.mu.RUnlock()
171- onRatioChanged(pending)189+
190+ if onRatioChanged != nil {
191+ onRatioChanged(pending)
192+ }
172 return false193 return false
173 })194 })
174- glib.IdleAdd(&cb, 0)195+ idleAdd(&cb, 0)
175 })196 })
176 197
177 sv.mu.Unlock()198 sv.mu.Unlock()
@@ -219,18 +240,23 @@ func NewSplitView(
219 // tree rebuild and GTK teardown can both reach the same view.240 // tree rebuild and GTK teardown can both reach the same view.
220 func (sv *SplitView) Cleanup() {241 func (sv *SplitView) Cleanup() {
221 sv.mu.Lock()242 sv.mu.Lock()
222- defer sv.mu.Unlock()
223 if sv.cleanedUp {243 if sv.cleanedUp {
244+ sv.mu.Unlock()
224 return245 return
225 }246 }
226 sv.cleanedUp = true247 sv.cleanedUp = true
248+ sv.notifyGeneration++
249+ sv.onRatioChanged = nil
227 if sv.notifyDebounceTimer != nil {250 if sv.notifyDebounceTimer != nil {
228 sv.notifyDebounceTimer.Stop()251 sv.notifyDebounceTimer.Stop()
229 sv.notifyDebounceTimer = nil252 sv.notifyDebounceTimer = nil
230 }253 }
231- if sv.tickCallbackID != 0 {254+ tickCallbackID := sv.tickCallbackID
232- sv.paned.RemoveTickCallback(sv.tickCallbackID)255+ sv.tickCallbackID = 0
233- sv.tickCallbackID = 0256+ sv.mu.Unlock()
257+
258+ if tickCallbackID != 0 {
259+ sv.paned.RemoveTickCallback(tickCallbackID)
234 }260 }
235 }261 }
236 262
A internal/ui/layout/split_notify_cleanup_test.go +105 -0
@@ -0,0 +1,105 @@
1+package layout
2+
3+import (
4+ "context"
5+ "sync/atomic"
6+ "testing"
7+ "time"
8+
9+ "github.com/stretchr/testify/require"
10+
11+ "github.com/bnema/puregotk/v4/glib"
12+)
13+
14+// notifyingPaned only implements the SplitView interactions exercised below.
15+// Embedding the interface supplies the unused methods.
16+type notifyingPaned struct {
17+ PanedWidget
18+ position int
19+ notify func()
20+}
21+
22+func (p *notifyingPaned) SetResizeStartChild(bool) {}
23+func (p *notifyingPaned) SetResizeEndChild(bool) {}
24+func (p *notifyingPaned) SetVisible(bool) {}
25+func (p *notifyingPaned) ConnectNotifyPosition(fn func()) uint { p.notify = fn; return 0 }
26+func (p *notifyingPaned) GetAllocatedWidth() int { return 100 }
27+func (p *notifyingPaned) GetPosition() int { return p.position }
28+func (p *notifyingPaned) SetPosition(position int) { p.position = position }
29+
30+type notifyingFactory struct {
31+ WidgetFactory
32+ paned PanedWidget
33+}
34+
35+func (f notifyingFactory) NewPaned(Orientation) PanedWidget { return f.paned }
36+
37+func newNotifyingSplitView(t *testing.T) (*SplitView, func(int)) {
38+ t.Helper()
39+
40+ paned := &notifyingPaned{}
41+ sv := NewSplitView(context.Background(), notifyingFactory{paned: paned}, OrientationHorizontal, nil, nil, 0)
42+ sv.mu.Lock()
43+ sv.suppressNotifyUntil = time.Time{}
44+ sv.mu.Unlock()
45+ require.NotNil(t, paned.notify)
46+ return sv, func(nextPosition int) {
47+ paned.position = nextPosition
48+ paned.notify()
49+ }
50+}
51+
52+func receiveQueuedIdle(t *testing.T, queued <-chan *glib.SourceFunc) *glib.SourceFunc {
53+ t.Helper()
54+ select {
55+ case source := <-queued:
56+ return source
57+ case <-time.After(time.Second):
58+ t.Fatal("timed out waiting for debounce to queue idle callback")
59+ return nil
60+ }
61+}
62+
63+func TestSplitViewQueuedRatioCallbackDoesNotRunAfterCleanup(t *testing.T) {
64+ queued := make(chan *glib.SourceFunc, 1)
65+ previousIdleAdd := idleAdd
66+ idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
67+ queued <- source
68+ return 1
69+ }
70+ t.Cleanup(func() { idleAdd = previousIdleAdd })
71+
72+ sv, notify := newNotifyingSplitView(t)
73+ var calls atomic.Int32
74+ sv.SetOnRatioChanged(func(float64) { calls.Add(1) })
75+
76+ notify(25)
77+ source := receiveQueuedIdle(t, queued) // Debounce has fired and queued this work.
78+ sv.Cleanup()
79+ (*source)(0) // Run the queued GLib closure after cleanup.
80+
81+ require.Zero(t, calls.Load(), "cleanup must invalidate queued ratio callbacks")
82+}
83+
84+func TestSplitViewSupersededRatioCallbackDeliversOnlyLatestValueOnce(t *testing.T) {
85+ queued := make(chan *glib.SourceFunc, 2)
86+ previousIdleAdd := idleAdd
87+ idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
88+ queued <- source
89+ return 1
90+ }
91+ t.Cleanup(func() { idleAdd = previousIdleAdd })
92+
93+ sv, notify := newNotifyingSplitView(t)
94+ var ratios []float64
95+ sv.SetOnRatioChanged(func(ratio float64) { ratios = append(ratios, ratio) })
96+
97+ notify(20)
98+ first := receiveQueuedIdle(t, queued)
99+ notify(80)
100+ second := receiveQueuedIdle(t, queued)
101+ (*first)(0)
102+ (*second)(0)
103+
104+ require.Equal(t, []float64{0.8}, ratios)
105+}
@@ -0,0 +1,105 @@
1+package layout
2+
3+import (
4+ "context"
5+ "sync/atomic"
6+ "testing"
7+ "time"
8+
9+ "github.com/stretchr/testify/require"
10+
11+ "github.com/bnema/puregotk/v4/glib"
12+)
13+
14+// notifyingPaned only implements the SplitView interactions exercised below.
15+// Embedding the interface supplies the unused methods.
16+type notifyingPaned struct {
17+ PanedWidget
18+ position int
19+ notify func()
20+}
21+
22+func (p *notifyingPaned) SetResizeStartChild(bool) {}
23+func (p *notifyingPaned) SetResizeEndChild(bool) {}
24+func (p *notifyingPaned) SetVisible(bool) {}
25+func (p *notifyingPaned) ConnectNotifyPosition(fn func()) uint { p.notify = fn; return 0 }
26+func (p *notifyingPaned) GetAllocatedWidth() int { return 100 }
27+func (p *notifyingPaned) GetPosition() int { return p.position }
28+func (p *notifyingPaned) SetPosition(position int) { p.position = position }
29+
30+type notifyingFactory struct {
31+ WidgetFactory
32+ paned PanedWidget
33+}
34+
35+func (f notifyingFactory) NewPaned(Orientation) PanedWidget { return f.paned }
36+
37+func newNotifyingSplitView(t *testing.T) (*SplitView, func(int)) {
38+ t.Helper()
39+
40+ paned := &notifyingPaned{}
41+ sv := NewSplitView(context.Background(), notifyingFactory{paned: paned}, OrientationHorizontal, nil, nil, 0)
42+ sv.mu.Lock()
43+ sv.suppressNotifyUntil = time.Time{}
44+ sv.mu.Unlock()
45+ require.NotNil(t, paned.notify)
46+ return sv, func(nextPosition int) {
47+ paned.position = nextPosition
48+ paned.notify()
49+ }
50+}
51+
52+func receiveQueuedIdle(t *testing.T, queued <-chan *glib.SourceFunc) *glib.SourceFunc {
53+ t.Helper()
54+ select {
55+ case source := <-queued:
56+ return source
57+ case <-time.After(time.Second):
58+ t.Fatal("timed out waiting for debounce to queue idle callback")
59+ return nil
60+ }
61+}
62+
63+func TestSplitViewQueuedRatioCallbackDoesNotRunAfterCleanup(t *testing.T) {
64+ queued := make(chan *glib.SourceFunc, 1)
65+ previousIdleAdd := idleAdd
66+ idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
67+ queued <- source
68+ return 1
69+ }
70+ t.Cleanup(func() { idleAdd = previousIdleAdd })
71+
72+ sv, notify := newNotifyingSplitView(t)
73+ var calls atomic.Int32
74+ sv.SetOnRatioChanged(func(float64) { calls.Add(1) })
75+
76+ notify(25)
77+ source := receiveQueuedIdle(t, queued) // Debounce has fired and queued this work.
78+ sv.Cleanup()
79+ (*source)(0) // Run the queued GLib closure after cleanup.
80+
81+ require.Zero(t, calls.Load(), "cleanup must invalidate queued ratio callbacks")
82+}
83+
84+func TestSplitViewSupersededRatioCallbackDeliversOnlyLatestValueOnce(t *testing.T) {
85+ queued := make(chan *glib.SourceFunc, 2)
86+ previousIdleAdd := idleAdd
87+ idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
88+ queued <- source
89+ return 1
90+ }
91+ t.Cleanup(func() { idleAdd = previousIdleAdd })
92+
93+ sv, notify := newNotifyingSplitView(t)
94+ var ratios []float64
95+ sv.SetOnRatioChanged(func(ratio float64) { ratios = append(ratios, ratio) })
96+
97+ notify(20)
98+ first := receiveQueuedIdle(t, queued)
99+ notify(80)
100+ second := receiveQueuedIdle(t, queued)
101+ (*first)(0)
102+ (*second)(0)
103+
104+ require.Equal(t, []float64{0.8}, ratios)
105+}

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 8

Tags 42

↑↓ moveenter openesc close