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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
other non-browser) tabs keep their titled layout.

### Fixed
- A tab title change no longer re-renders the whole tab bar. `TabItem` is now a
reference type, so writing one tab's title touches that object instead of the
`@Observable` `PaneState.tabs` array, and observation delivers the change to
the one `TabItemView` that reads the title. Inserting, removing, and moving
tabs still publish through `tabs`. Equality and hashing were already by `id`,
so identity comparisons are unaffected.
- 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
Expand Down
16 changes: 14 additions & 2 deletions Sources/Bonsplit/Internal/Models/TabItem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,19 @@ extension UTType {
}

/// Represents a single tab in a pane's tab bar (internal representation)
struct TabItem: Identifiable, Hashable, Codable {
///
/// A reference type on purpose. `PaneState.tabs` is an `@Observable` property,
/// so when tabs were values, writing one tab's title wrote the whole array and
/// invalidated every view that had read it: all of `TabBarView`, not just the
/// one tab that changed. As references, a per-tab write touches only this
/// object, and observation delivers it to the `TabItemView` that read the
/// property. `PaneState.tabs` still changes on insert, remove, and move, which
/// is what `TabBarView` actually needs to see.
///
/// Equality and hashing are by `id`, as they were when this was a struct, so
/// identity comparisons behave the same.
@Observable
final class TabItem: Identifiable, Hashable, Codable {
let id: UUID
var title: String
var hasCustomTitle: Bool
Expand Down Expand Up @@ -93,7 +105,7 @@ struct TabItem: Identifiable, Hashable, Codable {
case presence
}

init(from decoder: Decoder) throws {
required init(from decoder: Decoder) throws {
let c = try decoder.container(keyedBy: CodingKeys.self)
self.id = try c.decode(UUID.self, forKey: .id)
self.title = try c.decode(String.self, forKey: .title)
Expand Down
26 changes: 13 additions & 13 deletions Sources/Bonsplit/Public/BonsplitController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -365,43 +365,43 @@ public final class BonsplitController {
guard didChange else { return }

if let title = title {
pane.tabs[tabIndex].title = title
currentTab.title = title
}
if let icon = icon {
pane.tabs[tabIndex].icon = icon
currentTab.icon = icon
}
if let iconImageData = iconImageData {
pane.tabs[tabIndex].iconImageData = iconImageData
currentTab.iconImageData = iconImageData
}
if let iconAsset = iconAsset {
pane.tabs[tabIndex].iconAsset = iconAsset
currentTab.iconAsset = iconAsset
}
if let kind = kind {
pane.tabs[tabIndex].kind = kind
currentTab.kind = kind
}
if let hasCustomTitle = hasCustomTitle {
pane.tabs[tabIndex].hasCustomTitle = hasCustomTitle
currentTab.hasCustomTitle = hasCustomTitle
}
if let isDirty = isDirty {
pane.tabs[tabIndex].isDirty = isDirty
currentTab.isDirty = isDirty
}
if let showsNotificationBadge = showsNotificationBadge {
pane.tabs[tabIndex].showsNotificationBadge = showsNotificationBadge
currentTab.showsNotificationBadge = showsNotificationBadge
}
if let isLoading = isLoading {
pane.tabs[tabIndex].isLoading = isLoading
currentTab.isLoading = isLoading
}
if let isAudioMuted = isAudioMuted {
pane.tabs[tabIndex].isAudioMuted = isAudioMuted
currentTab.isAudioMuted = isAudioMuted
}
if let isAudioPlaying = isAudioPlaying {
pane.tabs[tabIndex].isAudioPlaying = isAudioPlaying
currentTab.isAudioPlaying = isAudioPlaying
}
if let isPinned = isPinned {
pane.tabs[tabIndex].isPinned = isPinned
currentTab.isPinned = isPinned
}
if let showsRemoteIndicator = showsRemoteIndicator {
pane.tabs[tabIndex].showsRemoteIndicator = showsRemoteIndicator
currentTab.showsRemoteIndicator = showsRemoteIndicator
}
if let presence = presence {
pane.tabs[tabIndex].presence = presence
Expand Down
140 changes: 140 additions & 0 deletions Tests/BonsplitTests/TabObservationScopeTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import Observation
import XCTest
@testable import Bonsplit

/// `PaneState.tabs` is an `@Observable` property. `TabBarView.body` reads it to
/// build its `ForEach`, and `TabItemView.body` reads a single tab's `title`.
///
/// When `TabItem` was a struct, `tabs[i].title = x` wrote the whole array, so a
/// title change invalidated everything that had read `tabs`: every tab item, the
/// scroll content, and the split button row. A host whose tab titles animate
/// drove that about 20 times a second.
///
/// These tests pin who gets woken.
final class TabObservationScopeTests: XCTestCase {
@MainActor
func testChangingOneTabTitleDoesNotInvalidateTheTabsArray() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))
let tabId = try XCTUnwrap(controller.createTab(title: "First"))

// Stand in for TabBarView.body, which reads the array to lay out tabs.
var tabsInvalidated = false
withObservationTracking {
_ = pane.tabs
} onChange: {
tabsInvalidated = true
}

controller.updateTab(tabId, title: "First (running)")

XCTAssertFalse(
tabsInvalidated,
"A title change must not invalidate readers of the tabs array"
)
}

@MainActor
func testChangingOneTabTitleInvalidatesThatTab() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))
let tabId = try XCTUnwrap(controller.createTab(title: "First"))
let tab = try XCTUnwrap(pane.tabs.first { $0.id == tabId.id })

// Stand in for TabItemView.body, which renders the title.
var titleInvalidated = false
withObservationTracking {
_ = tab.title
} onChange: {
titleInvalidated = true
}

controller.updateTab(tabId, title: "First (running)")

XCTAssertTrue(titleInvalidated, "The tab whose title changed must still redraw")
XCTAssertEqual(tab.title, "First (running)")
}

@MainActor
func testChangingOneTabTitleDoesNotInvalidateItsSiblings() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))
let firstId = try XCTUnwrap(controller.createTab(title: "First"))
let secondId = try XCTUnwrap(controller.createTab(title: "Second"))
let second = try XCTUnwrap(pane.tabs.first { $0.id == secondId.id })

var siblingInvalidated = false
withObservationTracking {
_ = second.title
} onChange: {
siblingInvalidated = true
}

controller.updateTab(firstId, title: "First (running)")

XCTAssertFalse(siblingInvalidated, "A sibling tab must not redraw for another tab's title")
}

/// The array must still publish the changes it is the right home for.
@MainActor
func testAddingATabStillInvalidatesTheTabsArray() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))

var tabsInvalidated = false
withObservationTracking {
_ = pane.tabs
} onChange: {
tabsInvalidated = true
}

_ = controller.createTab(title: "Second")

XCTAssertTrue(tabsInvalidated, "Adding a tab changes the layout and must invalidate the array")
}

@MainActor
func testRemovingATabStillInvalidatesTheTabsArray() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))
let tabId = try XCTUnwrap(controller.createTab(title: "Second"))

var tabsInvalidated = false
withObservationTracking {
_ = pane.tabs
} onChange: {
tabsInvalidated = true
}

_ = controller.closeTab(tabId)

XCTAssertTrue(tabsInvalidated, "Removing a tab changes the layout and must invalidate the array")
}

/// A no-op write stays a no-op. `updateTab` guards on `didChange`, and that
/// guard is what keeps a terminal re-emitting an unchanged title free.
@MainActor
func testWritingTheSameTitleWakesNobody() throws {
let controller = BonsplitController()
let paneId = try XCTUnwrap(controller.focusedPaneId)
let pane = try XCTUnwrap(controller.internalController.paneState(for: paneId))
let tabId = try XCTUnwrap(controller.createTab(title: "First"))
let tab = try XCTUnwrap(pane.tabs.first { $0.id == tabId.id })

var titleInvalidated = false
withObservationTracking {
_ = tab.title
} onChange: {
titleInvalidated = true
}

controller.updateTab(tabId, title: "First")

XCTAssertFalse(titleInvalidated)
}
}
Loading