diff --git a/resources/l10n/Localizable.strings b/resources/l10n/Localizable.strings index 7a3dfa42..b4b98af8 100644 --- a/resources/l10n/Localizable.strings +++ b/resources/l10n/Localizable.strings @@ -656,7 +656,7 @@ "Shortcut" = "Shortcut"; /* No comment provided by engineer. */ -"Shortcut already assigned to another action: %@" = "Shortcut already assigned to another action: %@"; +"Shortcut already assigned to: %@" = "Shortcut already assigned to: %@"; /* No comment provided by engineer. */ "Shortcuts When Active" = "Shortcuts When Active"; diff --git a/resources/l10n/ar.lproj/Localizable.strings b/resources/l10n/ar.lproj/Localizable.strings index 5549d049..07527a0a 100644 --- a/resources/l10n/ar.lproj/Localizable.strings +++ b/resources/l10n/ar.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "تم استيراد الإعدادات"; "Settings…" = "الإعدادات…"; "Shortcut" = "اختصار"; -"Shortcut already assigned to another action: %@" = "الاختصار معيّن بالفعل لإجراء آخر: %@"; +"Shortcut already assigned to: %@" = "الاختصار معيّن بالفعل: %@"; "Shortcuts When Active" = "الاختصارات عند النشاط"; "Shortcuts when active…" = "الاختصارات عند النشاط…"; "Show" = "إظهار"; diff --git a/resources/l10n/de.lproj/Localizable.strings b/resources/l10n/de.lproj/Localizable.strings index 73c21c3f..a877ab4b 100644 --- a/resources/l10n/de.lproj/Localizable.strings +++ b/resources/l10n/de.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Einstellungen importiert"; "Settings…" = "Einstellungen…"; "Shortcut" = "Tastenkürzel"; -"Shortcut already assigned to another action: %@" = "Tastenkürzel ist bereits einer anderen Aktion zugewiesen: %@"; +"Shortcut already assigned to: %@" = "Tastenkürzel ist bereits zugewiesen: %@"; "Shortcuts When Active" = "Tastenkürzel bei Aktivität"; "Shortcuts when active…" = "Tastenkürzel bei Aktivität…"; "Show" = "Anzeigen"; diff --git a/resources/l10n/en.lproj/Localizable.strings b/resources/l10n/en.lproj/Localizable.strings index 3fcf6925..5a4250e8 100644 --- a/resources/l10n/en.lproj/Localizable.strings +++ b/resources/l10n/en.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Settings imported"; "Settings…" = "Settings…"; "Shortcut" = "Shortcut"; -"Shortcut already assigned to another action: %@" = "Shortcut already assigned to another action: %@"; +"Shortcut already assigned to: %@" = "Shortcut already assigned to: %@"; "Shortcuts When Active" = "Shortcuts When Active"; "Shortcuts when active…" = "Shortcuts when active…"; "Show" = "Show"; diff --git a/resources/l10n/es.lproj/Localizable.strings b/resources/l10n/es.lproj/Localizable.strings index 48598b6f..d21ba1c0 100644 --- a/resources/l10n/es.lproj/Localizable.strings +++ b/resources/l10n/es.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Ajustes importados"; "Settings…" = "Ajustes…"; "Shortcut" = "Atajo"; -"Shortcut already assigned to another action: %@" = "Atajo ya asignado a otra acción: %@"; +"Shortcut already assigned to: %@" = "Atajo ya asignado a: %@"; "Shortcuts When Active" = "Atajos cuando está activo"; "Shortcuts when active…" = "Atajos cuando está activo…"; "Show" = "Mostrar"; diff --git a/resources/l10n/fr.lproj/Localizable.strings b/resources/l10n/fr.lproj/Localizable.strings index 16781416..64b038c8 100644 --- a/resources/l10n/fr.lproj/Localizable.strings +++ b/resources/l10n/fr.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Réglages importés"; "Settings…" = "Réglages…"; "Shortcut" = "Raccourci"; -"Shortcut already assigned to another action: %@" = "Raccourci déjà affecté à une autre action : %@"; +"Shortcut already assigned to: %@" = "Raccourci déjà affecté à : %@"; "Shortcuts When Active" = "Raccourcis pendant l'utilisation"; "Shortcuts when active…" = "Raccourcis pendant l'utilisation…"; "Show" = "Afficher"; diff --git a/resources/l10n/he.lproj/Localizable.strings b/resources/l10n/he.lproj/Localizable.strings index 3ea7423a..a4f4a210 100644 --- a/resources/l10n/he.lproj/Localizable.strings +++ b/resources/l10n/he.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "ההגדרות יובאו"; "Settings…" = "הגדרות…"; "Shortcut" = "קיצור דרך"; -"Shortcut already assigned to another action: %@" = "קיצור הדרך כבר משויך לפעולה אחרת: %@"; +"Shortcut already assigned to: %@" = "קיצור הדרך כבר משויך: %@"; "Shortcuts When Active" = "קיצורי דרך כאשר פעיל"; "Shortcuts when active…" = "קיצורי דרך כאשר פעיל…"; "Show" = "הצג"; diff --git a/resources/l10n/id.lproj/Localizable.strings b/resources/l10n/id.lproj/Localizable.strings index 85533b07..737c5116 100644 --- a/resources/l10n/id.lproj/Localizable.strings +++ b/resources/l10n/id.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Pengaturan diimpor"; "Settings…" = "Pengaturan…"; "Shortcut" = "Pintasan"; -"Shortcut already assigned to another action: %@" = "Pintasan sudah ditetapkan untuk tindakan lain: %@"; +"Shortcut already assigned to: %@" = "Pintasan sudah ditetapkan untuk: %@"; "Shortcuts When Active" = "Pintasan Saat Aktif"; "Shortcuts when active…" = "Pintasan saat aktif…"; "Show" = "Tampilkan"; diff --git a/resources/l10n/it.lproj/Localizable.strings b/resources/l10n/it.lproj/Localizable.strings index fb310bcf..eef21c7d 100644 --- a/resources/l10n/it.lproj/Localizable.strings +++ b/resources/l10n/it.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Impostazioni importate"; "Settings…" = "Impostazioni…"; "Shortcut" = "Scorciatoia"; -"Shortcut already assigned to another action: %@" = "Scorciatoia già assegnata a un'altra azione: %@"; +"Shortcut already assigned to: %@" = "Scorciatoia già assegnata a: %@"; "Shortcuts When Active" = "Scorciatoie quando attivo"; "Shortcuts when active…" = "Scorciatoie quando attivo…"; "Show" = "Mostra"; diff --git a/resources/l10n/ja.lproj/Localizable.strings b/resources/l10n/ja.lproj/Localizable.strings index def5f64d..c961911c 100644 --- a/resources/l10n/ja.lproj/Localizable.strings +++ b/resources/l10n/ja.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "設定を読み込みました"; "Settings…" = "設定…"; "Shortcut" = "ショートカット"; -"Shortcut already assigned to another action: %@" = "ショートカットはすでに別のアクションに割り当てられています: %@"; +"Shortcut already assigned to: %@" = "ショートカットはすでに割り当てられています: %@"; "Shortcuts When Active" = "アクティブ時のショートカット"; "Shortcuts when active…" = "アクティブ時のショートカット…"; "Show" = "表示"; diff --git a/resources/l10n/ko.lproj/Localizable.strings b/resources/l10n/ko.lproj/Localizable.strings index 0c2782fc..618cf4a2 100644 --- a/resources/l10n/ko.lproj/Localizable.strings +++ b/resources/l10n/ko.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "설정을 가져왔습니다"; "Settings…" = "설정…"; "Shortcut" = "단축키"; -"Shortcut already assigned to another action: %@" = "단축키가 이미 다른 동작에 할당되어 있습니다: %@"; +"Shortcut already assigned to: %@" = "단축키가 이미 할당되어 있습니다: %@"; "Shortcuts When Active" = "활성 시 단축키"; "Shortcuts when active…" = "활성 시 단축키…"; "Show" = "보이기"; diff --git a/resources/l10n/nl.lproj/Localizable.strings b/resources/l10n/nl.lproj/Localizable.strings index 24e474dc..e825f5f6 100644 --- a/resources/l10n/nl.lproj/Localizable.strings +++ b/resources/l10n/nl.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Instellingen geïmporteerd"; "Settings…" = "Instellingen…"; "Shortcut" = "Sneltoets"; -"Shortcut already assigned to another action: %@" = "Sneltoets is al toegewezen aan een andere actie: %@"; +"Shortcut already assigned to: %@" = "Sneltoets is al toegewezen aan: %@"; "Shortcuts When Active" = "Sneltoetsen wanneer actief"; "Shortcuts when active…" = "Sneltoetsen wanneer actief…"; "Show" = "Toon"; diff --git a/resources/l10n/pl.lproj/Localizable.strings b/resources/l10n/pl.lproj/Localizable.strings index 87262757..13b5342d 100644 --- a/resources/l10n/pl.lproj/Localizable.strings +++ b/resources/l10n/pl.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Ustawienia zaimportowane"; "Settings…" = "Ustawienia…"; "Shortcut" = "Skrót"; -"Shortcut already assigned to another action: %@" = "Skrót jest już przypisany do innej akcji: %@"; +"Shortcut already assigned to: %@" = "Skrót jest już przypisany do: %@"; "Shortcuts When Active" = "Skróty, gdy aktywne"; "Shortcuts when active…" = "Skróty, gdy aktywne…"; "Show" = "Pokaż"; diff --git a/resources/l10n/pt-BR.lproj/Localizable.strings b/resources/l10n/pt-BR.lproj/Localizable.strings index 584c2578..16351ea3 100644 --- a/resources/l10n/pt-BR.lproj/Localizable.strings +++ b/resources/l10n/pt-BR.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Ajustes importados"; "Settings…" = "Ajustes…"; "Shortcut" = "Atalho"; -"Shortcut already assigned to another action: %@" = "Atalho já atribuído a outra ação: %@"; +"Shortcut already assigned to: %@" = "Atalho já atribuído a: %@"; "Shortcuts When Active" = "Atalhos quando ativo"; "Shortcuts when active…" = "Atalhos quando ativo…"; "Show" = "Mostrar"; diff --git a/resources/l10n/ru.lproj/Localizable.strings b/resources/l10n/ru.lproj/Localizable.strings index efd002c1..9f365a62 100644 --- a/resources/l10n/ru.lproj/Localizable.strings +++ b/resources/l10n/ru.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Настройки импортированы"; "Settings…" = "Настройки…"; "Shortcut" = "Сочетание клавиш"; -"Shortcut already assigned to another action: %@" = "Сочетание клавиш уже назначено другому действию: %@"; +"Shortcut already assigned to: %@" = "Сочетание клавиш уже назначено: %@"; "Shortcuts When Active" = "Сочетания клавиш при активном переключателе"; "Shortcuts when active…" = "Сочетания клавиш при активном переключателе…"; "Show" = "Показать"; diff --git a/resources/l10n/sv.lproj/Localizable.strings b/resources/l10n/sv.lproj/Localizable.strings index 8ddacf49..6303bc7d 100644 --- a/resources/l10n/sv.lproj/Localizable.strings +++ b/resources/l10n/sv.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Inställningar importerade"; "Settings…" = "Inställningar…"; "Shortcut" = "Kortkommando"; -"Shortcut already assigned to another action: %@" = "Kortkommandot är redan tilldelat en annan åtgärd: %@"; +"Shortcut already assigned to: %@" = "Kortkommandot är redan tilldelat: %@"; "Shortcuts When Active" = "Kortkommandon när aktiv"; "Shortcuts when active…" = "Kortkommandon när aktiv…"; "Show" = "Visa"; diff --git a/resources/l10n/th.lproj/Localizable.strings b/resources/l10n/th.lproj/Localizable.strings index 58f4072a..843a335a 100644 --- a/resources/l10n/th.lproj/Localizable.strings +++ b/resources/l10n/th.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "นำเข้าการตั้งค่าแล้ว"; "Settings…" = "การตั้งค่า…"; "Shortcut" = "ทางลัด"; -"Shortcut already assigned to another action: %@" = "ทางลัดถูกกำหนดให้กับการกระทำอื่นแล้ว: %@"; +"Shortcut already assigned to: %@" = "ทางลัดถูกกำหนดไว้แล้ว: %@"; "Shortcuts When Active" = "ทางลัดเมื่อทำงานอยู่"; "Shortcuts when active…" = "ทางลัดเมื่อทำงานอยู่…"; "Show" = "แสดง"; diff --git a/resources/l10n/tr.lproj/Localizable.strings b/resources/l10n/tr.lproj/Localizable.strings index 6b1f0938..fabf6c5b 100644 --- a/resources/l10n/tr.lproj/Localizable.strings +++ b/resources/l10n/tr.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Ayarlar içe aktarıldı"; "Settings…" = "Ayarlar…"; "Shortcut" = "Kısayol"; -"Shortcut already assigned to another action: %@" = "Kısayol zaten başka bir eyleme atanmış: %@"; +"Shortcut already assigned to: %@" = "Kısayol zaten şuna atanmış: %@"; "Shortcuts When Active" = "Etkinken Kullanılan Kısayollar"; "Shortcuts when active…" = "Etkinken kullanılan kısayollar…"; "Show" = "Göster"; diff --git a/resources/l10n/vi.lproj/Localizable.strings b/resources/l10n/vi.lproj/Localizable.strings index 0e6a02ed..995037c5 100644 --- a/resources/l10n/vi.lproj/Localizable.strings +++ b/resources/l10n/vi.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "Đã nhập cài đặt"; "Settings…" = "Cài đặt…"; "Shortcut" = "Phím tắt"; -"Shortcut already assigned to another action: %@" = "Phím tắt đã được gán cho một hành động khác: %@"; +"Shortcut already assigned to: %@" = "Phím tắt đã được gán cho: %@"; "Shortcuts When Active" = "Phím tắt khi hoạt động"; "Shortcuts when active…" = "Phím tắt khi hoạt động…"; "Show" = "Hiển thị"; diff --git a/resources/l10n/zh-CN.lproj/Localizable.strings b/resources/l10n/zh-CN.lproj/Localizable.strings index 5889c2e6..d1542740 100644 --- a/resources/l10n/zh-CN.lproj/Localizable.strings +++ b/resources/l10n/zh-CN.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "设置已导入"; "Settings…" = "设置…"; "Shortcut" = "快捷键"; -"Shortcut already assigned to another action: %@" = "快捷键已分配给其他操作：%@"; +"Shortcut already assigned to: %@" = "快捷键已分配给：%@"; "Shortcuts When Active" = "激活时的快捷键"; "Shortcuts when active…" = "激活时的快捷键…"; "Show" = "显示"; diff --git a/resources/l10n/zh-HK.lproj/Localizable.strings b/resources/l10n/zh-HK.lproj/Localizable.strings index 1826cf0a..dc7488e2 100644 --- a/resources/l10n/zh-HK.lproj/Localizable.strings +++ b/resources/l10n/zh-HK.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "已輸入設定"; "Settings…" = "設定…"; "Shortcut" = "快捷鍵"; -"Shortcut already assigned to another action: %@" = "快捷鍵已指派給另一項動作:%@"; +"Shortcut already assigned to: %@" = "快捷鍵已指派給:%@"; "Shortcuts When Active" = "作用中時的快捷鍵"; "Shortcuts when active…" = "作用中時的快捷鍵…"; "Show" = "顯示"; diff --git a/resources/l10n/zh-TW.lproj/Localizable.strings b/resources/l10n/zh-TW.lproj/Localizable.strings index b3bd64e1..840cebe4 100644 --- a/resources/l10n/zh-TW.lproj/Localizable.strings +++ b/resources/l10n/zh-TW.lproj/Localizable.strings @@ -217,7 +217,7 @@ "Settings imported" = "設定已輸入"; "Settings…" = "設定…"; "Shortcut" = "快速鍵"; -"Shortcut already assigned to another action: %@" = "快速鍵已指派給其他動作：%@"; +"Shortcut already assigned to: %@" = "快速鍵已指派給：%@"; "Shortcuts When Active" = "啟用時的快速鍵"; "Shortcuts when active…" = "啟用時的快速鍵…"; "Show" = "顯示"; diff --git a/src/_test-support/Mocks.swift b/src/_test-support/Mocks.swift index 2c60cbf2..928da149 100644 --- a/src/_test-support/Mocks.swift +++ b/src/_test-support/Mocks.swift @@ -220,6 +220,8 @@ class Preferences { static var holdShortcut = ["⌥", "⌥", "⌥"] static let minShortcutCount = 1 static let maxShortcutCount = 9 + // Matches `defaultShortcuts` (3 hold slots: holdShortcut / holdShortcut2 / holdShortcut3). + static var shortcutCount = 3 static func indexToName(_ baseName: String, _ index: Int) -> String { return baseName + (index == 0 ? "" : String(index + 1)) diff --git a/src/kit/CustomRecorderControl.swift b/src/kit/CustomRecorderControl.swift index 6e37d02f..eda4c01a 100644 --- a/src/kit/CustomRecorderControl.swift +++ b/src/kit/CustomRecorderControl.swift @@ -5,7 +5,10 @@ import ShortcutRecorder class CustomRecorderControl: RecorderControl { static let allowedModifiers = NSEvent.ModifierFlags(arrayLiteral: [.command, .control, .option, .shift]) var clearable: Bool! - var id: String! + /// The preference key this recorder edits. Derived from `identifier` (set in `init` and re-aimed + /// by `TriggerBinding.bind` as the recycled editor switches shortcuts) so it can never drift out + /// of sync with the key the write path (`controlWasChanged`) and the conflict detector key off. + var id: String { identifier!.rawValue } convenience init(_ shortcutString: String, _ clearable: Bool, _ id: String) { self.init(Shortcut(keyEquivalent: shortcutString), clearable, id) @@ -15,7 +18,7 @@ class CustomRecorderControl: RecorderControl { self.init(frame: .zero) translatesAutoresizingMaskIntoConstraints = false self.clearable = clearable - self.id = id + identifier = NSUserInterfaceItemIdentifier(id) delegate = self allowsEscapeToCancelRecording = false allowsDeleteToClearShortcutAndEndRecording = false @@ -41,49 +44,39 @@ class CustomRecorderControl: RecorderControl { set(allowedModifierFlags: CustomRecorderControl.allowedModifiers.subtracting(restrictedModifiers), requiredModifierFlags: [], allowsEmptyModifierFlags: true) } + /// How to clear the conflicting shortcut when the user picks "Unassign and continue". Resolved + /// from the id alone — arrow/vim are toggled off via their checkboxes; everything else is cleared + /// through `Preferences` by id (so a shortcut that isn't on screen is handled correctly). The + /// human-readable label shown in the dialog comes separately from `ControlsTab.conflictLabel`. private enum ShortcutConflict { case arrow case vim - case regular(control: CustomRecorderControl, label: String) - case unknown - - var label: String { - switch self { - case .arrow: return "Arrow keys" - case .vim: return "Vim keys" - case .regular(_, let label): return label - case .unknown: return "an unknown action" - } - } + case regular(id: String) static func classify(_ id: String) -> ShortcutConflict { if ["←", "→", "↑", "↓"].contains(id) { return .arrow } if id.starts(with: "vimCycle") { return .vim } - if let existing = ControlsTab.shortcutControls[id] { - return .regular(control: existing.0, label: existing.1) - } - assertionFailure("conflict id '\(id)' missing from shortcutControls") - return .unknown + return .regular(id: id) } } func alertIfSameShortcutAlreadyAssigned(_ candidateShortcut: Shortcut, _ shortcutAlreadyAssigned: String) { let conflict = ShortcutConflict.classify(shortcutAlreadyAssigned) + // `conflictLabel` returns nil only for an id with no known action, which can't happen for a + // real detected conflict; keep the prior plain-string fallback (not a new l10n key). + let label = ControlsTab.conflictLabel(shortcutAlreadyAssigned) ?? "an unknown action" let alert = NSAlert() alert.alertStyle = .warning alert.messageText = NSLocalizedString("Conflicting shortcut", comment: "") - alert.informativeText = String(format: NSLocalizedString("Shortcut already assigned to another action: %@", comment: ""), - conflict.label.replacingOccurrences(of: " ", with: "\u{00A0}")) - if !id.starts(with: "holdShortcut") { - alert.addButton(withTitle: NSLocalizedString("Unassign existing shortcut and continue", comment: "")).setAccessibilityFocused(true) - } + alert.informativeText = String(format: NSLocalizedString("Shortcut already assigned to: %@", comment: ""), + label.replacingOccurrences(of: " ", with: "\u{00A0}")) + // Always offer to resolve it, including when editing a Hold: unassigning a conflicting Trigger + // clears its "and press" part (the hold itself is never the thing unassigned). + alert.addButton(withTitle: NSLocalizedString("Unassign existing shortcut and continue", comment: "")).setAccessibilityFocused(true) let cancelButton = alert.addButton(withTitle: NSLocalizedString("Cancel", comment: "")) cancelButton.keyEquivalent = "\u{1b}" - if id.starts(with: "holdShortcut") { - cancelButton.setAccessibilityFocused(true) - } let userChoice = alert.runModal() - guard !id.starts(with: "holdShortcut"), userChoice == .alertFirstButtonReturn else { return } + guard userChoice == .alertFirstButtonReturn else { return } switch conflict { case .arrow: if let cb = ControlsTab.arrowKeysCheckbox { @@ -97,10 +90,8 @@ class CustomRecorderControl: RecorderControl { ControlsTab.vimKeysEnabledCallback(cb) LabelAndControl.controlWasChanged(cb, nil) } - case .regular(let existingControl, _): - updateShortcut(existingControl, nil, existingControl, shortcutAlreadyAssigned) - case .unknown: - return + case .regular(let conflictingId): + ControlsTab.unassignShortcut(conflictingId) } updateShortcut(self, candidateShortcut, self, id) } @@ -112,23 +103,18 @@ class CustomRecorderControl: RecorderControl { } func alertIfShortcutReservedByMacos(_ candidateShortcut: Shortcut, _ shortcutReservedByMacos: String) { - let existing = ControlsTab.shortcutControls[shortcutReservedByMacos] - if existing == nil { - assertionFailure("reserved-by-macos id '\(shortcutReservedByMacos)' missing from shortcutControls") - } + let label = ControlsTab.conflictLabel(shortcutReservedByMacos) ?? "an unknown action" let alert = NSAlert() alert.alertStyle = .warning alert.messageText = NSLocalizedString("Conflicting shortcut", comment: "") - alert.informativeText = String(format: NSLocalizedString("macOS reserves ⌘⌥⎋, ⌘⌥⇧⎋, and ⌘⌥⇧⌃⎋ for Force Quit and they cannot be unbound. AltTab cannot use them.\n\nYour change would assign one of these to: %@.", comment: ""), existing?.1 ?? "an unknown action") + alert.informativeText = String(format: NSLocalizedString("macOS reserves ⌘⌥⎋, ⌘⌥⇧⎋, and ⌘⌥⇧⌃⎋ for Force Quit and they cannot be unbound. AltTab cannot use them.\n\nYour change would assign one of these to: %@.", comment: ""), label) alert.addButton(withTitle: NSLocalizedString("Unassign existing shortcut and continue", comment: "")) let cancelButton = alert.addButton(withTitle: NSLocalizedString("Cancel", comment: "")) cancelButton.keyEquivalent = "\u{1b}" cancelButton.setAccessibilityFocused(true) let userChoice = alert.runModal() guard userChoice == .alertFirstButtonReturn, id != shortcutReservedByMacos else { return } - if let existing { - updateShortcut(existing.0, nil, existing.0, shortcutReservedByMacos) - } + ControlsTab.unassignShortcut(shortcutReservedByMacos) updateShortcut(self, candidateShortcut, self, id) } diff --git a/src/kit/CustomRecorderControlSpecs.md b/src/kit/CustomRecorderControlSpecs.md index dbea5be7..54b2b6e4 100644 --- a/src/kit/CustomRecorderControlSpecs.md +++ b/src/kit/CustomRecorderControlSpecs.md @@ -18,6 +18,11 @@ existing AltTab shortcut or a macOS-reserved one. - A combo **reserved by macOS** is rejected. - Regression (#5585): a Cmd-based hold shortcut is **no longer blocked** by the macOS 26 Game Overlay reservation — that specific over-broad rejection was removed. +- The `candidateId` handed in must be a **well-formed bound preference key** (`isWellFormedCandidateId`): + a `holdShortcut`/`nextWindowShortcut` id has to resolve to an in-range shortcut index. The recycled + `ShortcutEditor` regression fed a frozen placeholder (`"nextWindowShortcut0"`, index -1) that matched + nothing and silently returned `.accepted`, so the conflict dialog stopped appearing. A `#if DEBUG` + assert now trips loudly if a malformed id ever reaches the check. ## Test scenarios @@ -27,9 +32,14 @@ Mirrors `CustomRecorderControlTests.swift` 1:1. - **testIsShortcutAcceptable_accepted** — a valid key+modifier combo is accepted. - **testIsShortcutAcceptable_modifiersOnlyButContainsKeycode** — modifiers-only flags but with a real keycode → accepted. - **testIsShortcutAcceptable_conflictWithExistingShortcut** — collides with an existing shortcut → rejected. +- **testIsShortcutAcceptable_holdChangeStripsOldHoldFromCombinedNextWindow** — changing a shortcut's hold (e.g. ⌘→⌥) so its chord now duplicates another shortcut is detected; the same-index nextWindow is stored COMBINED with the old hold, so the old hold is stripped before applying the new one (else ⌥⌘+Tab is compared instead of ⌥+Tab and the conflict is missed). - **testIsShortcutAcceptable_reservedByMacos** — a macOS-reserved combo → rejected. - **testIsShortcutAcceptable_cmdHoldShortcutNoLongerBlockedByGameOverlay** — regression #5585: a Cmd hold shortcut is accepted (Game Overlay no longer blocks it). +### isWellFormedCandidateId +Guards the recycled-`ShortcutEditor` regression where a stale recorder id (id/identifier drift) reached the conflict check and silently suppressed the dialog. +- **testIsWellFormedCandidateId** — hold/next ids must resolve to an in-range index; the `"…Shortcut0"` placeholders (index -1) and out-of-range ids are rejected; static/arrow/vim ids are always well-formed. + ### combinedModifiersMatch Used by the keyboard matcher to recognize a chord whose modifiers are physically split between the configured `holdShortcut` and a local shortcut (e.g. commandShiftTab = ⌘⌥-hold + ⇧). - **testCombinedModifiersMatchEqualToItself** — a modifier set matches itself (trivial union). diff --git a/src/kit/CustomRecorderControlTestable.swift b/src/kit/CustomRecorderControlTestable.swift index 3766cfef..7e8e1489 100644 --- a/src/kit/CustomRecorderControlTestable.swift +++ b/src/kit/CustomRecorderControlTestable.swift @@ -3,7 +3,19 @@ import Carbon.HIToolbox.Events import ShortcutRecorder class CustomRecorderControlTestable { + /// A `holdShortcut`/`nextWindowShortcut` candidate id must resolve to an in-range shortcut index; + /// every other id (the static "when active" shortcuts, arrow, vim) is well-formed by definition. + /// This is the contract the recycled `ShortcutEditor` must uphold: the id handed to the conflict + /// check is the bound preference key, not a stale placeholder. The regression that hid the + /// conflict dialog was exactly a malformed id — the recorder's frozen `"nextWindowShortcut0"`, + /// whose `nameToIndex` is -1 — silently matching nothing and returning `.accepted`. + static func isWellFormedCandidateId(_ id: String) -> Bool { + guard id.hasPrefix("holdShortcut") || id.hasPrefix("nextWindowShortcut") else { return true } + return (0.. ShortcutAcceptance { + assert(isWellFormedCandidateId(candidateId), "malformed candidateId '\(candidateId)' reached the conflict check — recycled editor id/identifier drift?") if let currentShortcutWithSameId = ControlsTab.shortcuts[candidateId]?.shortcut, candidateShortcut.carbonKeyCode == currentShortcutWithSameId.carbonKeyCode && candidateShortcut.carbonModifierFlags == currentShortcutWithSameId.carbonModifierFlags { return .accepted @@ -32,11 +44,16 @@ class CustomRecorderControlTestable { && !(atShortcut.shortcut.keyCode == .none && atShortcut.shortcut.modifierFlags == []) else { continue } // candidate is holdShortcut if candidateId.starts(with: "holdShortcut") { - // combine with local shortcut - if (!atShortcut.id.starts(with: "holdShortcut") && !atShortcut.id.starts(with: "nextWindowShortcut")) - // combine with nextWindowShortcut of same index - || (atShortcut.id.starts(with: "nextWindowShortcut") && Preferences.nameToIndex(candidateId) == Preferences.nameToIndex(atShortcut.id)) { + // combine the candidate's (new) hold modifiers with a local shortcut, which is stored raw + if !atShortcut.id.starts(with: "holdShortcut") && !atShortcut.id.starts(with: "nextWindowShortcut") { combos.append((atShortcut.id, Shortcut(code: atShortcut.shortcut.keyCode, modifierFlags: [candidateShortcut.modifierFlags, atShortcut.shortcut.modifierFlags], characters: nil, charactersIgnoringModifiers: nil))) + // combine with the nextWindowShortcut of the same index. That shortcut is stored COMBINED + // with the CURRENT (old) hold, so strip the old hold before applying the candidate's new + // hold — otherwise changing the hold (e.g. ⌘→⌥) leaks the old modifier (⌥⌘⇥ instead of + // ⌥⇥) and a real conflict is missed. (No-op when nextWindow is stored raw, e.g. in tests.) + } else if atShortcut.id.starts(with: "nextWindowShortcut") && Preferences.nameToIndex(candidateId) == Preferences.nameToIndex(atShortcut.id) { + let oldHoldModifiers = ControlsTab.shortcuts[Preferences.indexToName("holdShortcut", Preferences.nameToIndex(candidateId))]?.shortcut.modifierFlags ?? [] + combos.append((atShortcut.id, Shortcut(code: atShortcut.shortcut.keyCode, modifierFlags: [candidateShortcut.modifierFlags, atShortcut.shortcut.modifierFlags.subtracting(oldHoldModifiers)], characters: nil, charactersIgnoringModifiers: nil))) } // candidate is nextWindowShortcut } else if candidateId.starts(with: "nextWindowShortcut") { diff --git a/src/kit/CustomRecorderControlTests.swift b/src/kit/CustomRecorderControlTests.swift index 2c4ad092..8a30d250 100644 --- a/src/kit/CustomRecorderControlTests.swift +++ b/src/kit/CustomRecorderControlTests.swift @@ -27,6 +27,25 @@ final class CustomRecorderControlTests: XCTestCase { XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("nextWindowShortcut", Shortcut(keyEquivalent: "⇧")!), .conflictWithExistingShortcut(shortcutAlreadyAssigned: "previousWindowShortcut")) } + /// Regression: S1 = ⌥+Tab, S2 = ⌘+Tab. Changing S2's *hold* from ⌘ to ⌥ makes it ⌥+Tab, which must + /// conflict with S1. The registry here mirrors PRODUCTION, where `nextWindowShortcut` is stored + /// COMBINED (hold ∪ key) — unlike `defaultShortcuts`, which stores it raw, which is precisely why + /// the existing tests never exercised this. Without stripping S2's current ⌘ hold before applying + /// the new ⌥, the chord is mis-computed as ⌥⌘+Tab and the conflict is silently accepted. + func testIsShortcutAcceptable_holdChangeStripsOldHoldFromCombinedNextWindow() { + ControlsTab.shortcuts = [ + "holdShortcut": ATShortcut(Shortcut(keyEquivalent: "⌥")!, "holdShortcut", .global, .up, 0), + "nextWindowShortcut": ATShortcut(Shortcut(keyEquivalent: "⌥⇥")!, "nextWindowShortcut", .global, .down), + "holdShortcut2": ATShortcut(Shortcut(keyEquivalent: "⌘")!, "holdShortcut2", .global, .up, 1), + "nextWindowShortcut2": ATShortcut(Shortcut(keyEquivalent: "⌘⇥")!, "nextWindowShortcut2", .global, .down), + ] + defer { ControlsTab.shortcuts = ControlsTab.defaultShortcuts } + XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("holdShortcut2", Shortcut(keyEquivalent: "⌥")!), + .conflictWithExistingShortcut(shortcutAlreadyAssigned: "nextWindowShortcut")) + // The mirror case: changing the hold to something that does NOT collide stays accepted. + XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("holdShortcut2", Shortcut(keyEquivalent: "⌃")!), .accepted) + } + func testIsShortcutAcceptable_reservedByMacos() { XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("previousWindowShortcut", Shortcut(keyEquivalent: "⌘⇧")!), .accepted) // ⌘⎋ XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("previousWindowShortcut", Shortcut(keyEquivalent: "⌘⌃⇧")!), .accepted) // ⌘⎋ @@ -74,6 +93,30 @@ final class CustomRecorderControlTests: XCTestCase { XCTAssertEqual(CustomRecorderControlTestable.isShortcutAcceptable("holdShortcut", Shortcut(keyEquivalent: "⌘")!), .accepted) } + // MARK: - isWellFormedCandidateId + + /// Regression guard for the conflict dialog that silently stopped appearing under the recycled + /// `ShortcutEditor`: the recorder handed the kernel a frozen placeholder id (`"nextWindowShortcut0"`, + /// whose `nameToIndex` is -1), which matched no shortcut, so `isShortcutAcceptable` returned + /// `.accepted` and no dialog showed. `isWellFormedCandidateId` rejects exactly that shape — and a + /// `#if DEBUG` assert in `isShortcutAcceptable` now trips loudly if such an id ever reaches it. + func testIsWellFormedCandidateId() { + // hold/next ids must resolve to an in-range shortcut index (shortcutCount == 3 in the mock). + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("holdShortcut")) // index 0 + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("nextWindowShortcut")) // index 0 + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("nextWindowShortcut2")) // index 1 + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("holdShortcut3")) // index 2 + // The regressing placeholders: trailing "0" → nameToIndex -1 → out of range. + XCTAssertFalse(CustomRecorderControlTestable.isWellFormedCandidateId("nextWindowShortcut0")) + XCTAssertFalse(CustomRecorderControlTestable.isWellFormedCandidateId("holdShortcut0")) + // Beyond shortcutCount is also malformed. + XCTAssertFalse(CustomRecorderControlTestable.isWellFormedCandidateId("nextWindowShortcut4")) // index 3 + // Static "when active" / arrow / vim ids are well-formed regardless of any index. + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("cancelShortcut")) + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("←")) + XCTAssertTrue(CustomRecorderControlTestable.isWellFormedCandidateId("vimCycleLeft")) + } + // MARK: - combinedModifiersMatch // Per the kernel's doc-comment: the function checks whether two modifier sets match once each is diff --git a/src/preferences/settings-window/tabs/controls/ControlsTab.swift b/src/preferences/settings-window/tabs/controls/ControlsTab.swift index 9ef7403f..5f02cc93 100644 --- a/src/preferences/settings-window/tabs/controls/ControlsTab.swift +++ b/src/preferences/settings-window/tabs/controls/ControlsTab.swift @@ -52,6 +52,21 @@ class ControlsTab { "focusWindowShortcut", "previousWindowShortcut", "cancelShortcut", "searchShortcut", "lockSearchShortcut", "closeWindowShortcut", "minDeminWindowShortcut", "toggleFullscreenWindowShortcut", "quitAppShortcut", "hideShowAppShortcut", ] + /// Canonical id → localized label for the always-active ("when active") shortcuts. Single source + /// of truth: `ShortcutsWhenActiveSheet` reads its row titles from here, and `conflictLabel(_:)` + /// uses it to name a conflicting shortcut without needing its (possibly-unbuilt) sheet control. + static let staticShortcutLabels = [ + "focusWindowShortcut": NSLocalizedString("Focus selected window", comment: ""), + "previousWindowShortcut": NSLocalizedString("Select previous window", comment: ""), + "cancelShortcut": NSLocalizedString("Cancel", comment: ""), + "searchShortcut": NSLocalizedString("Search", comment: ""), + "lockSearchShortcut": NSLocalizedString("Lock search", comment: ""), + "closeWindowShortcut": NSLocalizedString("Close window", comment: ""), + "minDeminWindowShortcut": NSLocalizedString("Minimize/Deminimize window", comment: ""), + "toggleFullscreenWindowShortcut": NSLocalizedString("Fullscreen/Defullscreen window", comment: ""), + "quitAppShortcut": NSLocalizedString("Quit app", comment: ""), + "hideShowAppShortcut": NSLocalizedString("Hide/Show app", comment: ""), + ] private static let removableShortcutPreferences = [ "holdShortcut", "nextWindowShortcut", "appsToShow", "spacesToShow", "screensToShow", @@ -916,17 +931,37 @@ class ControlsTab { return true } - private static func conflictLabel(_ controlId: String) -> String? { - if let shortcutControl = shortcutControls[controlId] { - return shortcutControl.1 + /// Human-readable label for the action bound to `id`, resolved purely from the model — the id's + /// shape plus `staticShortcutLabels` — NOT from `shortcutControls`. This is what lets the conflict + /// dialog name a shortcut that isn't currently displayed in the recycled editor (which keeps only + /// the on-screen shortcut in `shortcutControls`). Returns nil for ids with no known action. + /// + /// A numbered shortcut's hold and "and press" both belong to that shortcut's Trigger, so they + /// resolve to e.g. "Shortcut 2 - Trigger" — naming WHICH shortcut, since "Select next window" + /// alone doesn't disambiguate when several shortcuts exist. + static func conflictLabel(_ id: String) -> String? { + if arrowKeys.contains(id) { return NSLocalizedString("Arrow keys", comment: "") } + if vimKeyActions.values.contains(id) { return NSLocalizedString("Vim keys", comment: "") } + if id.hasPrefix("holdShortcut") || id.hasPrefix("nextWindowShortcut") { + return shortcutTitle(Preferences.nameToIndex(id)) + " - " + ShortcutEditor.triggerLabel } - if arrowKeys.contains(controlId) { - return NSLocalizedString("Arrow keys", comment: "") - } - if vimKeyActions.values.contains(controlId) { - return NSLocalizedString("Vim keys", comment: "") - } - return nil + return staticShortcutLabels[id] + } + + /// Clear the shortcut bound to `id` and let the normal preference-change pipeline reconcile the + /// registry and UI. Used by the conflict dialog's "Unassign existing shortcut and continue" for a + /// shortcut that may not be on screen, so it goes through `Preferences` (cached UserDefaults) + /// rather than mutating a live recorder. If that recorder happens to be displayed, sync it too. + /// + /// For a numbered shortcut's Trigger the hold can't stand alone, so "unassign" clears the "and + /// press" (nextWindowShortcut) part — e.g. ⌥+Tab becomes ⌥+(unassigned) — whether the conflict + /// was reported against the hold or the press. + static func unassignShortcut(_ id: String) { + let keyToClear = (id.hasPrefix("holdShortcut") || id.hasPrefix("nextWindowShortcut")) + ? Preferences.indexToName("nextWindowShortcut", Preferences.nameToIndex(id)) + : id + Preferences.setShortcut(keyToClear, nil) + shortcutControls[keyToClear]?.0.objectValue = nil } private static func shouldClearConflictingShortcuts(_ conflicts: [String], _ messageFormat: String) -> Bool { diff --git a/src/preferences/settings-window/tabs/controls/ShortcutEditor.swift b/src/preferences/settings-window/tabs/controls/ShortcutEditor.swift index 87762e36..4ae49f7f 100644 --- a/src/preferences/settings-window/tabs/controls/ShortcutEditor.swift +++ b/src/preferences/settings-window/tabs/controls/ShortcutEditor.swift @@ -220,8 +220,7 @@ final class TriggerBinding { private let andPressLabel: NSTextField private static let holdLabelText = NSLocalizedString("Hold", comment: "") private static let andPressText = NSLocalizedString("and press", comment: "") - // Not shown as a trigger-row label anymore; kept as the next-window recorder's human-readable - // name in `shortcutControls`, used by ControlsTab's conflict dialogs (see `conflictLabel`). + // The next-window recorder's human-readable name, stored alongside it in `shortcutControls`. private static let selectNextText = NSLocalizedString("Select next window", comment: "") private var currentIndex: Int = -1 diff --git a/src/preferences/settings-window/tabs/controls/ShortcutsWhenActiveSheet.swift b/src/preferences/settings-window/tabs/controls/ShortcutsWhenActiveSheet.swift index 538daf9b..f2ec7982 100644 --- a/src/preferences/settings-window/tabs/controls/ShortcutsWhenActiveSheet.swift +++ b/src/preferences/settings-window/tabs/controls/ShortcutsWhenActiveSheet.swift @@ -2,16 +2,18 @@ import Cocoa class ShortcutsWhenActiveSheet: SheetWindow { private static let title = NSLocalizedString("Shortcuts When Active", comment: "") - private static let labelFocus = NSLocalizedString("Focus selected window", comment: "") - private static let labelPrevious = NSLocalizedString("Select previous window", comment: "") - private static let labelCancel = NSLocalizedString("Cancel", comment: "") - private static let labelSearch = NSLocalizedString("Search", comment: "") - private static let labelLockSearch = NSLocalizedString("Lock search", comment: "") - private static let labelClose = NSLocalizedString("Close window", comment: "") - private static let labelMinDemin = NSLocalizedString("Minimize/Deminimize window", comment: "") - private static let labelFullscreen = NSLocalizedString("Fullscreen/Defullscreen window", comment: "") - private static let labelQuit = NSLocalizedString("Quit app", comment: "") - private static let labelHideShow = NSLocalizedString("Hide/Show app", comment: "") + // Row titles come from `ControlsTab.staticShortcutLabels` (single source of truth, also used by + // the conflict dialog to name these actions). + private static let labelFocus = ControlsTab.staticShortcutLabels["focusWindowShortcut"]! + private static let labelPrevious = ControlsTab.staticShortcutLabels["previousWindowShortcut"]! + private static let labelCancel = ControlsTab.staticShortcutLabels["cancelShortcut"]! + private static let labelSearch = ControlsTab.staticShortcutLabels["searchShortcut"]! + private static let labelLockSearch = ControlsTab.staticShortcutLabels["lockSearchShortcut"]! + private static let labelClose = ControlsTab.staticShortcutLabels["closeWindowShortcut"]! + private static let labelMinDemin = ControlsTab.staticShortcutLabels["minDeminWindowShortcut"]! + private static let labelFullscreen = ControlsTab.staticShortcutLabels["toggleFullscreenWindowShortcut"]! + private static let labelQuit = ControlsTab.staticShortcutLabels["quitAppShortcut"]! + private static let labelHideShow = ControlsTab.staticShortcutLabels["hideShowAppShortcut"]! /// Pre-build search index for the open-button. See `SettingsSearchIndex.sheetSearchableStrings`. /// `ProBadgeView.proLabel` contributes the "Pro" tag rendered on the search/lock-search rows.