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
2 changes: 1 addition & 1 deletion Sources/ContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12143,7 +12143,7 @@ enum ShortcutHintModifierPolicy {
enum ShortcutHintDebugSettings {
static let defaultSidebarHintX = 0.0
static let defaultSidebarHintY = 0.0
static let defaultTitlebarHintX = 4.0
static let defaultTitlebarHintX = 0.0
static let defaultTitlebarHintY = -5.0
static let defaultPaneHintX = 0.0
static let defaultPaneHintY = 0.0
Expand Down
2 changes: 1 addition & 1 deletion Sources/Update/MinimalModeSidebarControls.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ struct MinimalModeSidebarControlActionProxyView: NSViewRepresentable {
}

enum TitlebarControlsHitRegions {
static let outerLeadingPadding: CGFloat = 0
static let outerLeadingPadding: CGFloat = HeaderChromeControlMetrics.titlebarControlsLeadingPadding
static let buttonCount = MinimalModeSidebarControlActionSlot.allCases.count

static func buttonXRanges(config: TitlebarControlsStyleConfig) -> [ClosedRange<CGFloat>] {
Expand Down
114 changes: 52 additions & 62 deletions Sources/Update/UpdateTitlebarAccessory.swift
Original file line number Diff line number Diff line change
Expand Up @@ -433,22 +433,17 @@ func titlebarHintLayoutRightmostExtent(
let shortcut = KeyboardShortcutSettings.shortcut(for: slot.action)
guard !shortcut.isUnbound, shortcut.command else { continue }
let width = titlebarHintPillWidth(for: shortcut, config: config)
let index = CGFloat(slot.rawValue)
let buttonRightEdge = (index + 1) * config.buttonSize + index * config.spacing
let rightEdge = config.groupPadding.leading
+ buttonRightEdge
+ xOffset
+ TitlebarControlsLayoutMetrics.hintRightSafetyShift
+ TitlebarControlsLayoutMetrics.hintBaseXShift
intervals.append((rightEdge - width)...rightEdge)
intervals.append(
TitlebarControlsLayoutMetrics.hintInterval(
for: slot,
width: width,
config: config,
xOffset: xOffset
)
)
}
guard !intervals.isEmpty else { return 0 }
let assignedRightEdges = ShortcutHintHorizontalPlanner.assignRightEdges(
for: intervals,
minSpacing: 6,
minLeadingEdge: config.groupPadding.leading
)
return assignedRightEdges.max() ?? 0
return intervals.map(\.upperBound).max() ?? 0
}

enum TitlebarShortcutHintMetrics {
Expand Down Expand Up @@ -485,22 +480,16 @@ enum TitlebarShortcutHintActionSlot: Int, CaseIterable {

enum TitlebarControlsLayoutMetrics {
static let outerLeadingPadding: CGFloat = TitlebarControlsHitRegions.outerLeadingPadding
static let hintRightSafetyShift: CGFloat = 10
static let hintTrailingBaseInset: CGFloat = 8
static let trafficLightGap: CGFloat = 2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dead constant after leadingOffset refactor

trafficLightGap is now defined but referenced nowhere in production code. The only call sites were the old leadingOffset body (which now ignores the traffic-light frame entirely) and the test that was updated to expect 0. Leaving an unreachable constant here risks future callers picking it up and re-introducing the double-offset that this PR is specifically removing.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

/// Constant X shift applied to every hint's right edge in the view's layout. Must
/// match `TitlebarControlsView.titlebarHintBaseXShift` so the reserved width matches
/// the rendered positions.
static let hintBaseXShift: CGFloat = -10
/// Leading inset the controls content sits at inside the accessory; must match the
/// `.padding(.leading, …)` applied to `controlsGroup` in the view body.
static let hintLeadingPadding: CGFloat = 4
static let hintLeadingPadding: CGFloat = HeaderChromeControlMetrics.titlebarControlsLeadingPadding
/// Extra trailing room past the rightmost pill for its capsule stroke and shadow.
static let hintShadowMargin: CGFloat = 4

static func hintTrailingInset(titlebarShortcutHintXOffset: Double = ShortcutHintDebugSettings.defaultTitlebarHintX) -> CGFloat {
max(0, ShortcutHintDebugSettings.clamped(titlebarShortcutHintXOffset))
+ hintRightSafetyShift
+ hintTrailingBaseInset
}

Expand All @@ -510,6 +499,26 @@ enum TitlebarControlsLayoutMetrics {
return (buttonCount * config.buttonSize) + (gapCount * config.spacing)
}

static func buttonCenterX(
for slot: TitlebarShortcutHintActionSlot,
config: TitlebarControlsStyleConfig
) -> CGFloat {
let index = CGFloat(slot.rawValue)
return config.groupPadding.leading
+ (index * (config.buttonSize + config.spacing))
+ (config.buttonSize / 2.0)
}

static func hintInterval(
for slot: TitlebarShortcutHintActionSlot,
width: CGFloat,
config: TitlebarControlsStyleConfig,
xOffset: CGFloat
) -> ClosedRange<CGFloat> {
let centerX = buttonCenterX(for: slot, config: config) + xOffset
return (centerX - (width / 2.0))...(centerX + (width / 2.0))
}

static func contentSize(
config: TitlebarControlsStyleConfig,
titlebarShortcutHintXOffset: Double = ShortcutHintDebugSettings.defaultTitlebarHintX
Expand Down Expand Up @@ -544,16 +553,12 @@ enum TitlebarControlsLayoutMetrics {
}

static func leadingOffset(
trafficLightFrame: NSRect?,
trafficLightFrame _: NSRect?,
debugSnapshot: MinimalModeTitlebarDebugSnapshot
) -> CGFloat {
let debugOffset = MinimalModeTitlebarDebugSettings.leftControlsXOffset(
MinimalModeTitlebarDebugSettings.leftControlsXOffset(
leadingInset: debugSnapshot.leftControlsLeadingInset
)
guard let trafficLightFrame, !trafficLightFrame.isEmpty else {
return debugOffset
}
return max(debugOffset, trafficLightFrame.maxX + trafficLightGap)
}
Comment on lines 555 to 562
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The trafficLightFrame parameter is now completely ignored — its internal label is _. Since AppKit already positions the titlebar accessory past the traffic lights, the frame is no longer needed for the x-offset calculation. Keeping the parameter in the signature means every call site still computes the frame unnecessarily and future readers may assume the argument influences the result. Removing it makes the contract explicit.

Suggested change
static func leadingOffset(
trafficLightFrame: NSRect?,
trafficLightFrame _: NSRect?,
debugSnapshot: MinimalModeTitlebarDebugSnapshot
) -> CGFloat {
let debugOffset = MinimalModeTitlebarDebugSettings.leftControlsXOffset(
MinimalModeTitlebarDebugSettings.leftControlsXOffset(
leadingInset: debugSnapshot.leftControlsLeadingInset
)
guard let trafficLightFrame, !trafficLightFrame.isEmpty else {
return debugOffset
}
return max(debugOffset, trafficLightFrame.maxX + trafficLightGap)
}
static func leadingOffset(
debugSnapshot: MinimalModeTitlebarDebugSnapshot
) -> CGFloat {
MinimalModeTitlebarDebugSettings.leftControlsXOffset(
leadingInset: debugSnapshot.leftControlsLeadingInset
)
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


static func yOffset(
Expand Down Expand Up @@ -822,13 +827,12 @@ struct TitlebarControlsView: View {
private let titlebarShortcutHintXOffset = ShortcutHintDebugSettings.defaultTitlebarHintX
private let titlebarShortcutHintYOffset = ShortcutHintDebugSettings.defaultTitlebarHintY
private let alwaysShowShortcutHints = ShortcutHintDebugSettings.alwaysShowHints()
private let titlebarHintBaseXShift: CGFloat = TitlebarControlsLayoutMetrics.hintBaseXShift

private struct TitlebarHintLayoutItem: Identifiable {
let action: KeyboardShortcutSettings.Action
let shortcut: StoredShortcut
let width: CGFloat
let leftEdge: CGFloat
let centerX: CGFloat

var id: String { action.rawValue }
}
Expand Down Expand Up @@ -1071,24 +1075,15 @@ struct TitlebarControlsView: View {
let intervals = titlebarHintIntervals(config: config, xOffset: xOffset)
guard !intervals.isEmpty else { return [] }

// Keep all titlebar hints on the same Y lane and resolve overlaps by shifting left.
let minimumSpacing: CGFloat = 6
let assignedRightEdges = ShortcutHintHorizontalPlanner.assignRightEdges(
for: intervals.map { $0.interval },
minSpacing: minimumSpacing,
minLeadingEdge: config.groupPadding.leading
)

var items: [TitlebarHintLayoutItem] = []
items.reserveCapacity(intervals.count)
for (index, item) in intervals.enumerated() {
let rightEdge = assignedRightEdges[index]
for item in intervals {
items.append(
TitlebarHintLayoutItem(
action: item.action,
shortcut: item.shortcut,
width: item.width,
leftEdge: rightEdge - item.width
centerX: (item.interval.lowerBound + item.interval.upperBound) / 2.0
)
)
}
Expand All @@ -1110,24 +1105,20 @@ struct TitlebarControlsView: View {
) else { return nil }

let width = titlebarHintWidth(for: shortcut, config: config)
let rightEdge = config.groupPadding.leading
+ titlebarButtonRightEdge(for: slot, config: config)
+ xOffset
+ TitlebarControlsLayoutMetrics.hintRightSafetyShift
+ titlebarHintBaseXShift
return (slot.action, shortcut, width, (rightEdge - width)...rightEdge)
let interval = TitlebarControlsLayoutMetrics.hintInterval(
for: slot,
width: width,
config: config,
xOffset: xOffset
)
return (slot.action, shortcut, width, interval)
}
}

private func titlebarHintWidth(for shortcut: StoredShortcut, config: TitlebarControlsStyleConfig) -> CGFloat {
titlebarHintPillWidth(for: shortcut, config: config)
}

private func titlebarButtonRightEdge(for slot: TitlebarShortcutHintActionSlot, config: TitlebarControlsStyleConfig) -> CGFloat {
let index = CGFloat(slot.rawValue)
return (index + 1) * config.buttonSize + index * config.spacing
}

@ViewBuilder
private func titlebarShortcutHintOverlay(
items: [TitlebarHintLayoutItem],
Expand All @@ -1138,18 +1129,17 @@ struct TitlebarControlsView: View {
+ ShortcutHintDebugSettings.clamped(titlebarShortcutHintYOffset)

ZStack(alignment: .topLeading) {
Color.clear
ForEach(items) { item in
VStack(alignment: .leading, spacing: 0) {
Color.clear.frame(height: yOffset)
HStack(spacing: 0) {
Color.clear.frame(width: item.leftEdge)
titlebarShortcutHintPill(shortcut: item.shortcut, config: config)
.accessibilityIdentifier("titlebarShortcutHint.\(item.action.rawValue)")
.frame(width: item.width, alignment: .leading)
.background(TitlebarChromeGeometryReporter(keyPrefix: "titlebarShortcutHint_\(item.action.rawValue)"))
}
}
.shortcutHintTransition()
titlebarShortcutHintPill(shortcut: item.shortcut, config: config)
.accessibilityIdentifier("titlebarShortcutHint.\(item.action.rawValue)")
.frame(width: item.width, alignment: .center)
.background(TitlebarChromeGeometryReporter(keyPrefix: "titlebarShortcutHint_\(item.action.rawValue)"))
.position(
x: item.centerX,
y: yOffset + titlebarShortcutHintHeight(for: config) / 2.0
)
.shortcutHintTransition()
}
}
.shortcutHintVisibilityAnimation(value: shouldShowTitlebarShortcutHints)
Expand Down
1 change: 1 addition & 0 deletions Sources/WindowChromeMetrics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ enum HeaderChromeControlMetrics {
static let iconSize: CGFloat = 12
static let iconFrameSize: CGFloat = 14
static let cornerRadius: CGFloat = 6
static let titlebarControlsLeadingPadding: CGFloat = 4

static func iconFrameSize(forIconSize iconSize: CGFloat) -> CGFloat {
max(Self.iconFrameSize, iconSize + 2)
Expand Down
2 changes: 1 addition & 1 deletion cmuxTests/ShortcutAndCommandPaletteTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,7 @@ final class ShortcutHintDebugSettingsTests: XCTestCase {
func testDefaultOffsetsMatchCurrentBadgePlacements() {
XCTAssertEqual(ShortcutHintDebugSettings.defaultSidebarHintX, 0.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultSidebarHintY, 0.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultTitlebarHintX, 4.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultTitlebarHintX, 0.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultTitlebarHintY, -5.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultPaneHintX, 0.0)
XCTAssertEqual(ShortcutHintDebugSettings.defaultPaneHintY, 0.0)
Expand Down
8 changes: 4 additions & 4 deletions cmuxTests/UpdatePillReleaseVisibilityTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -219,15 +219,15 @@ final class TitlebarControlsSizingPolicyTests: XCTestCase {

func testTitlebarControlsUseDeterministicContentSize() {
let classic = TitlebarControlsLayoutMetrics.contentSize(config: TitlebarControlsStyle.classic.config)
XCTAssertEqual(classic.width, 150, accuracy: 0.001)
XCTAssertEqual(classic.width, 136, accuracy: 0.001)
XCTAssertEqual(classic.height, WindowChromeMetrics.appTitlebarHeight, accuracy: 0.001)

let compact = TitlebarControlsLayoutMetrics.contentSize(config: TitlebarControlsStyle.compact.config)
XCTAssertEqual(compact.width, 136, accuracy: 0.001)
XCTAssertEqual(compact.width, 126, accuracy: 0.001)
XCTAssertEqual(compact.height, WindowChromeMetrics.appTitlebarHeight, accuracy: 0.001)
}

func testTitlebarControlsLeadingOffsetSticksToTrafficLights() {
func testTitlebarControlsLeadingOffsetDoesNotDoubleApplyTrafficLightPosition() {
let snapshot = MinimalModeTitlebarDebugSnapshot(
leftControlsLeadingInset: MinimalModeTitlebarDebugSettings.defaultLeftControlsLeadingInset,
leftControlsTopInset: MinimalModeTitlebarDebugSettings.defaultLeftControlsTopInset,
Expand All @@ -241,7 +241,7 @@ final class TitlebarControlsSizingPolicyTests: XCTestCase {
trafficLightFrame: trafficLightFrame,
debugSnapshot: snapshot
),
trafficLightFrame.maxX + TitlebarControlsLayoutMetrics.trafficLightGap,
0,
accuracy: 0.001
)
}
Expand Down
6 changes: 6 additions & 0 deletions cmuxTests/WindowAndDragTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,12 @@ final class WindowDragHandleHitTests: XCTestCase {
let config = TitlebarControlsStyle.classic.config
let ranges = TitlebarControlsHitRegions.buttonXRanges(config: config)
XCTAssertEqual(ranges.count, MinimalModeSidebarControlActionSlot.allCases.count)
XCTAssertEqual(
ranges[0].lowerBound,
TitlebarControlsLayoutMetrics.hintLeadingPadding + config.groupPadding.leading,
accuracy: 0.001,
"Hidden titlebar hit regions should share the visible titlebar control leading position."
)

XCTAssertTrue(
TitlebarControlsHitRegions.pointFallsInButtonColumn(
Expand Down
11 changes: 3 additions & 8 deletions cmuxUITests/UpdatePillUITests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -357,16 +357,11 @@ final class TitlebarShortcutHintsUITests: XCTestCase {

XCTAssertEqual(sidebarHintFrame.minY, notificationsHintFrame.minY, accuracy: 1.0)
XCTAssertEqual(notificationsHintFrame.minY, newTabHintFrame.minY, accuracy: 1.0)
XCTAssertEqual(sidebarHintFrame.midX, hintedToggleFrame.midX, accuracy: 1.0)
XCTAssertEqual(notificationsHintFrame.midX, hintedNotificationsFrame.midX, accuracy: 1.0)
XCTAssertEqual(newTabHintFrame.midX, hintedNewTabFrame.midX, accuracy: 1.0)
// Keep the sidebar hint lane to the right of the sidebar icon so it cannot clip into the traffic-light backdrop.
XCTAssertGreaterThanOrEqual(sidebarHintFrame.minX, hintedToggleFrame.minX - 4.0)

let sortedHintFrames = [sidebarHintFrame, notificationsHintFrame, newTabHintFrame]
.sorted { $0.minX < $1.minX }
for index in 1..<sortedHintFrames.count {
let previousFrame = sortedHintFrames[index - 1]
let currentFrame = sortedHintFrames[index]
XCTAssertGreaterThanOrEqual(currentFrame.minX - previousFrame.maxX, 2.0)
}
}

private func launchApp(alwaysShowShortcutHints: Bool = false) -> (XCUIApplication, String) {
Expand Down
Loading