Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
modifier-hold) with its width reserved so the tab never resizes. Pinned terminal (and
other non-browser) tabs keep their titled layout.

### Fixed
- Split-button icons no longer re-resolve their SF Symbol on every tab bar body
evaluation. `TabBarStyling.splitActionSystemImage(for:)` tested whether a name
was a real symbol by allocating an `NSImage` and discarding it, which cost
~27µs per call and ran once per button per evaluation. Results are now
memoized by name, which the symbol catalog makes safe: the answer cannot
change while the process runs.

## [1.1.1] - 2025-01-29

### Fixed
Expand Down
63 changes: 63 additions & 0 deletions Sources/Bonsplit/Internal/Views/TabBarView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,23 @@ enum TabBarStyling {
)
}

/// Resolves a configured split-button icon name to the glyph the tab bar
/// should draw.
///
/// Memoized, because `splitActionButtonIcon` calls this during every tab
/// bar body evaluation, once per split button, and resolving is far more
/// expensive than it looks. See ``resolveSplitActionSystemImage(for:)``.
static func splitActionSystemImage(for name: String) -> SplitActionSystemImage {
SplitActionSystemImageCache.shared.image(for: name)
}

/// The uncached resolution.
///
/// The only way to ask AppKit whether a name is a real SF Symbol is to load
/// the symbol image and throw it away, which walks the symbol catalog and
/// builds a representation. The answer cannot change while the process
/// runs, so it is worth remembering.
static func resolveSplitActionSystemImage(for name: String) -> SplitActionSystemImage {
if NSImage(systemSymbolName: name, accessibilityDescription: nil) != nil {
return SplitActionSystemImage(name: name, rotationDegrees: 0, pointSize: 12)
}
Expand Down Expand Up @@ -1876,6 +1892,53 @@ private struct TabBarLayerBackedColor: NSViewRepresentable {
}
}

/// Remembers ``TabBarStyling/splitActionSystemImage(for:)`` results by name.
///
/// Names come from host configuration, so the live set is a handful of entries
/// and never turns over. The bound exists so a pathological host cannot grow
/// this without limit.
final class SplitActionSystemImageCache {
/// The instance the tab bar uses. Tests build their own so they never race
/// each other through shared state.
static let shared = SplitActionSystemImageCache()

private static let capacity = 256

private let lock = NSLock()
private var resolved: [String: TabBarStyling.SplitActionSystemImage] = [:]
private var resolutions = 0

init() {}

func image(for name: String) -> TabBarStyling.SplitActionSystemImage {
lock.lock()
if let hit = resolved[name] {
lock.unlock()
return hit
}
lock.unlock()

// Resolved outside the lock so a symbol lookup never blocks another
// caller. Two threads racing on the same name both resolve and agree.
let image = TabBarStyling.resolveSplitActionSystemImage(for: name)

lock.lock()
resolutions += 1
if resolved.count < Self.capacity {
resolved[name] = image
}
lock.unlock()
return image
}

/// How many times the uncached resolution has run. Tests only.
var resolutionCount: Int {
lock.lock()
defer { lock.unlock() }
return resolutions
}
}

private final class SplitActionButtonImageCache {
static let shared = SplitActionButtonImageCache()

Expand Down
87 changes: 87 additions & 0 deletions Tests/BonsplitTests/SplitActionSystemImageCacheTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import AppKit
@testable import Bonsplit
import Testing

/// `splitActionButtonIcon` resolves every split button's icon during every tab
/// bar body evaluation, and resolving asks AppKit whether a name is a real SF
/// Symbol by loading the symbol image and discarding it. Under an animating tab
/// title the tab bar re-evaluates ~20 times a second, so the resolution has to
/// be memoized rather than repeated.
@Suite("Split action symbol resolution")
struct SplitActionSystemImageCacheTests {
private let names = ["plus", "square.split.2x1", "ellipsis.vertical", "not.a.real.symbol.name"]

@Test func repeatedLookupsResolveEachNameOnce() {
let cache = SplitActionSystemImageCache()

for _ in 0..<500 {
for name in names {
_ = cache.image(for: name)
}
}

#expect(cache.resolutionCount == names.count)
}

@Test func memoizingDoesNotChangeWhatIsResolved() {
let cache = SplitActionSystemImageCache()

// A real symbol keeps its own name and is drawn upright.
let real = cache.image(for: "plus")
#expect(real == TabBarStyling.resolveSplitActionSystemImage(for: "plus"))
#expect(real.name == "plus")
#expect(real.rotationDegrees == 0)

// `ellipsis.vertical` is not a symbol; it is a rotated `ellipsis`.
let rotated = cache.image(for: "ellipsis.vertical")
#expect(rotated == TabBarStyling.resolveSplitActionSystemImage(for: "ellipsis.vertical"))
#expect(rotated.name == "ellipsis")
#expect(rotated.rotationDegrees == 90)

// Anything else falls back rather than drawing nothing.
let unknown = cache.image(for: "not.a.real.symbol.name")
#expect(unknown == TabBarStyling.resolveSplitActionSystemImage(for: "not.a.real.symbol.name"))
#expect(unknown.name == "questionmark.circle")
}

@Test func theTabBarGoesThroughTheCache() {
// A name no other test in this file touches, so the shared cache has
// never seen it and the count below is exact. 200 says the accessor
// resolves every time; 0 says it went around the cache entirely.
let name = "rectangle.split.3x1"

let before = SplitActionSystemImageCache.shared.resolutionCount
for _ in 0..<200 {
_ = TabBarStyling.splitActionSystemImage(for: name)
}
let resolutions = SplitActionSystemImageCache.shared.resolutionCount - before

#expect(resolutions == 1)
}

/// A repeat lookup must not reach AppKit at all. Timed rather than counted
/// so the assertion still means something if the cache is later replaced by
/// a different mechanism. The bound is loose on purpose: the measured gap is
/// two to three orders of magnitude.
@Test func cachedLookupsAreOrdersOfMagnitudeCheaper() {
let iterations = 2_000
let name = "square.split.2x1"
let cache = SplitActionSystemImageCache()

let uncachedStart = ContinuousClock.now
for _ in 0..<iterations {
_ = TabBarStyling.resolveSplitActionSystemImage(for: name)
}
let uncached = ContinuousClock.now - uncachedStart

_ = cache.image(for: name)
let cachedStart = ContinuousClock.now
for _ in 0..<iterations {
_ = cache.image(for: name)
}
let cached = ContinuousClock.now - cachedStart

print("[split-action-symbol] \(iterations) lookups: uncached \(uncached), cached \(cached)")
#expect(cached < uncached / 10)
}
}
Loading