Add repeated-press logic to cycle popup selection

[alexisgaziello]

Hey!

I was hoping to contribute to Maccy.

I'd like to add an option to cycle over the clipboard option on repeated "open" shortcut stroke.

Before merge, there are probably a couple of things needed to be done, such as adding tests.

I was looking to get your thoughts before spending more time on it. Couple of questions:

• Would you like to get this feature merged?
• Do you use any formatter/linter? (It's my first time working with Swift/Xcode) I tried using swiftformat and most of the files changed.
• Does the current logic look OK?
• Any general requirements to create a PR/commit?

[p0deje]

Thank you for the pull request!

Would you like to get this feature merged?
Does the current logic look OK?

I very much like the idea of cycle hotkey, but I believe it should be implemented as the oldest open issue in Maccy (#96) requests:

1. You press COMMAND+SHIFT+C (or other shortcut) to popup Maccy.
2. You release С but keep COMMAND+SHIFT pressed.
3. You press С as many times as you need. Each press navigates to the next item in history.
4. You release COMMAND+SHIFT which confirms the current selection and copies it.
5. Maccy window is now closed.

Unfortunately, I don't think it's possible to implement this using KeyboardShortcuts framework, but if you are up to investigate it and see if there is a way to trigger shortcut while keeping modifier keys pressed, it would make things much easier.

Do you use any formatter/linter? (It's my first time working with Swift/Xcode) I tried using swiftformat and most of the files changed.

I usually just use swiftlint --strict and fix all warnings.

Any general requirements to create a PR/commit?

Nope, just the code that works 😄

[alexisgaziello]

I see, releasing CMD + Shift doesn't confirm the selection. You have to manually press enter. Now that I have been using it for a bit, indeed that seems necessary. let me see if I can add it

[alexisgaziello]

Ill work on this during the next couple of weeks. I havent got a lot of time during these Christmas. Right now the main logic looks like:

  init() {
    if Defaults[.cyclePopUp] {
      self.cycleSelection()
    } else {
      self.toggle()  // todo: keep old logic?
    }
  }
  
  private func cycleSelection() {
    guard let popupShortcut = KeyboardShortcuts.getShortcut(for: .popup) else { return }
    
    let keyCode = popupShortcut.carbonKeyCode
    let requiredModifiers = popupShortcut.modifiers

    // Event monitor when the app is not open. On keyshortcut press, open the app
    globalDownEvent = NSEvent.addGlobalMonitorForEvents(matching: [.keyDown]) {[weak self] event in
      guard let self = self else { return }
      if event.keyCode == keyCode && event.modifierFlags.intersection(.deviceIndependentFlagsMask) == requiredModifiers {
        self.open(height: self.height)
      }
    }
    
    // Event monitor when the app is open. On keyshortcut press, select next item
    localDownEvent = NSEvent.addLocalMonitorForEvents(matching: [.keyDown]) {event in
      if event.keyCode == keyCode && event.modifierFlags.intersection(.deviceIndependentFlagsMask) == requiredModifiers {
        AppState.shared.highlightNext()
        return nil
      }
      return event
    }
    
    // Event monitor when the app is open. On keyshortcut release, close the app
    //  .flagsChanged is required for the case when the user first releases the letter, and later the modifiers
    localUpEvent = NSEvent.addLocalMonitorForEvents(matching: [.keyUp, .flagsChanged]) {[weak self] event in
      guard let self = self else { return event }
      let stillHoldingModifiers = event.modifierFlags.intersection(.deviceIndependentFlagsMask) == requiredModifiers
      if self.isOpen() && !stillHoldingModifiers {
        Task { @MainActor in
          AppState.shared.select()
        }
        return nil
      }
      return event
    }
  }

There are a couple of things that still need polishing + I want to test it for a bit.

[p0deje]

I think your localDownEvent should only be triggered when COMMAND+SHIFT was kept held, right now it would be called when pressed again.

[alexisgaziello]

What do you mean? Pressing the key again indeed calls highlightNext, which is the objective, to cycle over the options.

To make sure CMD+SHIFT are pressed I have: event.modifierFlags.intersection(.deviceIndependentFlagsMask) == requiredModifiers.

---

I have extracted all of this into a separate PopupManager class, which keeps current Popup class clean.

import Foundation
import AppKit
import Cocoa
import Defaults
import KeyboardShortcuts
import Observation

// MARK: - PopupAction Protocol

/// A protocol representing an action that can be performed on a popup.
protocol PopupAction {
  /// Initializes the popup action.
  init()
}

// MARK: - PopupManager

/// Manages the current popup action, allowing it to be reset based on user preferences.
final class PopupManager {
  
  /// The current popup action. Read-only from outside the class.
  private(set) var action: PopupAction
  private var shouldCycle: Bool
  
  /// Initializes the PopupManager with the specified cycling behavior.
  init(shouldCycle: Bool = Defaults[.cyclePopUp]) {
    self.shouldCycle = shouldCycle
    self.action = PopupManager.createAction(shouldCycle: shouldCycle)
  }
  
  /// Resets the current action
  /// - Parameter shouldCycle: A Boolean indicating whether cycling should be enabled.
  public func reset(shouldCycle: Bool? = nil) {
    let cycleValue = shouldCycle ?? self.shouldCycle
    self.shouldCycle = cycleValue
    self.action = PopupManager.createAction(shouldCycle: cycleValue)
  }
  
  /// Factory method to create the appropriate `PopupAction` based on `shouldCycle`.
  /// - Parameter shouldCycle: A Boolean indicating whether cycling should be enabled.
  /// - Returns: An instance of `PopupCycle` or `PopupToggle`.
  private static func createAction(shouldCycle: Bool) -> PopupAction {
    return shouldCycle ? PopupCycle() : PopupToggle()
  }
}

// MARK: - PopupCycle

/// Represents a popup action that cycles through clipboard history items.
final class PopupCycle: PopupAction {
  
  // Event monitors
  private var globalDownEvent: Any?
  private var localDownEvent: Any?
  private var localUpEvent: Any?
  
  required init() {
    guard let popupShortcut = KeyboardShortcuts.getShortcut(for: .popup) else { return }
    
    let keyCode = popupShortcut.carbonKeyCode
    let modifiers = popupShortcut.modifiers
    
    // Event monitor when the app is not open. On keyshortcut press, open the app
    globalDownEvent = NSEvent.addGlobalMonitorForEvents(matching: [.keyDown]) { event in
      if Self.isKeyCode(event: event, keyCode: keyCode) && Self.isModifiers(event: event, modifiers: modifiers) {
        DispatchQueue.main.async {
          AppState.shared.popup.open(height: AppState.shared.popup.height)
        }
      }
    }
    
    // Event monitor when the app is open. On keyshortcut press, select next item
    localDownEvent = NSEvent.addLocalMonitorForEvents(matching: [.keyDown]) { event in
      if Self.isKeyCode(event: event, keyCode: keyCode) && Self.isModifiers(event: event, modifiers: modifiers) {
        DispatchQueue.main.async {
          AppState.shared.highlightNext()
        }
        return nil
      }
      return event
    }
    
    // Event monitor when the app is open. On keyshortcut release, close the app and restore focus.
    //  '.flagsChanged' is required for the case when the user first releases the letter, and later the modifiers
    localUpEvent = NSEvent.addLocalMonitorForEvents(matching: [.keyUp, .flagsChanged]) { event in
      if AppState.shared.popup.isOpen() && !Self.isModifiers(event: event, modifiers: modifiers) {
        DispatchQueue.main.async {
          AppState.shared.select()
        }
        return nil
      }
      return event
    }
  }
  
  /// Removes all event monitors.
  deinit {
    if let monitor = globalDownEvent {
      NSEvent.removeMonitor(monitor)
    }
    
    if let monitor = localDownEvent {
      NSEvent.removeMonitor(monitor)
    }
    
    if let monitor = localUpEvent {
      NSEvent.removeMonitor(monitor)
    }
  }
  
  private static func isKeyCode(event: NSEvent, keyCode: Int) -> Bool {
    return event.keyCode == keyCode
  }
  
  private static func isModifiers(event: NSEvent, modifiers: NSEvent.ModifierFlags) -> Bool {
    return event.modifierFlags.intersection(.deviceIndependentFlagsMask) == modifiers
  }
}

// MARK: - PopupToggle

/// Represents a popup action that toggles the popup's visibility.
final class PopupToggle: PopupAction {
  
  required init() {
    KeyboardShortcuts.onKeyDown(for: .popup) {
      DispatchQueue.main.async {
        AppState.shared.popup.toggle()
      }
    }
  }
  
  deinit {
    KeyboardShortcuts.disable(.popup)
  }
}

[p0deje]

To make sure CMD+SHIFT are pressed I have: event.modifierFlags.intersection(.deviceIndependentFlagsMask) == requiredModifiers.

What I mean is that there were 2 cases when this code would be executed:

1. A user presses shortcut (Command+Shift+C) to open Maccy, keeps modifiers pressed and releases С key, then presses С again.
2. A user presses shortcut (Command+Shift+C) to open Maccy, *releases all keys, then presses shortcut again.

The behavior should be different for these two scenarios:

1. Each press of С should select the next item.
2. The shortcut press should close Maccy window.

[alexisgaziello]

A user presses shortcut (Command+Shift+C) to open Maccy, keeps modifiers pressed and releases С key, then presses С again.

Each press of С should select the next item.

This is what happens

A user presses shortcut (Command+Shift+C) to open Maccy, *releases all keys, then presses shortcut again.

Here, what would happen is:

1. A user presses shortcut (Command+Shift+C) -> Maccy opens
2. releases all keys -> Maccy closes
3. then presses shortcut again -> maccy opens

The shortcut press should close Maccy window.

I apologize, but I am not sure I am understanding your point. which shortcut press, only the second one? Maccy shouldnt close on release?

Right now the logic is as follows:

• You press COMMAND+SHIFT+C (or other shortcut) to open Maccy.
• Once Maccy is open:
  • Releasing any modifier key, in this case, COMMAND or SHIFT, will close Maccy.
  • Releasing the main key, in this case С, will not do anything.
  • Pressing C again will select the next item in the popup.

See the video attached -

mov.mp4

[p0deje]

Thanks for recording the video and writing it down, I guess I was misreading the code.

A user presses shortcut (Command+Shift+C) -> Maccy opens
releases all keys -> Maccy closes

I can imagine people releasing modifiers milliseconds later than "C" and they would be confused that Maccy suddenly closed. Instead, releasing the modifier keys should not close Maccy unless the user presses "C" again. Pressing "C" would put Maccy into a new mode (let's call it "cycle mode") which would close as soon as the modifier keys are
released.

[alexisgaziello]

Hey! Finally wrote those tests. I got some issues but finally got it resolved. It looks like they are still failing in the CI though. They pass on my local computer. Do you know what could be happening?

In terms of the app, I'm pretty happy with the result. Please pull the code and try the new feature.

If the feature is fine, feel free to also review the code.

[p0deje]

I just tried pulling your PR but I cannot make the cycle work. I press command+shift+c, keep command+shift held, then press "c" and Maccy window closes.

[alexisgaziello]

Yeah! After some thought, I ended up setting the default shortcut to CMD + SHIFT + V for this new feature.

The reasons being:

• Code was becoming unnecessarily complex
• Some users may want to keep both the classic and the new feature available at the same time
• If you prefer to remove the old feature and replace it with the new one, simply remove the old shortcut for toggling and add CMD + SHIFT + C to the cycle shortcut in your preferences

[p0deje]

Ok, I see. I think that the implementation should work with the same shortcut as popup (default CMD+SHIFT+C) and the following main scenarios:

1. User presses the popup shortcut, releases all keys -> Maccy is still open.
2. User presses the popup shortcut, releases all keys, presses again -> Maccy closes.
3. User presses the popup shortcut, holds modifiers, presses C again -> Maccy cycles to next item.
4. User presses the popup shortcut, holds modifiers, presses C again, releases modifier keys -> Maccy selects an active item.

[alexisgaziello]

I tried implementing both in the same shortcut as you describe, but it didn't work.

When I listen to the key up event to differentiate between 2 & 3, modifiers are always active, whether you release them together with C or not.

I was investigating further and something that works is dispatching a task >.1 second after the keyUp event, and refetch the state of the modifiers via CGEventSource.flagsState.

Note that this will complicate the logic & state quite a bit. Ill submit a separate PR to have both options available and see the differences.

[alexisgaziello]

New option: #1013

I still have to cleanup, add tests, and add logic for enabling/disabling cycle mode, but now you should be able to pull and assert that Maccy behaves as you described:

1. User presses the popup shortcut, releases all keys -> Maccy is still open.
2. User presses the popup shortcut, releases all keys, presses again -> Maccy closes.
3. User presses the popup shortcut, holds modifiers, presses C again -> Maccy cycles to next item.
4. User presses the popup shortcut, holds modifiers, presses C again, releases modifier keys -> Maccy selects an active item.

[p0deje]

I like the new option, it works pretty much as I think it should be! There is only one small issue I've noticed, let me describe the desired behavior:

5. User presses the popup shortcut, holds modifiers for a second, releases modifiers -> Maccy is still option.

[alexisgaziello]

User presses the popup shortcut, holds modifiers for a second, releases modifiers -> Maccy is still option.

I think you want to tune the behavior for the line:

DispatchQueue.main.asyncAfter(deadline: .now() + 0.2, execute: task)

Can you pull the last version that I just updated in #1013 now and set

DispatchQueue.main.asyncAfter(deadline: .now() + 1, execute: task) instead?

I personally prefer 0.2 - 1 second is too much IMO. But we could set a preference for that.

Also, I told you the line of code instead of pushing a change, in case you want to play around with that timer.

[p0deje]

The implementation should not depend on the passed time, the cycle mode should only be activated when both conditions are met:

1. user kept holding modifiers;
2. user pressed "C" key again.

If at least one of the conditions is not met, releasing modifiers must not close the Maccy window.

[alexisgaziello]

Oh I understand now, essentially the behavior is decided on whether there is a second press or not, I got confused by the 1s.

I think the latest push on #1013 should have the behavior you are looking for.

[p0deje]

Yes, my brief testing shows that it works correctly, thank you!
Shall I start reviewing the PR or you want to make any other changes?

[alexisgaziello]

Could you help me define the requirements? (and avoid too much back and forth)

I initially thought that we could add an option to disable/enable cycling mode, but I am not sure it's necessary, given that 1, original behaviour remains untouched (fully backward compatible) 2, it's difficult to trigger cycle mode unintentionally, and 3, we will just complicate the codebase (simple == better)

So if you are OK leaving this new behavior as default and only option, I just need to update the tests and clean up a bit.

[p0deje]

There should be no extra configuration for that, this behavior should be enabled by default. Nothing will change for users unless they hold CMD+SHIFT and press C again (or another shortcut they use).

Regarding tests, I suggest you add a simple 1 test to existing MaccyUITests that cycles to the next item and releases modifiers.

[alexisgaziello]

Sounds good. I added a couple of tests, but in a separate file. IMO MaccyUITests is getting too big, and might get a bit difficult to manage. I think a single file with all the tests of the feature in a single place gives more structure and organization to the repo.

If you dont like these changes I can just add a test in MaccyUITests as you suggested

Ready for review: #1016