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

@@ -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")

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)

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)
             }
         }
     }

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
     }