From 7a9ae36f12b9017125028ece1728273fdd499ba6 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 18 Aug 2026 21:45:00 +0200 Subject: [PATCH] fix(tkbind): follow toolkit's RangeSlider to MVVM-only, via a new BindTwoWay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mvvm no longer compiles against toolkit at HEAD. toolkit v0.202.0 made RangeSlider.Low/High Observables rather than settable fields and dropped OnChange, so tkbind fails to build with seven errors, and any application depending on both mvvm and a current toolkit fails with it. The fix is not a patch at the call site but the primitive the new shape asks for. BindField's contract is "a value field plus a callback slot", which a widget owning an Observable has neither of; what it has is a second property that must agree with the ViewModel's. That is a symmetric link, so the core package gains BindTwoWay — generic, backend-free, and useful to every widget toolkit's MVVM-only sweep converts next, not to RangeSlider alone. BindRange becomes two BindTwoWay links, one per handle. One subtlety is pinned by its own test: Low().Set and High().Set do NOT clamp (only SetRange and the drag/key paths do), so seeding a slider from a ViewModel holding an out-of-range or inverted band would leave it illegal. SetRange is therefore called AFTER the links exist, letting the widget's own invariant travel back to the observables — both sides end up holding the same legal band. Loop-freedom is asserted by COUNTING notifications rather than comparing values: values agreeing proves nothing about how many round trips it took to agree. 100.0% statement coverage, race-clean. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 4 +++ bind.go | 38 +++++++++++++++++++++ bind_test.go | 71 +++++++++++++++++++++++++++++++++++++++ go.mod | 13 +++++--- go.sum | 26 +++++++++------ tkbind/tkbind.go | 51 +++++++++++----------------- tkbind/tkbind_test.go | 77 +++++++++++++++++++++++++++---------------- 7 files changed, 205 insertions(+), 75 deletions(-) diff --git a/README.md b/README.md index c089b0a..cf51aee 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,10 @@ terminal-cell [`tui`](https://github.com/go-widgets/tui). Binding adapters reach a widget only through pointers to its value field and its callback slot — `BindField`, `OneWay`, `BindCommand`, `BindList`. +A widget may instead expose its state AS an `Observable`, which go-widgets/toolkit +is moving its widgets to. There is then no field to seed and no hook to compose, +just two properties that must agree, so the adapter is symmetric: `BindTwoWay`. + ## Why it's backend-agnostic `go-widgets/toolkit` and `go-widgets/tui` mirror the same field names and diff --git a/bind.go b/bind.go index b04fddd..b806fd9 100644 --- a/bind.go +++ b/bind.go @@ -102,3 +102,41 @@ func BindList[T any](l *ObservableList[T], items *[]string, project func(T) stri rebuild() return l.Subscribe(func(ListEvent[T]) { rebuild() }) } + +// BindTwoWay links two Observables of the same type so that a change to either +// is reflected in the other, and returns an unbind detaching both directions. +// +// It is the adapter for widgets that expose their state AS an Observable rather +// than as a value field plus a callback slot — the shape BindField takes. A +// widget that owns an Observable has no field to seed and no hook to compose; +// there are simply two properties that must agree, and the binding is symmetric +// where BindField's is not. +// +// src is the source of truth at bind time: dst is seeded from it, matching +// BindField's rule that the ViewModel wins over whatever the widget was +// constructed with. Afterwards neither side is privileged. +// +// It is loop-free by the same mechanism as the rest of this package: Set skips +// values equal to the current one, so the echo stops at the first hop. That +// mechanism is the change test, so — as NewObservableEq documents — a two-way +// binding over an Observable built with a nil eq would never settle. Give +// anything bound this way a real equality function. +func BindTwoWay[T any](src, dst *Observable[T], invalidate func()) (unbind func()) { + dst.Set(src.Get()) + us := src.Subscribe(func(v T) { + dst.Set(v) + if invalidate != nil { + invalidate() + } + }) + ud := dst.Subscribe(func(v T) { + src.Set(v) + if invalidate != nil { + invalidate() + } + }) + return func() { + us() + ud() + } +} diff --git a/bind_test.go b/bind_test.go index df317f8..29ffc3d 100644 --- a/bind_test.go +++ b/bind_test.go @@ -191,3 +191,74 @@ func TestBindListRebuildsAndProjects(t *testing.T) { t.Fatalf("nil-invalidate list = %v", it2) } } + +func TestBindTwoWaySeedsFromSourceAndLinksBothWays(t *testing.T) { + src := NewObservable(7) + dst := NewObservable(0) + repaints := 0 + unbind := BindTwoWay(src, dst, func() { repaints++ }) + + if dst.Get() != 7 { + t.Fatalf("seed: dst=%d, want 7 (src is the source of truth)", dst.Get()) + } + src.Set(9) + if dst.Get() != 9 { + t.Fatalf("src→dst: dst=%d, want 9", dst.Get()) + } + dst.Set(11) + if src.Get() != 11 { + t.Fatalf("dst→src: src=%d, want 11", src.Get()) + } + if repaints == 0 { + t.Fatal("changes should have requested repaints") + } + + unbind() + src.Set(1) + if dst.Get() != 11 { + t.Fatalf("after unbind src→dst still live: dst=%d", dst.Get()) + } + dst.Set(2) + if src.Get() != 1 { + t.Fatalf("after unbind dst→src still live: src=%d", src.Get()) + } +} + +// TestBindTwoWayIsLoopFree pins the property the whole package rests on: the +// echo must stop at the first hop rather than ping-pong. Counting notifications +// is what makes that visible — asserting the values agree would pass even if +// they had agreed after a thousand round trips. +func TestBindTwoWayIsLoopFree(t *testing.T) { + src := NewObservable(0) + dst := NewObservable(0) + defer BindTwoWay(src, dst, nil)() + + srcNotes, dstNotes := 0, 0 + defer src.Subscribe(func(int) { srcNotes++ })() + defer dst.Subscribe(func(int) { dstNotes++ })() + + src.Set(5) + if srcNotes != 1 || dstNotes != 1 { + t.Fatalf("one Set should notify each side once: src=%d dst=%d", srcNotes, dstNotes) + } + dst.Set(6) + if srcNotes != 2 || dstNotes != 2 { + t.Fatalf("the reverse Set should also settle at one: src=%d dst=%d", srcNotes, dstNotes) + } +} + +// TestBindTwoWayNilInvalidate covers the nil-invalidate branch on both +// directions. +func TestBindTwoWayNilInvalidate(t *testing.T) { + src := NewObservable("a") + dst := NewObservable("z") + defer BindTwoWay(src, dst, nil)() + src.Set("b") + if dst.Get() != "b" { + t.Fatalf("src→dst with nil invalidate: dst=%q", dst.Get()) + } + dst.Set("c") + if src.Get() != "c" { + t.Fatalf("dst→src with nil invalidate: src=%q", src.Get()) + } +} diff --git a/go.mod b/go.mod index 962b0c7..3df0eb8 100644 --- a/go.mod +++ b/go.mod @@ -4,17 +4,20 @@ go 1.26.4 require ( github.com/go-widgets/data v0.1.0 - github.com/go-widgets/toolkit v0.150.0 + github.com/go-widgets/toolkit v0.202.0 github.com/go-widgets/tui v0.49.0 ) require ( + github.com/go-gfx/gfx v0.6.0 // indirect + github.com/go-iconoir/iconoir v0.2.0 // indirect github.com/go-images/images v0.0.0-20260811115337-bc5d586f8e38 // indirect - github.com/go-opentype/bidi v0.2.1 // indirect github.com/go-opentype/fonts v0.6.0 // indirect github.com/go-opentype/opentype v0.5.0 // indirect - github.com/go-opentype/shape v0.4.0 // indirect - github.com/go-widgets/painter v0.9.0 // indirect - golang.org/x/sys v0.46.0 // indirect + github.com/go-opentype/shape v0.5.0 // indirect + github.com/go-typeset/bidi v0.3.0 // indirect + github.com/go-widgets/painter v0.11.0 // indirect + golang.org/x/image v0.45.0 // indirect + golang.org/x/sys v0.47.0 // indirect golang.org/x/term v0.44.0 // indirect ) diff --git a/go.sum b/go.sum index a7fd037..92137f5 100644 --- a/go.sum +++ b/go.sum @@ -1,22 +1,28 @@ +github.com/go-gfx/gfx v0.6.0 h1:sQkGEmB9YLouHxrscwvS4OW6oELsTtrKtQqsNdJ8L8Q= +github.com/go-gfx/gfx v0.6.0/go.mod h1:bFt/MWyYWRU3Ic9IaB8XOC9KLMMHRRmahMk4FaIGK7g= +github.com/go-iconoir/iconoir v0.2.0 h1:2ANqG6gkvHMoCtpNgNhSeOsuGMnlWQ+Sl43EndujIC0= +github.com/go-iconoir/iconoir v0.2.0/go.mod h1:BrOQ68YO5BMsF7y+/nht4shbgjVfh9CxBoQF35eNosw= github.com/go-images/images v0.0.0-20260811115337-bc5d586f8e38 h1:p+DjIujiwUvBiyD0oS9SUatf3pMKo0GpcA4p/Kyv7d8= github.com/go-images/images v0.0.0-20260811115337-bc5d586f8e38/go.mod h1:nWwQCf77xG1GhaSD3rvlp7FNjjDhz47vrztyeBvIerk= -github.com/go-opentype/bidi v0.2.1 h1:7xCGqywPYxmwSfPBgP1Ewrp2Iwg2S03NeKxGWTz2guo= -github.com/go-opentype/bidi v0.2.1/go.mod h1:PaWZ9P93jwQaXsEvAoSxvJ3P70S9f8TuFGDstTvcfmM= github.com/go-opentype/fonts v0.6.0 h1:pUKITjwC/gZg+XNCqP5eGLi8RHu2RAIcDrr6yp1VKMU= github.com/go-opentype/fonts v0.6.0/go.mod h1:C6yQL2apHItfEZ5hztpsHF0S5mlX/hklLlq/Z5fRG/g= github.com/go-opentype/opentype v0.5.0 h1:++VoqgbXgYACyTTNzUAivoxW9M3m2gxWMAn6bIY7DXg= github.com/go-opentype/opentype v0.5.0/go.mod h1:AOixevJf7XQaH7+WG+OMIOZEbYPXfMqklVk26Y6YTUU= -github.com/go-opentype/shape v0.4.0 h1:Yz8ooo9+7HlzuzS72YWjJVM22GNGFf6jcxWfNNJOsng= -github.com/go-opentype/shape v0.4.0/go.mod h1:7kybm69yrtRgQnjaxPyYI9nPRFFdBdzlrmjcYCBfCbg= +github.com/go-opentype/shape v0.5.0 h1:jHNaOMHNBdDj5EixOevlrrsi92svMxvVMKN7GYaPfxo= +github.com/go-opentype/shape v0.5.0/go.mod h1:3ImRYNIj6zpwWQ/DV3BWhgMFfmzDITH6XpZw2auQHTo= +github.com/go-typeset/bidi v0.3.0 h1:4fjGjejvjE2LzLNzY4si8PkVO321NcsKIiANhWT3jF4= +github.com/go-typeset/bidi v0.3.0/go.mod h1:ct3cmYT8Qt1FGJQ+2QaakrUxqaP4ZysgdQQg2ym5+Xo= github.com/go-widgets/data v0.1.0 h1:PB9EZtd3ASBeVx6XxgCaFr1YU8bgrmNkYULyyvyPkvU= github.com/go-widgets/data v0.1.0/go.mod h1:OUFAK+pXaIt6Hc7wE49iMNykmcXEWICZtycbDiqMLFc= -github.com/go-widgets/painter v0.9.0 h1:/y0qn0+TP3bhdLMi4+/vmUUBnSp3/L2RQBBgwAmJkes= -github.com/go-widgets/painter v0.9.0/go.mod h1:ccmlkH2UmcXQh6rt9Fu2eDt2RI0B5V0POaGmUKeW0EQ= -github.com/go-widgets/toolkit v0.150.0 h1:tBkGdsO6fYotdWpJ7gaOuj9BEf0xUdktPFH8gie6oOY= -github.com/go-widgets/toolkit v0.150.0/go.mod h1:UZ8tGcFgtmcIZuZPmX1fPncrni7IOG+E0AaeMi0MXcI= +github.com/go-widgets/painter v0.11.0 h1:xsj4zTz8B43rOZnWrx7ZsaUBMgJyvgaE/Pq2FfcG2Sw= +github.com/go-widgets/painter v0.11.0/go.mod h1:IPRLqdUJuJX8sfuHeYLZCzjoLvA0ApbOlyIAVmguJDQ= +github.com/go-widgets/toolkit v0.202.0 h1:SIqmvHCVoihatITY6Jc8sL3ZQJ6gIOxAqHB5C9n2inA= +github.com/go-widgets/toolkit v0.202.0/go.mod h1:pNroe6a4TiuFC//n6kbsikgs/actqsvqN88vUxjwESg= github.com/go-widgets/tui v0.49.0 h1:yFzxU3jsaE++mCrs4PY0ZzAgAe7RQ41F21bXXSVmzDA= github.com/go-widgets/tui v0.49.0/go.mod h1:n+8knhgRZJn1nXAucKetFmZySH6JvTYnKWSW2x1Jx3E= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/image v0.45.0 h1:FMb1nTbH5H9vF55SriQHgFw5GnNL9Jg6L25BwXKzhB0= +golang.org/x/image v0.45.0/go.mod h1:n62x/7RqlwXDvGsSU4u6IUTUf6KghUZ9Bt7cG/T9Fx4= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= diff --git a/tkbind/tkbind.go b/tkbind/tkbind.go index 4c92074..91ed9a1 100644 --- a/tkbind/tkbind.go +++ b/tkbind/tkbind.go @@ -5,8 +5,8 @@ // Package tkbind holds the MVVM binding adapters that are specific to the pixel // toolkit (github.com/go-widgets/toolkit) — the widgets whose value/callback // shape the generic mvvm adapters can't express, such as a two-handle range -// slider (a multi-argument OnChange). It is the ONLY MVVM package that imports -// toolkit; the core mvvm package stays backend-free. +// slider. It is the ONLY MVVM package that imports toolkit; the core mvvm +// package stays backend-free. package tkbind import ( @@ -14,37 +14,26 @@ import ( "github.com/go-widgets/toolkit" ) -// BindRange two-way-binds a RangeSlider's Low/High to a pair of observables. -// The slider fires OnChange(low, high) on every drag — a shape the generic -// single-value BindField can't take — so this adapter fans it out to both -// observables and pushes each back to the matching field. Loop-free: the field -// writes are silent and Observable.Set skips equal values. Returns an unbind -// that restores the prior OnChange and detaches both subscriptions. +// BindRange two-way-binds a RangeSlider's band to a pair of observables. +// +// The slider now OWNS its band as two mvvm.Observables rather than as settable +// fields plus a multi-argument OnChange, so this is two symmetric links rather +// than a callback fan-out — mvvm.BindTwoWay per handle. +// +// The band is normalised THROUGH the widget after the links exist, not before: +// Low().Set and High().Set do not clamp (only SetRange and the drag/key paths +// do), so a ViewModel holding an out-of-range or inverted band would otherwise +// seed an illegal slider. Calling SetRange once the links are live lets the +// widget's own invariant — clamped to [Min, Max], Low <= High — travel back to +// the observables, leaving both sides holding the same legal band. +// +// Loop-free by Observable.Set's equality check, as everywhere in this package. +// The returned unbind detaches all four subscriptions. func BindRange(low, high *mvvm.Observable[float64], rs *toolkit.RangeSlider, invalidate func()) (unbind func()) { - rs.Low = low.Get() - rs.High = high.Get() - prev := rs.OnChange - rs.OnChange = func(lo, hi float64) { - if prev != nil { - prev(lo, hi) - } - low.Set(lo) - high.Set(hi) - } - ul := low.Subscribe(func(v float64) { - rs.Low = v - if invalidate != nil { - invalidate() - } - }) - uh := high.Subscribe(func(v float64) { - rs.High = v - if invalidate != nil { - invalidate() - } - }) + ul := mvvm.BindTwoWay(low, rs.Low(), invalidate) + uh := mvvm.BindTwoWay(high, rs.High(), invalidate) + rs.SetRange(low.Get(), high.Get()) return func() { - rs.OnChange = prev ul() uh() } diff --git a/tkbind/tkbind_test.go b/tkbind/tkbind_test.go index 74f0d81..c1f0d04 100644 --- a/tkbind/tkbind_test.go +++ b/tkbind/tkbind_test.go @@ -15,55 +15,74 @@ func TestBindRangeTwoWay(t *testing.T) { low := mvvm.NewObservable(10.0) high := mvvm.NewObservable(90.0) rs := toolkit.NewRangeSlider(0, 100, 0, 0) - priorCalls := 0 - rs.OnChange = func(lo, hi float64) { priorCalls++ } // pre-existing handler repaints := 0 unbind := BindRange(low, high, rs, func() { repaints++ }) - // Seeded from the observables. - if rs.Low != 10 || rs.High != 90 { - t.Fatalf("seed: Low=%v High=%v, want 10/90", rs.Low, rs.High) + // Seeded from the observables: the ViewModel wins over the band the slider + // was constructed with. + if rs.Low().Get() != 10 || rs.High().Get() != 90 { + t.Fatalf("seed: Low=%v High=%v, want 10/90", rs.Low().Get(), rs.High().Get()) } - // View→VM: a drag fires OnChange(lo,hi) → both observables update, and the - // prior handler still runs. - rs.OnChange(20, 80) - if low.Get() != 20 || high.Get() != 80 || priorCalls != 1 { - t.Fatalf("view→vm: low=%v high=%v prior=%d", low.Get(), high.Get(), priorCalls) + // View→VM: the widget Sets its own observables on a drag or key press. + rs.Low().Set(20) + rs.High().Set(80) + if low.Get() != 20 || high.Get() != 80 { + t.Fatalf("view→vm: low=%v high=%v, want 20/80", low.Get(), high.Get()) } - // VM→View: setting an observable pushes to the matching field. + // VM→View. low.Set(5) high.Set(95) - if rs.Low != 5 || rs.High != 95 { - t.Fatalf("vm→view: Low=%v High=%v, want 5/95", rs.Low, rs.High) + if rs.Low().Get() != 5 || rs.High().Get() != 95 { + t.Fatalf("vm→view: Low=%v High=%v, want 5/95", rs.Low().Get(), rs.High().Get()) } if repaints == 0 { t.Fatal("VM→View pushes should have requested repaints") } - // Unbind restores the prior handler and detaches. + // Unbind detaches BOTH directions. unbind() low.Set(0) - if rs.Low != 5 { - t.Fatalf("after unbind Low changed to %v", rs.Low) + if rs.Low().Get() != 5 { + t.Fatalf("after unbind VM→View still live: Low=%v", rs.Low().Get()) } - rs.OnChange(1, 2) // only the prior handler now - if priorCalls != 2 || low.Get() != 0 && high.Get() != 95 { - t.Fatalf("after unbind: prior=%d low=%v high=%v", priorCalls, low.Get(), high.Get()) + rs.High().Set(42) + if high.Get() != 95 { + t.Fatalf("after unbind View→VM still live: high=%v", high.Get()) } } -func TestBindRangeNilPriorAndInvalidate(t *testing.T) { - // Covers the prev==nil and invalidate==nil branches. +// TestBindRangeNormalisesThroughTheWidget pins the reason SetRange is called +// after the links exist rather than before: Low().Set and High().Set do not +// clamp, so an out-of-range, inverted ViewModel band must be corrected by the +// widget and the correction must travel BACK, leaving both sides equal and +// legal. +func TestBindRangeNormalisesThroughTheWidget(t *testing.T) { + low := mvvm.NewObservable(140.0) // above Max, and above high + high := mvvm.NewObservable(-20.0) + rs := toolkit.NewRangeSlider(0, 100, 10, 90) + defer BindRange(low, high, rs, nil)() + + if rs.Low().Get() != 0 || rs.High().Get() != 100 { + t.Fatalf("widget band not normalised: Low=%v High=%v, want 0/100", + rs.Low().Get(), rs.High().Get()) + } + if low.Get() != 0 || high.Get() != 100 { + t.Fatalf("correction did not travel back: low=%v high=%v, want 0/100", + low.Get(), high.Get()) + } +} + +// TestBindRangeNilInvalidate covers the nil-invalidate branch. +func TestBindRangeNilInvalidate(t *testing.T) { low := mvvm.NewObservable(0.0) high := mvvm.NewObservable(100.0) rs := toolkit.NewRangeSlider(0, 100, 50, 50) - unbind := BindRange(low, high, rs, nil) - defer unbind() - rs.OnChange(10, 90) // prev is nil — must not panic - if low.Get() != 10 || high.Get() != 90 { - t.Fatalf("nil-prior view→vm: low=%v high=%v", low.Get(), high.Get()) - } + defer BindRange(low, high, rs, nil)() low.Set(25) // nil invalidate — must not panic - if rs.Low != 25 { - t.Fatalf("nil-invalidate push: Low=%v", rs.Low) + if rs.Low().Get() != 25 { + t.Fatalf("nil-invalidate push: Low=%v", rs.Low().Get()) + } + rs.High().Set(75) + if high.Get() != 75 { + t.Fatalf("nil-invalidate view→vm: high=%v", high.Get()) } }