Merge pull request #353 from bnema/fix/stale-split-ratio-callback

fix: invalidate stale split ratio callbacks

commit: 80795e36108cb9b5fd7a772ebc3f809010a0648b
author: bnema · 3mo ago · 2026-07-15 08:18 UTC
committer: GitHub · 3mo ago · 2026-07-15 08:18 UTC
merge of: 6903d03 + b0266fa · diff against the first parent
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

2 files · +156 -11

M internal/ui/layout/split.go +33 -11


@@ -21,8 +21,9 @@ type SplitView struct {
21	21		logger zerolog.Logger
22	22
23	23		onRatioChanged func(ratio float64)
24		-	pendingNotifyRatio float64
	24	+	idleAdd func(*glib.SourceFunc, uintptr) uint
25	25		notifyDebounceTimer *time.Timer
	26	+	notifyGeneration uint64
26	27		tickCallbackID uint
27	28		cleanedUp bool
28	29

@@ -111,6 +112,7 @@ func NewSplitView(
111	112		endChild: endChild,
112	113		ratio: clampRatio(ratio),
113	114		logger: log.With().Str("component", "split-view").Logger(),
	115	+	idleAdd: glib.IdleAdd,
114	116		}
115	117
116	118		// Set children

@@ -153,25 +155,40 @@ func NewSplitView(
153	155		ratio := clampRatio(float64(position) / float64(totalSize))
154	156
155	157		sv.mu.Lock()
	158	+	if sv.cleanedUp {
	159	+	sv.mu.Unlock()
	160	+	return
	161	+	}
156	162		sv.ratio = ratio
157		-	sv.pendingNotifyRatio = ratio
158		-	onRatioChanged := sv.onRatioChanged
	163	+	sv.notifyGeneration++
	164	+	generation := sv.notifyGeneration
159	165
160	166		if sv.notifyDebounceTimer != nil {
161	167		sv.notifyDebounceTimer.Stop()
162	168		}
163	169		sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {
164		-	if onRatioChanged == nil {
	170	+	sv.mu.RLock()
	171	+	valid := !sv.cleanedUp && generation == sv.notifyGeneration
	172	+	sv.mu.RUnlock()
	173	+	if !valid {
165	174		return
166	175		}
	176	+
167	177		cb := glib.SourceFunc(func(_ uintptr) bool {
168	178		sv.mu.RLock()
169		-	pending := sv.pendingNotifyRatio
	179	+	if sv.cleanedUp \|\| generation != sv.notifyGeneration {
	180	+	sv.mu.RUnlock()
	181	+	return false
	182	+	}
	183	+	onRatioChanged := sv.onRatioChanged
170	184		sv.mu.RUnlock()
171		-	onRatioChanged(pending)
	185	+
	186	+	if onRatioChanged != nil {
	187	+	onRatioChanged(ratio)
	188	+	}
172	189		return false
173	190		})
174		-	glib.IdleAdd(&cb, 0)
	191	+	sv.idleAdd(&cb, 0)
175	192		})
176	193
177	194		sv.mu.Unlock()

@@ -219,18 +236,23 @@ func NewSplitView(
219	236		// tree rebuild and GTK teardown can both reach the same view.
220	237		func (sv *SplitView) Cleanup() {
221	238		sv.mu.Lock()
222		-	defer sv.mu.Unlock()
223	239		if sv.cleanedUp {
	240	+	sv.mu.Unlock()
224	241		return
225	242		}
226	243		sv.cleanedUp = true
	244	+	sv.notifyGeneration++
	245	+	sv.onRatioChanged = nil
227	246		if sv.notifyDebounceTimer != nil {
228	247		sv.notifyDebounceTimer.Stop()
229	248		sv.notifyDebounceTimer = nil
230	249		}
231		-	if sv.tickCallbackID != 0 {
232		-	sv.paned.RemoveTickCallback(sv.tickCallbackID)
233		-	sv.tickCallbackID = 0
	250	+	tickCallbackID := sv.tickCallbackID
	251	+	sv.tickCallbackID = 0
	252	+	sv.mu.Unlock()
	253	+
	254	+	if tickCallbackID != 0 {
	255	+	sv.paned.RemoveTickCallback(tickCallbackID)
234	256		}
235	257		}
236	258


@@ -21,8 +21,9 @@ type SplitView struct {
21		logger zerolog.Logger	21		logger zerolog.Logger
22			22
23		onRatioChanged func(ratio float64)	23		onRatioChanged func(ratio float64)
24	-	pendingNotifyRatio float64	24	+	idleAdd func(*glib.SourceFunc, uintptr) uint
25		notifyDebounceTimer *time.Timer	25		notifyDebounceTimer *time.Timer
			26	+	notifyGeneration uint64
26		tickCallbackID uint	27		tickCallbackID uint
27		cleanedUp bool	28		cleanedUp bool
28			29

@@ -111,6 +112,7 @@ func NewSplitView(
111		endChild: endChild,	112		endChild: endChild,
112		ratio: clampRatio(ratio),	113		ratio: clampRatio(ratio),
113		logger: log.With().Str("component", "split-view").Logger(),	114		logger: log.With().Str("component", "split-view").Logger(),
			115	+	idleAdd: glib.IdleAdd,
114		}	116		}
115			117
116		// Set children	118		// Set children

@@ -153,25 +155,40 @@ func NewSplitView(
153		ratio := clampRatio(float64(position) / float64(totalSize))	155		ratio := clampRatio(float64(position) / float64(totalSize))
154			156
155		sv.mu.Lock()	157		sv.mu.Lock()
			158	+	if sv.cleanedUp {
			159	+	sv.mu.Unlock()
			160	+	return
			161	+	}
156		sv.ratio = ratio	162		sv.ratio = ratio
157	-	sv.pendingNotifyRatio = ratio	163	+	sv.notifyGeneration++
158	-	onRatioChanged := sv.onRatioChanged	164	+	generation := sv.notifyGeneration
159			165
160		if sv.notifyDebounceTimer != nil {	166		if sv.notifyDebounceTimer != nil {
161		sv.notifyDebounceTimer.Stop()	167		sv.notifyDebounceTimer.Stop()
162		}	168		}
163		sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {	169		sv.notifyDebounceTimer = time.AfterFunc(notifyPositionDebounceDelay, func() {
164	-	if onRatioChanged == nil {	170	+	sv.mu.RLock()
			171	+	valid := !sv.cleanedUp && generation == sv.notifyGeneration
			172	+	sv.mu.RUnlock()
			173	+	if !valid {
165		return	174		return
166		}	175		}
			176	+
167		cb := glib.SourceFunc(func(_ uintptr) bool {	177		cb := glib.SourceFunc(func(_ uintptr) bool {
168		sv.mu.RLock()	178		sv.mu.RLock()
169	-	pending := sv.pendingNotifyRatio	179	+	if sv.cleanedUp \|\| generation != sv.notifyGeneration {
			180	+	sv.mu.RUnlock()
			181	+	return false
			182	+	}
			183	+	onRatioChanged := sv.onRatioChanged
170		sv.mu.RUnlock()	184		sv.mu.RUnlock()
171	-	onRatioChanged(pending)	185	+
			186	+	if onRatioChanged != nil {
			187	+	onRatioChanged(ratio)
			188	+	}
172		return false	189		return false
173		})	190		})
174	-	glib.IdleAdd(&cb, 0)	191	+	sv.idleAdd(&cb, 0)
175		})	192		})
176			193
177		sv.mu.Unlock()	194		sv.mu.Unlock()

@@ -219,18 +236,23 @@ func NewSplitView(
219		// tree rebuild and GTK teardown can both reach the same view.	236		// tree rebuild and GTK teardown can both reach the same view.
220		func (sv *SplitView) Cleanup() {	237		func (sv *SplitView) Cleanup() {
221		sv.mu.Lock()	238		sv.mu.Lock()
222	-	defer sv.mu.Unlock()
223		if sv.cleanedUp {	239		if sv.cleanedUp {
			240	+	sv.mu.Unlock()
224		return	241		return
225		}	242		}
226		sv.cleanedUp = true	243		sv.cleanedUp = true
			244	+	sv.notifyGeneration++
			245	+	sv.onRatioChanged = nil
227		if sv.notifyDebounceTimer != nil {	246		if sv.notifyDebounceTimer != nil {
228		sv.notifyDebounceTimer.Stop()	247		sv.notifyDebounceTimer.Stop()
229		sv.notifyDebounceTimer = nil	248		sv.notifyDebounceTimer = nil
230		}	249		}
231	-	if sv.tickCallbackID != 0 {	250	+	tickCallbackID := sv.tickCallbackID
232	-	sv.paned.RemoveTickCallback(sv.tickCallbackID)	251	+	sv.tickCallbackID = 0
233	-	sv.tickCallbackID = 0	252	+	sv.mu.Unlock()
			253	+
			254	+	if tickCallbackID != 0 {
			255	+	sv.paned.RemoveTickCallback(tickCallbackID)
234		}	256		}
235		}	257		}
236			258

A internal/ui/layout/split_notify_cleanup_test.go +123 -0

@@ -0,0 +1,123 @@
	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 setIdleScheduler(sv SplitView, queued chan<- glib.SourceFunc) {
	64	+	sv.idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
	65	+	queued <- source
	66	+	return 1
	67	+	}
	68	+	}
	69	+
	70	+	func TestSplitViewQueuedRatioCallbackDoesNotRunAfterCleanup(t *testing.T) {
	71	+	queued := make(chan *glib.SourceFunc, 1)
	72	+	sv, notify := newNotifyingSplitView(t)
	73	+	setIdleScheduler(sv, queued)
	74	+	var calls atomic.Int32
	75	+	sv.SetOnRatioChanged(func(float64) { calls.Add(1) })
	76	+
	77	+	notify(25)
	78	+	source := receiveQueuedIdle(t, queued) // Debounce has fired and queued this work.
	79	+	sv.Cleanup()
	80	+	(*source)(0) // Run the queued GLib closure after cleanup.
	81	+
	82	+	require.Zero(t, calls.Load(), "cleanup must invalidate queued ratio callbacks")
	83	+	}
	84	+
	85	+	func TestSplitViewSupersededRatioCallbackDeliversOnlyLatestValueOnce(t *testing.T) {
	86	+	queued := make(chan *glib.SourceFunc, 2)
	87	+	sv, notify := newNotifyingSplitView(t)
	88	+	setIdleScheduler(sv, queued)
	89	+	var ratios []float64
	90	+	sv.SetOnRatioChanged(func(ratio float64) { ratios = append(ratios, ratio) })
	91	+
	92	+	notify(20)
	93	+	first := receiveQueuedIdle(t, queued)
	94	+	notify(80)
	95	+	second := receiveQueuedIdle(t, queued)
	96	+	(*first)(0)
	97	+	(*second)(0)
	98	+
	99	+	require.Equal(t, []float64{0.8}, ratios)
	100	+	}
	101	+
	102	+	func TestSplitViewIdleSchedulersAreInstanceScoped(t *testing.T) {
	103	+	firstQueued := make(chan *glib.SourceFunc, 1)
	104	+	secondQueued := make(chan *glib.SourceFunc, 1)
	105	+	first, firstNotify := newNotifyingSplitView(t)
	106	+	second, secondNotify := newNotifyingSplitView(t)
	107	+	setIdleScheduler(first, firstQueued)
	108	+	setIdleScheduler(second, secondQueued)
	109	+
	110	+	firstNotify(20)
	111	+	secondNotify(80)
	112	+
	113	+	firstSource := receiveQueuedIdle(t, firstQueued)
	114	+	secondSource := receiveQueuedIdle(t, secondQueued)
	115	+	var firstCalls, secondCalls atomic.Int32
	116	+	first.SetOnRatioChanged(func(float64) { firstCalls.Add(1) })
	117	+	second.SetOnRatioChanged(func(float64) { secondCalls.Add(1) })
	118	+	(*firstSource)(0)
	119	+	(*secondSource)(0)
	120	+
	121	+	require.Equal(t, int32(1), firstCalls.Load())
	122	+	require.Equal(t, int32(1), secondCalls.Load())
	123	+	}

@@ -0,0 +1,123 @@
	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 setIdleScheduler(sv SplitView, queued chan<- glib.SourceFunc) {
	64	+	sv.idleAdd = func(source *glib.SourceFunc, _ uintptr) uint {
	65	+	queued <- source
	66	+	return 1
	67	+	}
	68	+	}
	69	+
	70	+	func TestSplitViewQueuedRatioCallbackDoesNotRunAfterCleanup(t *testing.T) {
	71	+	queued := make(chan *glib.SourceFunc, 1)
	72	+	sv, notify := newNotifyingSplitView(t)
	73	+	setIdleScheduler(sv, queued)
	74	+	var calls atomic.Int32
	75	+	sv.SetOnRatioChanged(func(float64) { calls.Add(1) })
	76	+
	77	+	notify(25)
	78	+	source := receiveQueuedIdle(t, queued) // Debounce has fired and queued this work.
	79	+	sv.Cleanup()
	80	+	(*source)(0) // Run the queued GLib closure after cleanup.
	81	+
	82	+	require.Zero(t, calls.Load(), "cleanup must invalidate queued ratio callbacks")
	83	+	}
	84	+
	85	+	func TestSplitViewSupersededRatioCallbackDeliversOnlyLatestValueOnce(t *testing.T) {
	86	+	queued := make(chan *glib.SourceFunc, 2)
	87	+	sv, notify := newNotifyingSplitView(t)
	88	+	setIdleScheduler(sv, queued)
	89	+	var ratios []float64
	90	+	sv.SetOnRatioChanged(func(ratio float64) { ratios = append(ratios, ratio) })
	91	+
	92	+	notify(20)
	93	+	first := receiveQueuedIdle(t, queued)
	94	+	notify(80)
	95	+	second := receiveQueuedIdle(t, queued)
	96	+	(*first)(0)
	97	+	(*second)(0)
	98	+
	99	+	require.Equal(t, []float64{0.8}, ratios)
	100	+	}
	101	+
	102	+	func TestSplitViewIdleSchedulersAreInstanceScoped(t *testing.T) {
	103	+	firstQueued := make(chan *glib.SourceFunc, 1)
	104	+	secondQueued := make(chan *glib.SourceFunc, 1)
	105	+	first, firstNotify := newNotifyingSplitView(t)
	106	+	second, secondNotify := newNotifyingSplitView(t)
	107	+	setIdleScheduler(first, firstQueued)
	108	+	setIdleScheduler(second, secondQueued)
	109	+
	110	+	firstNotify(20)
	111	+	secondNotify(80)
	112	+
	113	+	firstSource := receiveQueuedIdle(t, firstQueued)
	114	+	secondSource := receiveQueuedIdle(t, secondQueued)
	115	+	var firstCalls, secondCalls atomic.Int32
	116	+	first.SetOnRatioChanged(func(float64) { firstCalls.Add(1) })
	117	+	second.SetOnRatioChanged(func(float64) { secondCalls.Add(1) })
	118	+	(*firstSource)(0)
	119	+	(*secondSource)(0)
	120	+
	121	+	require.Equal(t, int32(1), firstCalls.Load())
	122	+	require.Equal(t, int32(1), secondCalls.Load())
	123	+	}