diff --git a/.github/swift-file-length-budget.tsv b/.github/swift-file-length-budget.tsv index fa65d6dcf094..698df2753594 100644 --- a/.github/swift-file-length-budget.tsv +++ b/.github/swift-file-length-budget.tsv @@ -24,8 +24,8 @@ 4483 Sources/cmuxApp.swift 4482 Sources/Panels/FilePreviewPanel.swift 4367 cmuxTests/BrowserPanelTests.swift -4283 Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift 4121 Sources/BrowserWindowPortal.swift +3964 Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift 3934 Sources/Feed/FeedPanelView.swift 3926 cmuxTests/TabManagerUnitTests.swift 3896 cmuxTests/WindowAndDragTests.swift @@ -173,11 +173,11 @@ 637 Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTestSupport.swift 635 cmuxUITests/RightSidebarChromeHeightUITests.swift 630 Packages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutWhenClause.swift -620 Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift 620 cmuxTests/FinderFileDropRegressionTests.swift 620 cmuxTests/TerminalNotificationQueueTests.swift 617 Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellRenderGridLivenessTests.swift 617 Sources/SettingsNavigation.swift +610 Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift 608 cmuxUITests/FeedSidebarUITests.swift 607 Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swift 607 Sources/SessionIndexModels.swift diff --git a/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift b/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift index 6024433ecfcf..d65340b8d2f8 100644 --- a/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift +++ b/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/KeyboardShortcutsSection.swift @@ -316,6 +316,12 @@ public struct KeyboardShortcutsSection: View { /// `shortcuts.when` override when one is configured, otherwise a localized /// description of the action's built-in focus scope. `nil` for ordinary /// always-on shortcuts so unscoped rows stay uncluttered. + /// + /// The built-in scope mapping lives in ``ShortcutScopeCaptionResolver`` so it + /// can be unit-tested directly; it labels context-scoped duplicate defaults + /// (e.g. the ⌘=/⌘-/⌘0 zoom family shared across browser, markdown, and canvas) + /// so they read as deliberately context-separated rather than as a conflict + /// (issue #5810). private func scopeCaption(for action: ShortcutAction) -> String? { if let overrideClause = whenOverrideClauses[action.rawValue] { // An explicit empty/`true` override means "no restriction" — show @@ -327,23 +333,7 @@ public struct KeyboardShortcutsSection: View { let format = String(localized: "shortcut.when.caption.override", defaultValue: "When: %@") return String.localizedStringWithFormat(format, raw) } - switch action.defaultFocusWhenClause { - case .always: - return nil - case .atom(.sidebarFocus): - return String(localized: "shortcut.when.caption.sidebarFocus", defaultValue: "Only while the right sidebar is focused") - case .atom(.browserFocus): - return String(localized: "shortcut.when.caption.browserFocus", defaultValue: "Only while a browser pane is focused") - case .atom(.filePreviewTextEditorFocus): - return String(localized: "shortcut.when.caption.filePreviewTextEditorFocus", defaultValue: "Only while a text file preview is focused") - case .or(.atom(.browserFocus), .atom(.filePreviewTextEditorFocus)), - .or(.atom(.filePreviewTextEditorFocus), .atom(.browserFocus)): - return String(localized: "shortcut.when.caption.browserOrFilePreviewTextEditorFocus", defaultValue: "Only while a browser pane or text file preview is focused") - case .atom(.markdownFocus): - return String(localized: "shortcut.when.caption.markdownFocus", defaultValue: "Only while a markdown preview is focused") - default: - return String(localized: "shortcut.when.caption.terminalFocus", defaultValue: "Only while a terminal pane is focused") - } + return ShortcutScopeCaptionResolver().caption(for: action.defaultFocusWhenClause) } private func detectConflict(for action: ShortcutAction, stroke: StoredShortcut) -> ShortcutAction? { diff --git a/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/ShortcutScopeCaption.swift b/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/ShortcutScopeCaption.swift new file mode 100644 index 000000000000..92f6cca2f2cc --- /dev/null +++ b/Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/ShortcutScopeCaption.swift @@ -0,0 +1,76 @@ +import CmuxSettings +import Foundation + +/// Resolves localized captions for built-in keyboard shortcut focus scopes. +struct ShortcutScopeCaptionResolver { + /// Returns a caption for the built-in focus/layout scope represented by `clause`. + func caption(for clause: ShortcutWhenClause) -> String? { + let canvasLayoutKey = ShortcutContextKnownKey.workspaceCanvasLayout.rawValue + switch clause { + case .always: + return nil + case .atom(.sidebarFocus): + return String( + localized: "shortcut.when.caption.sidebarFocus", + defaultValue: "Only while the right sidebar is focused" + ) + case .atom(.browserFocus): + return String( + localized: "shortcut.when.caption.browserFocus", + defaultValue: "Only while a browser pane is focused" + ) + case .atom(.filePreviewTextEditorFocus): + return String( + localized: "shortcut.when.caption.filePreviewTextEditorFocus", + defaultValue: "Only while a text file preview is focused" + ) + case .or(.atom(.browserFocus), .atom(.filePreviewTextEditorFocus)), + .or(.atom(.filePreviewTextEditorFocus), .atom(.browserFocus)): + return String( + localized: "shortcut.when.caption.browserOrFilePreviewTextEditorFocus", + defaultValue: "Only while a browser pane or text file preview is focused" + ) + case .atom(.markdownFocus): + return String( + localized: "shortcut.when.caption.markdownFocus", + defaultValue: "Only while a markdown preview is focused" + ) + case let other where clauseRequiresKey(other, key: canvasLayoutKey): + // Any clause that *requires* the canvas-layout key is scoped to the + // canvas layout — both the plain `Canvas: …` actions (`canvas`) and + // `Canvas: Actual Size`, whose `canvas && !browser && !markdown` clause + // keeps its ⌘0 default off the browser/markdown zoom-reset bindings. + // Matching the key as a required conjunct (rather than positionally) + // avoids re-introducing the very mislabel this caption fixes if a future + // clause places the canvas predicate on the other side of an `&&`. + return canvasLayoutCaption() + default: + // The remaining built-in scope is the terminal-pane predicate used by + // Rename Tab/Workspace, Send Ctrl-F, and Clear Screen + // (`!browser && !sidebar`). + return String( + localized: "shortcut.when.caption.terminalFocus", + defaultValue: "Only while a terminal pane is focused" + ) + } + } + + /// Whether `clause` requires `key` to be true as a top-level `&&` conjunct. + private func clauseRequiresKey(_ clause: ShortcutWhenClause, key: String) -> Bool { + switch clause { + case let .key(name): + return name == key + case let .and(lhs, rhs): + return clauseRequiresKey(lhs, key: key) || clauseRequiresKey(rhs, key: key) + default: + return false + } + } + + private func canvasLayoutCaption() -> String { + String( + localized: "shortcut.when.caption.canvasLayout", + defaultValue: "Only while the canvas layout is active" + ) + } +} diff --git a/Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/ShortcutScopeCaptionTests.swift b/Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/ShortcutScopeCaptionTests.swift new file mode 100644 index 000000000000..39f7df795ee6 --- /dev/null +++ b/Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/ShortcutScopeCaptionTests.swift @@ -0,0 +1,133 @@ +import Testing +import CmuxSettings +@testable import CmuxSettingsUI + +/// Coverage for the Settings → Keyboard Shortcuts scope caption that labels +/// context-scoped duplicate default shortcuts (issue #5810). +/// +/// Several actions intentionally ship the *same* default chord, disambiguated +/// only by focus/layout context — most visibly the zoom family: `⌘=` / `⌘-` +/// across Zoom In/Out (browser) and Markdown Viewer: Zoom In/Out, and `⌘0` +/// across the browser, markdown, and canvas "Actual Size" actions. Each row +/// must state the scope its binding fires in so the list does not read as a raw +/// duplicate-default collision. +@Suite("Keyboard shortcut scope captions") +struct ShortcutScopeCaptionTests { + // Package tests resolve `String(localized:)` to the `defaultValue` because + // the app's `Localizable.xcstrings` is not bundled with the package test + // target, so the English defaults are the source of truth here. + private let browserCaption = "Only while a browser pane is focused" + private let filePreviewTextEditorCaption = "Only while a text file preview is focused" + private let browserOrFilePreviewTextEditorCaption = "Only while a browser pane or text file preview is focused" + private let markdownCaption = "Only while a markdown preview is focused" + private let sidebarCaption = "Only while the right sidebar is focused" + private let terminalCaption = "Only while a terminal pane is focused" + private let canvasCaption = "Only while the canvas layout is active" + + private let resolver = ShortcutScopeCaptionResolver() + private var canvasLayoutKey: String { ShortcutContextKnownKey.workspaceCanvasLayout.rawValue } + + @Test func alwaysClauseHasNoCaption() { + #expect(resolver.caption(for: .always) == nil) + } + + @Test func focusAtomClausesMapToTheirCaption() { + #expect(resolver.caption(for: .atom(.sidebarFocus)) == sidebarCaption) + #expect(resolver.caption(for: .atom(.browserFocus)) == browserCaption) + #expect(resolver.caption(for: .atom(.filePreviewTextEditorFocus)) == filePreviewTextEditorCaption) + #expect(resolver.caption(for: .atom(.markdownFocus)) == markdownCaption) + } + + @Test func browserOrFilePreviewTextEditorClauseMapsToCombinedCaption() { + let browserFirst = ShortcutWhenClause.or(.atom(.browserFocus), .atom(.filePreviewTextEditorFocus)) + let filePreviewFirst = ShortcutWhenClause.or(.atom(.filePreviewTextEditorFocus), .atom(.browserFocus)) + + #expect(resolver.caption(for: browserFirst) == browserOrFilePreviewTextEditorCaption) + #expect(resolver.caption(for: filePreviewFirst) == browserOrFilePreviewTextEditorCaption) + } + + @Test func terminalPredicateMapsToTerminalCaption() { + // Rename Tab/Workspace, Send Ctrl-F, Clear Screen gate on + // `!browser && !sidebar`. + let clause = ShortcutWhenClause.and(.not(.atom(.browserFocus)), .not(.atom(.sidebarFocus))) + #expect(resolver.caption(for: clause) == terminalCaption) + } + + @Test func canvasLayoutClausesMapToCanvasCaption() { + // Plain canvas-layout actions (Canvas: Zoom In/Out, Tidy, Align, …). + #expect(resolver.caption(for: .key(canvasLayoutKey)) == canvasCaption) + // `Canvas: Actual Size` gates on `canvas && !browser && !markdown` so it + // never collides with the browser/markdown ⌘0 zoom-reset bindings. + let compound = ShortcutWhenClause.and( + .key(canvasLayoutKey), + .and(.not(.atom(.browserFocus)), .not(.atom(.markdownFocus))) + ) + #expect(resolver.caption(for: compound) == canvasCaption) + } + + /// The canvas key must be recognized as a required `&&` conjunct regardless + /// of which side of the `.and` it sits on, so a future clause structure can't + /// silently fall back to the terminal caption (the bug this PR fixes). + @Test func canvasLayoutKeyIsMatchedRegardlessOfConjunctPosition() { + let rightPlaced = ShortcutWhenClause.and(.not(.atom(.browserFocus)), .key(canvasLayoutKey)) + #expect(resolver.caption(for: rightPlaced) == canvasCaption) + + let nested = ShortcutWhenClause.and( + .not(.atom(.markdownFocus)), + .and(.not(.atom(.browserFocus)), .key(canvasLayoutKey)) + ) + #expect(resolver.caption(for: nested) == canvasCaption) + + // A *negated* canvas predicate is not a canvas requirement, so it must + // not borrow the canvas caption. + let negated = ShortcutWhenClause.and(.not(.key(canvasLayoutKey)), .not(.atom(.sidebarFocus))) + #expect(resolver.caption(for: negated) == terminalCaption) + } + + /// Regression for issue #5810: the ⌘= / ⌘- / ⌘0 zoom shortcuts ship the same + /// default across browser, markdown, and (for ⌘0) canvas. Each must carry the + /// caption for its own scope. In particular `canvasZoomReset` must read as the + /// canvas layout, not be mislabeled "Only while a terminal pane is focused". + @Test func zoomFamilyActionsCarryTheirOwnScopeCaption() { + for action in [ShortcutAction.browserZoomIn, .browserZoomOut, .browserZoomReset] { + #expect(resolver.caption(for: action.defaultFocusWhenClause) == browserOrFilePreviewTextEditorCaption) + } + for action in [ShortcutAction.markdownZoomIn, .markdownZoomOut, .markdownZoomReset] { + #expect(resolver.caption(for: action.defaultFocusWhenClause) == markdownCaption) + } + for action in [ShortcutAction.canvasZoomIn, .canvasZoomOut, .canvasZoomReset] { + #expect(resolver.caption(for: action.defaultFocusWhenClause) == canvasCaption) + } + // The specific mislabel this issue fixes: canvas must not fall through to + // the terminal caption. + #expect(resolver.caption(for: ShortcutAction.canvasZoomReset.defaultFocusWhenClause) != terminalCaption) + } + + /// The duplicate default chords are intentional: each pair shares one + /// keystroke yet has context-disjoint effective clauses, so the runtime and + /// the recorder let them coexist. That coexistence is exactly why the scope + /// caption is required to disambiguate them in Settings. + @Test func zoomDuplicateDefaultsShareAStrokeButCoexist() throws { + func assertCoexisting(_ lhs: ShortcutAction, _ rhs: ShortcutAction) throws { + let lhsShortcut = try #require(lhs.defaultShortcut) + let rhsShortcut = try #require(rhs.defaultShortcut) + #expect(!lhsShortcut.isUnbound) + #expect(!rhsShortcut.isUnbound) + // Same physical keystroke… + #expect(numberedAwareStrokesConflict( + lhsShortcut.first, numbered: lhs.usesNumberedDigitMatching, + rhsShortcut.first, numbered: rhs.usesNumberedDigitMatching + )) + // …but context-disjoint, so they are not a real collision. + #expect(!ShortcutWhenClause.bindingsCollide( + lhs.defaultFocusWhenClause, lhsHasPriority: lhs.hasPriorityShortcutRouting, + rhs.defaultFocusWhenClause, rhsHasPriority: rhs.hasPriorityShortcutRouting + )) + } + try assertCoexisting(.browserZoomIn, .markdownZoomIn) // ⌘= + try assertCoexisting(.browserZoomOut, .markdownZoomOut) // ⌘- + try assertCoexisting(.browserZoomReset, .markdownZoomReset) // ⌘0 + try assertCoexisting(.browserZoomReset, .canvasZoomReset) // ⌘0 + try assertCoexisting(.markdownZoomReset, .canvasZoomReset) // ⌘0 + } +} diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 4028a5ebc9f9..6393fe82560d 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -185840,6 +185840,131 @@ } } }, + "shortcut.when.caption.canvasLayout": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Only while the canvas layout is active" + } + }, + "ar": { + "stringUnit": { + "state": "translated", + "value": "فقط أثناء تفعيل تخطيط canvas" + } + }, + "bs": { + "stringUnit": { + "state": "translated", + "value": "Samo dok je canvas raspored aktivan" + } + }, + "da": { + "stringUnit": { + "state": "translated", + "value": "Kun mens canvas-layoutet er aktivt" + } + }, + "de": { + "stringUnit": { + "state": "translated", + "value": "Nur solange das Canvas-Layout aktiv ist" + } + }, + "es": { + "stringUnit": { + "state": "translated", + "value": "Solo mientras el diseño de lienzo está activo" + } + }, + "fr": { + "stringUnit": { + "state": "translated", + "value": "Uniquement lorsque la disposition canvas est active" + } + }, + "it": { + "stringUnit": { + "state": "translated", + "value": "Solo mentre il layout canvas è attivo" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "キャンバスレイアウトが有効なときのみ" + } + }, + "km": { + "stringUnit": { + "state": "translated", + "value": "តែពេលប្លង់ canvas កំពុងសកម្ម ប៉ុណ្ណោះ" + } + }, + "ko": { + "stringUnit": { + "state": "translated", + "value": "캔버스 레이아웃이 활성화된 동안에만" + } + }, + "nb": { + "stringUnit": { + "state": "translated", + "value": "Bare mens canvas-oppsettet er aktivt" + } + }, + "pl": { + "stringUnit": { + "state": "translated", + "value": "Tylko gdy układ canvas jest aktywny" + } + }, + "pt-BR": { + "stringUnit": { + "state": "translated", + "value": "Somente enquanto o layout de canvas está ativo" + } + }, + "ru": { + "stringUnit": { + "state": "translated", + "value": "Только пока активен макет canvas" + } + }, + "th": { + "stringUnit": { + "state": "translated", + "value": "เฉพาะเมื่อเลย์เอาต์ canvas เปิดใช้งานอยู่เท่านั้น" + } + }, + "tr": { + "stringUnit": { + "state": "translated", + "value": "Yalnızca canvas düzeni etkinken" + } + }, + "uk": { + "stringUnit": { + "state": "translated", + "value": "Лише коли активний макет canvas" + } + }, + "zh-Hans": { + "stringUnit": { + "state": "translated", + "value": "仅当画布布局处于活动状态时" + } + }, + "zh-Hant": { + "stringUnit": { + "state": "translated", + "value": "僅在畫布版面啟用時" + } + } + } + }, "shortcut.when.caption.filePreviewTextEditorFocus": { "extractionState": "manual", "localizations": {