diff --git a/appearance.go b/appearance.go new file mode 100644 index 0000000..790846c --- /dev/null +++ b/appearance.go @@ -0,0 +1,32 @@ +// Copyright (c) 2026, the go-widgets authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file. + +package toolkit + +import "sync" + +// appearanceMu guards the toolkit's global appearance: the metric scale, the +// touch density, the active font and the OpenType size a host last asked for. +// +// A host SETS these; every widget READS them, on every metric it lays out. One +// goroutine calling SetMetricScale while another builds a widget is therefore a +// plain data race — found by -race from an application whose tests set the +// scale in one test while another was constructing a list. In the wild it is +// not a test artefact: moving a window between displays is exactly this, the +// scale set from whatever thread the display-change event arrives on while the +// UI is drawing at the old one. What it costs is not a crash but silence — +// chrome laid out at one scale around type measured at another. +// +// It is deliberately ONE lock for the four of them. They are one fact, "how big +// is everything"; SetMetricScale re-renders the font, so the scale and the font +// change together; and two locks taken in two orders is how a toolkit deadlocks +// on a display change. +// +// THE INVARIANT, for anyone editing these files: no call holds this lock across +// another call that takes it. SetMetricScale drops it before rescaleText, which +// installs a face through SetFont, which takes it again. sync.RWMutex is not +// reentrant, and RLock is not safely reentrant either — a writer arriving +// between two nested RLocks deadlocks both. Read what you need, unlock, then +// compute. +var appearanceMu sync.RWMutex diff --git a/appearance_test.go b/appearance_test.go new file mode 100644 index 0000000..9c73439 --- /dev/null +++ b/appearance_test.go @@ -0,0 +1,79 @@ +// Copyright (c) 2026, the go-widgets authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file. + +package toolkit + +import ( + "sync" + "testing" +) + +// TestAppearanceSurvivesConcurrentUse covers the toolkit's global appearance +// being set on one goroutine while widgets read it on another. +// +// That is not a contrived pairing: it is a window moving between displays. The +// scale is set from whatever thread the display-change event arrives on, and +// the UI is laying out at the old one. Unguarded it is a data race over four +// globals at once — the scale, the density, the active font and the OpenType +// size — and what a race costs here is not a crash but silence: chrome laid out +// at one scale around type measured at another. +// +// Run with -race, this is the whole test. Without it, it still exercises that +// no ordering deadlocks: SetMetricScale re-renders the font through SetFont, +// which takes the same lock. +func TestAppearanceSurvivesConcurrentUse(t *testing.T) { + scale, dens, font := MetricScale(), Density(), CurrentFont() + t.Cleanup(func() { + SetFont(nil) + SetMetricScale(scale) + SetDensity(dens) + if font != nil { + SetFont(font) + } + }) + + const rounds = 200 + var wg sync.WaitGroup + writers := []func(int){ + func(i int) { SetMetricScale(1 + float64(i%3)) }, + func(i int) { SetDensity(DensityLevel(i % 3)) }, + func(int) { SetFont(nil) }, + } + readers := []func(){ + func() { _ = scaled(10) }, + func() { _ = dpiScaled(10) }, + func() { _ = MetricScale() }, + func() { _ = Density() }, + func() { _ = CurrentFont() }, + func() { _ = GlyphHeight() }, + } + for _, w := range writers { + wg.Add(1) + go func(w func(int)) { + defer wg.Done() + for i := range rounds { + w(i) + } + }(w) + } + for _, r := range readers { + wg.Add(1) + go func(r func()) { + defer wg.Done() + for range rounds { + r() + } + }(r) + } + wg.Wait() + + // The state is still coherent: whatever the last writer left, a reader + // gets a usable answer rather than a torn one. + if got := scaled(10); got <= 0 { + t.Errorf("scaled(10) = %d after concurrent use, want a positive metric", got) + } + if CurrentFont() == nil { + t.Error("there is no active font after concurrent use") + } +} diff --git a/defaulttext.go b/defaulttext.go index fc965b9..0c44319 100644 --- a/defaulttext.go +++ b/defaulttext.go @@ -89,10 +89,13 @@ var openTypeLogicalPx int // its text because a resize could not re-render it would be worse than one // whose text is briefly the wrong size. func rescaleText() { - if openTypeLogicalPx <= 0 { + appearanceMu.RLock() + logical, scale := openTypeLogicalPx, metricScale + appearanceMu.RUnlock() + if logical <= 0 { return } - px := int(float64(openTypeLogicalPx)*metricScale + 0.5) + px := int(float64(logical)*scale + 0.5) if px < 1 { px = 1 } @@ -110,7 +113,10 @@ func UseOpenTypeTextSize(sizePx int) error { // rendered at sizePx x MetricScale, and re-rendered whenever that scale // changes. A caller asking for 16 gets type that reads the same size on // every display, which is the whole point of a scale. - px := int(float64(sizePx)*metricScale + 0.5) + appearanceMu.RLock() + scale := metricScale + appearanceMu.RUnlock() + px := int(float64(sizePx)*scale + 0.5) if px < 1 { px = 1 } @@ -118,7 +124,12 @@ func UseOpenTypeTextSize(sizePx int) error { if err != nil { return err } + // SetFont takes the lock, so it is called without it held; the size is + // recorded after, exactly as before, because SetFont(nil) is the one that + // clears it and f is not nil here. SetFont(f) + appearanceMu.Lock() openTypeLogicalPx = sizePx + appearanceMu.Unlock() return nil } diff --git a/density.go b/density.go index f387556..a88622f 100644 --- a/density.go +++ b/density.go @@ -44,14 +44,21 @@ var density = DensityCompact // the density never lands on an undefined level (mirroring how [SetMetricScale] // ignores a non-positive scale). func SetDensity(d DensityLevel) { - if d >= DensityCompact && d <= DensityTouch { - density = d + if d < DensityCompact || d > DensityTouch { + return } + appearanceMu.Lock() + density = d + appearanceMu.Unlock() } // Density returns the current global touch profile ([DensityCompact] by // default). -func Density() DensityLevel { return density } +func Density() DensityLevel { + appearanceMu.RLock() + defer appearanceMu.RUnlock() + return density +} // densityFactor is the spacing multiplier a level applies to every base metric // on top of [MetricScale]. Compact is exactly 1.0 (byte-identical default); diff --git a/font.go b/font.go index f08141c..f6b0ace 100644 --- a/font.go +++ b/font.go @@ -82,6 +82,8 @@ var activeFont Font // whatever the current [MetricScale] is. All subsequent layout (GlyphHeight / // GlyphAdvance) and DrawText use it. func SetFont(f Font) { + appearanceMu.Lock() + defer appearanceMu.Unlock() activeFont = f if f == nil { // Back to the bitmap means back to the bitmap: the OpenType size a @@ -100,22 +102,30 @@ func SetFont(f Font) { // font chose its size too, so that one is left alone: the same rule Menu and // Browser follow for their own Scale fields. func CurrentFont() Font { - if activeFont != nil { - return activeFont + appearanceMu.RLock() + f, scale := activeFont, metricScale + appearanceMu.RUnlock() + if f != nil { + return f } - return scaledDefaultFont() + return scaledDefaultFont(scale) } // scaledDefaultFont is the built-in bitmap at the current metric scale, cached // so the common path allocates nothing. -func scaledDefaultFont() Font { - n := int(MetricScale() + 0.5) +func scaledDefaultFont(scale float64) Font { + n := int(scale + 0.5) if n < 1 { n = 1 } if n == 1 { return defaultFont } + // The scale is passed in rather than read again: the caller already holds + // it, and taking the read lock a second time inside one call is the nested + // RLock the invariant in appearance.go forbids. + appearanceMu.Lock() + defer appearanceMu.Unlock() if cachedScaledFont == nil || cachedScaledFont.Scale != n { cachedScaledFont = &bitmapFont{Scale: n} } diff --git a/metrics.go b/metrics.go index c6ca5ca..cf35802 100644 --- a/metrics.go +++ b/metrics.go @@ -27,10 +27,15 @@ func SetMetricScale(f float64) { if f <= 0 { return } - if f == metricScale { + appearanceMu.Lock() + changed := f != metricScale + if changed { + metricScale = f + } + appearanceMu.Unlock() + if !changed { return } - metricScale = f // The text follows. Every other metric here is multiplied by the scale and // the one thing a person actually reads was not, so an application on a // HiDPI display got labels at half the size it asked for and had to @@ -40,7 +45,11 @@ func SetMetricScale(f float64) { } // MetricScale returns the current global metric scale (1.0 by default). -func MetricScale() float64 { return metricScale } +func MetricScale() float64 { + appearanceMu.RLock() + defer appearanceMu.RUnlock() + return metricScale +} // scaled rounds a base (logical-pixel) metric to device pixels at the current // HiDPI scale AND touch density. It is the single seam every widget's pixel @@ -53,13 +62,23 @@ func MetricScale() float64 { return metricScale } // Under the default [DensityCompact] the factor is exactly 1.0, so this reduces // to the pure HiDPI form and every metric is byte-identical to a density-less // toolkit. -func scaled(v int) int { return int(float64(v)*metricScale*densityFactor(density) + 0.5) } +func scaled(v int) int { + appearanceMu.RLock() + s, d := metricScale, density + appearanceMu.RUnlock() + return int(float64(v)*s*densityFactor(d) + 0.5) +} // dpiScaled rounds a base (logical-pixel) length to device pixels at the current // HiDPI [MetricScale] ONLY — it does NOT apply the touch [Density] factor. It // backs [MinHitTarget], whose floor is an absolute reachability guarantee in // logical pixels that must grow with panel DPI but not with spacing density. -func dpiScaled(v int) int { return int(float64(v)*metricScale + 0.5) } +func dpiScaled(v int) int { + appearanceMu.RLock() + s := metricScale + appearanceMu.RUnlock() + return int(float64(v)*s + 0.5) +} // Scaled is the exported form of [scaled]: it rounds a base (logical-pixel) // metric to device pixels at the current [MetricScale]. Sibling packages that