Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false positives for isShortcutAlreadyAssigned #3826

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phazei
Copy link

@phazei phazei commented Nov 8, 2024

(closes #3288)

The issue is described in detail in the issue. This resolves that issue, it was tested as working on a 2024 MBP running Sonoma 14.6.1.

@lwouis
Copy link
Owner

lwouis commented Nov 8, 2024

I did some QA. Here are some issues I've found (S1 = Shortcut1):

  • Set S1 to (alt)+(shift); Set S2 to (alt+shift)+(cmd) => It lets me set it, even though S1 will break S2. That is, when I release cmd, instead of not doing anything and continue showing me S1 windows, it thinks I'm not holding S1 and cycle through S1 windows.
  • Set S1 to (alt)+(tab) and S2 to (alt+shift)+(tab). Press S2, then keep tab pressed and release shift => it closes instead of switching to S1

The possible scenarios are extremely complex once we allow for some shortcuts to be part of others, when combining the 2 parts of the global shortcuts.

It would be nice to list scenarios which we want, and scenarios we don't want. This way we can facilitate QA and ensure that we don't regress on cases when we fix other cases.

@phazei
Copy link
Author

phazei commented Nov 12, 2024

So, I can set some of those shortcuts as you mention doing them backwards (setting S2 first) as it is now, which is also the workaround for the original ticket which would otherwise disallow completely valid shortcuts.

So, as long as it doesn't totally break things, the question is, to allow a user to not select some valid shortcuts, or let them inadvertently pick something that could be problematic. If a user chooses to select overlapping shortcuts, I'd personally say that's on them, it's better to allow valid shortcuts to be selected.

The current check could be handled better in a couple way. Perhaps should also check for "previousWindowShortcut" to check against that too. It didn't seem to have anything about that in there before so I didn't, but it wouldn't be hard to add. I would say that more limitations could potentially be placed on the "holdShortcut". First, since the "previousWindowShortcut" is a single keypress, I'd suggest that the "holdShortcut" shouldn't be allowed to include whatever key is used in "previousWindowShortcut". I'd also suggest that the "nextWindowShortcut" shouldn't be allowed to be a modifier key, which should be strictly for the hold, so no ctrl/opt/command/shift" on "nextWindowShortcut"

That partially is slightly counter to what I said about letting the user pick something problematic and it being their problem, but also, it seems to be within standard of how most OS shortcuts function.

What do you think?

@lwouis lwouis force-pushed the master branch 2 times, most recently from d0d2314 to 0b198b4 Compare December 1, 2024 21:18
@lwouis lwouis force-pushed the master branch 2 times, most recently from baa411d to 987b6b2 Compare December 8, 2024 11:31
@lwouis lwouis force-pushed the master branch 2 times, most recently from 18ef16f to e07da9c Compare December 26, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants