From 339aeaabe63a49f6de96ee8aa4e9cd73f525bb71 Mon Sep 17 00:00:00 2001 From: Louis Pontoise Date: Sat, 13 Mar 2021 23:35:25 +0900 Subject: [PATCH] fix: more robust handling of custom shortcuts * user was able to set a nextWindowShortcut which contained modifiers from holdShortcut * detection of conflicts was incorrect in some cases, mostly because holdShortcut and nextWindowShortcut should be treated as 1 shortcut * after the user forces a shortcut to apply, resetting the existing one with the same keys, it wouldn't save to UserDefaults * forcing the holdShortcut to replace another holdShortcut would reset it, even though holdShortcut should always be set with some value --- src/logic/Preferences.swift | 32 +++++++++---- src/logic/events/KeyboardEvents.swift | 4 +- .../CustomRecorderControl.swift | 41 ++++++++++++++-- .../preferences-window/tabs/ControlsTab.swift | 47 ++++++++++++------- 4 files changed, 91 insertions(+), 33 deletions(-) diff --git a/src/logic/Preferences.swift b/src/logic/Preferences.swift index d9d88ad4..4e38a195 100644 --- a/src/logic/Preferences.swift +++ b/src/logic/Preferences.swift @@ -171,17 +171,33 @@ class Preferences { } private static func updateToNewPreferences(_ preferencesVersion: String) { - // dropdowns preferences used to store English text; now they store indexes - migrateDropdownsFromTextToIndexes() - // the "Hide menubar icon" checkbox was replaced with a dropdown of: icon1, icon2, hidden - migrateMenubarIconFromCheckboxToDropdown() - // "Show minimized/hidden/fullscreen windows" checkboxes were replaced with dropdowns - migrateShowWindowsCheckboxToDropdown() - // "Max size on screen" was split into max width and max height - migrateMaxSizeOnScreenToWidthAndHeight() + if App.version.compare("6.3.0", options: .numeric) == .orderedAscending { + // dropdowns preferences used to store English text; now they store indexes + migrateDropdownsFromTextToIndexes() + // the "Hide menubar icon" checkbox was replaced with a dropdown of: icon1, icon2, hidden + migrateMenubarIconFromCheckboxToDropdown() + // "Show minimized/hidden/fullscreen windows" checkboxes were replaced with dropdowns + migrateShowWindowsCheckboxToDropdown() + // "Max size on screen" was split into max width and max height + migrateMaxSizeOnScreenToWidthAndHeight() + } + // nextWindowShortcut used to be able to have modifiers already present in holdShortcut; we remove these + migrateNextWindowShortcuts() defaults.set(App.version, forKey: preferencesVersion) } + private static func migrateNextWindowShortcuts() { + ["", "2"].forEach { suffix in + if let oldHoldShortcut = defaults.string(forKey: "holdShortcut" + suffix), + let oldNextWindowShortcut = defaults.string(forKey: "nextWindowShortcut" + suffix) { + let nextWindowShortcutCleanedUp = oldHoldShortcut.reduce(oldNextWindowShortcut, { $0.replacingOccurrences(of: String($1), with: "") }) + if oldNextWindowShortcut != nextWindowShortcutCleanedUp { + defaults.set(nextWindowShortcutCleanedUp, forKey: "nextWindowShortcut" + suffix) + } + } + } + } + private static func migrateMaxSizeOnScreenToWidthAndHeight() { if let old = defaults.string(forKey: "maxScreenUsage") { defaults.set(old, forKey: "maxWidthOnScreen") diff --git a/src/logic/events/KeyboardEvents.swift b/src/logic/events/KeyboardEvents.swift index 08d6d56e..129bea79 100644 --- a/src/logic/events/KeyboardEvents.swift +++ b/src/logic/events/KeyboardEvents.swift @@ -39,8 +39,8 @@ class KeyboardEvents { if shortcut.keyCode != .none { let id = globalShortcutsIds[controlId]! let hotkeyId = EventHotKeyID(signature: signature, id: UInt32(id)) - let key = UInt32(shortcut.carbonKeyCode) - let mods = UInt32(shortcut.carbonModifierFlags) + let key = shortcut.carbonKeyCode + let mods = shortcut.carbonModifierFlags let options = UInt32(kEventHotKeyNoOptions) var shortcutsReference: EventHotKeyRef? RegisterEventHotKey(key, mods, hotkeyId, shortcutEventTarget, options, &shortcutsReference) diff --git a/src/ui/generic-components/CustomRecorderControl.swift b/src/ui/generic-components/CustomRecorderControl.swift index f02fcee4..34e0fc2e 100644 --- a/src/ui/generic-components/CustomRecorderControl.swift +++ b/src/ui/generic-components/CustomRecorderControl.swift @@ -1,6 +1,8 @@ import Cocoa import ShortcutRecorder +let allowedModifiers = NSEvent.ModifierFlags(arrayLiteral: [.command, .control, .option, .shift]) + class CustomRecorderControl: RecorderControl, RecorderControlDelegate { var clearable: Bool! var id: String! @@ -14,7 +16,7 @@ class CustomRecorderControl: RecorderControl, RecorderControlDelegate { allowsEscapeToCancelRecording = false allowsDeleteToClearShortcutAndEndRecording = false allowsModifierFlagsOnlyShortcut = true - set(allowedModifierFlags: CocoaModifierFlagsMask, requiredModifierFlags: [], allowsEmptyModifierFlags: true) + restrictModifiers([]) objectValue = Shortcut(keyEquivalent: shortcutString) widthAnchor.constraint(equalToConstant: 100).isActive = true } @@ -31,6 +33,10 @@ class CustomRecorderControl: RecorderControl, RecorderControlDelegate { } } + func restrictModifiers(_ restrictedModifiers: NSEvent.ModifierFlags) { + set(allowedModifierFlags: allowedModifiers.subtracting(restrictedModifiers), requiredModifierFlags: [], allowsEmptyModifierFlags: true) + } + // only allow modifiers: ⌥ -> valid, e -> invalid, ⌥e -> invalid func recorderControl(_ control: RecorderControl, canRecord shortcut: Shortcut) -> Bool { if !clearable && shortcut.keyCode != .none { @@ -42,11 +48,29 @@ class CustomRecorderControl: RecorderControl, RecorderControlDelegate { func alertIfSameShortcutAlreadyAssigned(_ shortcut: Shortcut) { if let shortcutAlreadyAssigned = (ControlsTab.shortcuts.values.first { - if $0.id == id || id.starts(with: "holdShortcut") || $0.id.starts(with: "holdShortcut") { + if id == $0.id { return false } + if (id.starts(with: "holdShortcut") && $0.id.starts(with: "holdShortcut") || (id.starts(with: "nextWindowShortcut")) && $0.id.starts(with: "nextWindowShortcut")) { + let index = id.last == "2" ? 1 : 0 + let otherIndex = id.last == "2" ? 0 : 1 + let otherSuffix = id.last == "2" ? "" : "2" + if id.starts(with: "holdShortcut") { + return Preferences.nextWindowShortcut[index] == Preferences.nextWindowShortcut[otherIndex] && + shortcut.modifierFlags == ControlsTab.shortcutControls["holdShortcut" + otherSuffix]!.0.objectValue!.modifierFlags + } + if id.starts(with: "nextWindowShortcut") { + if let nextWindowShortcut = ControlsTab.shortcutControls["nextWindowShortcut" + otherSuffix]?.0.objectValue { + return Preferences.holdShortcut[index] == Preferences.holdShortcut[otherIndex] && + shortcut.modifierFlags == nextWindowShortcut.modifierFlags && + shortcut.keyCode == nextWindowShortcut.keyCode + } + return false + } + } if $0.id.starts(with: "nextWindowShortcut") { - return $0.shortcut.keyCode == shortcut.keyCode + let suffix = $0.id.last == "2" ? "2" : "" + return $0.shortcut.keyCode == shortcut.keyCode && ($0.shortcut.carbonModifierFlags ^ ControlsTab.shortcutControls["holdShortcut" + suffix]!.0.objectValue!.carbonModifierFlags) == shortcut.carbonModifierFlags } return $0.shortcut.keyCode == shortcut.keyCode && $0.shortcut.modifierFlags == shortcut.modifierFlags }) { @@ -55,15 +79,22 @@ class CustomRecorderControl: RecorderControl, RecorderControlDelegate { alert.alertStyle = .critical alert.messageText = NSLocalizedString("Conflicting shortcut", comment: "") alert.informativeText = String(format: NSLocalizedString("Shortcut already assigned to another action: %@", comment: ""), existing.1.replacingOccurrences(of: " ", with: "\u{00A0}")) - alert.addButton(withTitle: NSLocalizedString("Unassign existing shortcut and continue", comment: "")).setAccessibilityFocused(true) + if !id.starts(with: "holdShortcut") { + 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() - if userChoice == .alertFirstButtonReturn { + if !id.starts(with: "holdShortcut") && userChoice == .alertFirstButtonReturn { existing.0.objectValue = nil ControlsTab.shortcutChangedCallback(existing.0) + LabelAndControl.controlWasChanged(existing.0, shortcutAlreadyAssigned.id) ControlsTab.shortcutControls[id]!.0.objectValue = shortcut ControlsTab.shortcutChangedCallback(self) + LabelAndControl.controlWasChanged(self, id) } } } diff --git a/src/ui/preferences-window/tabs/ControlsTab.swift b/src/ui/preferences-window/tabs/ControlsTab.swift index bcb33b15..940627e5 100644 --- a/src/ui/preferences-window/tabs/ControlsTab.swift +++ b/src/ui/preferences-window/tabs/ControlsTab.swift @@ -2,7 +2,6 @@ import Cocoa import ShortcutRecorder class ControlsTab { - static var nextWindowShortcut: [NSControl]! static var shortcuts = [String: ATShortcut]() static var shortcutControls = [String: (CustomRecorderControl, String)]() static var shortcutsActions = [ @@ -37,12 +36,14 @@ class ControlsTab { let checkboxes = StackView([StackView(enableArrows), StackView(enableMouse)], .vertical) let shortcuts = StackView([focusWindowShortcut, previousWindowShortcut, cancelShortcut, closeWindowShortcut, minDeminWindowShortcut, quitAppShortcut, hideShowAppShortcut].map { (view: [NSView]) in StackView(view) }, .vertical) let orPress = LabelAndControl.makeLabel(NSLocalizedString("While open, press:", comment: ""), shouldFit: false) - let (nextWindowShortcut, tab1View) = toShowSection("") - let (nextWindowShortcut2, tab2View) = toShowSection("2") + let (holdShortcut, nextWindowShortcut, tab1View) = toShowSection("") + let (holdShortcut2, nextWindowShortcut2, tab2View) = toShowSection("2") let tabView = TabView([(NSLocalizedString("Shortcut 1", comment: ""), tab1View), (NSLocalizedString("Shortcut 2", comment: ""), tab2View)]) - ControlsTab.nextWindowShortcut = [nextWindowShortcut, nextWindowShortcut2].map { $0[0] as! NSControl } ControlsTab.arrowKeysEnabledCallback(enableArrows[0] as! NSControl) + // trigger shortcutChanged for these shortcuts to trigger .restrictModifiers + [holdShortcut, holdShortcut2].forEach { ControlsTab.shortcutChangedCallback($0[1] as! NSControl) } + [nextWindowShortcut, nextWindowShortcut2].forEach { ControlsTab.shortcutChangedCallback($0[0] as! NSControl) } let grid = GridView([ [tabView], @@ -67,7 +68,7 @@ class ControlsTab { return grid } - private static func toShowSection(_ postfix: String) -> ([NSView], GridView) { + private static func toShowSection(_ postfix: String) -> ([NSView], [NSView], GridView) { let toShowExplanations = LabelAndControl.makeLabel(NSLocalizedString("Show windows from:", comment: "")) let toShowExplanations2 = LabelAndControl.makeLabel(NSLocalizedString("Minimized windows:", comment: "")) let toShowExplanations3 = LabelAndControl.makeLabel(NSLocalizedString("Hidden windows:", comment: "")) @@ -100,7 +101,7 @@ class ControlsTab { tab.column(at: 0).xPlacement = .trailing tab.mergeCells(inHorizontalRange: NSRange(location: 0, length: 2), verticalRange: NSRange(location: 4, length: 1)) tab.fit() - return (nextWindowShortcut, tab) + return (holdShortcut, nextWindowShortcut, tab) } private static func addShortcut(_ triggerPhase: ShortcutTriggerPhase, _ scope: ShortcutScope, _ shortcut: Shortcut, _ controlId: String, _ index: Int?) { @@ -129,30 +130,40 @@ class ControlsTab { if controlId.hasPrefix("holdShortcut") { let i = controlId == "holdShortcut" ? 0 : 1 addShortcut(.up, .global, Shortcut(keyEquivalent: Preferences.holdShortcut[i])!, controlId, i) - if let s = nextWindowShortcut?[i] { - shortcutChangedCallback(s) + if let nextWindowShortcut = shortcutControls["nextWindowShortcut" + (i == 0 ? "" : "2")]?.0 { + nextWindowShortcut.restrictModifiers([(sender as! CustomRecorderControl).objectValue!.modifierFlags]) + shortcutChangedCallback(nextWindowShortcut) } } else { - let newValue = shortcutStringValue(controlId, sender) + let newValue = combineHoldAndNextWindow(controlId, sender) if newValue.isEmpty { removeShortcutIfExists(controlId) + restrictModifiersOfHoldShortcut(controlId, []) } else { let i = controlId.hasPrefix("nextWindowShortcut") ? (controlId == "nextWindowShortcut" ? 0 : 1) : nil addShortcut(.down, controlId.hasPrefix("nextWindowShortcut") ? .global : .local, Shortcut(keyEquivalent: newValue)!, controlId, i) + restrictModifiersOfHoldShortcut(controlId, [(sender as! CustomRecorderControl).objectValue!.modifierFlags]) } } } - static func shortcutStringValue(_ controlId: String, _ sender: NSControl) -> String { - let baseValue = (sender as! RecorderControl).stringValue - if KeyboardEvents.globalShortcutsIds[controlId] != nil { - let holdShortcut = controlId == "nextWindowShortcut" ? Preferences.holdShortcut[0] : Preferences.holdShortcut[1] - // remove the holdShortcut character in case they also use it in the other shortcuts - let cleanedShortcut = holdShortcut + holdShortcut.reduce(baseValue, { $0.replacingOccurrences(of: String($1), with: "") }) - if cleanedShortcut.sorted() == holdShortcut.sorted() { - return "" + private static func restrictModifiersOfHoldShortcut(_ controlId: String, _ modifiers: NSEvent.ModifierFlags) { + if controlId.hasPrefix("nextWindowShortcut") { + let i = controlId == "nextWindowShortcut" ? "" : "2" + if let holdShortcut = shortcutControls["holdShortcut" + i]?.0 { + holdShortcut.restrictModifiers(modifiers) } - return cleanedShortcut + } + } + + static func combineHoldAndNextWindow(_ controlId: String, _ sender: NSControl) -> String { + let baseValue = (sender as! RecorderControl).stringValue + if baseValue == "" { + return "" + } + if controlId.starts(with: "nextWindowShortcut") { + let holdShortcut = controlId.last == "2" ? Preferences.holdShortcut[1] : Preferences.holdShortcut[0] + return holdShortcut + baseValue } return baseValue }