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

Use ctrl and remove alt usage with certain hotkeys for mac shortcuts #526

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

Conversation

mapringg
Copy link

@mapringg mapringg commented May 6, 2024

Fix broken shortcuts on mac devices. All hotkeys are now working properly. Before merging, I'd love to get feedback on this implementation. Please review and provide your thoughts on the adjustment I made.

Copy link

vercel bot commented May 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
big-agi-open ✅ Ready (Inspect) Visit Preview May 6, 2024 9:40am

@enricoros
Copy link
Owner

Fix broken shortcuts on mac devices. All hotkeys are now working properly. Before merging, I'd love to get feedback on this implementation. Please review and provide your thoughts on the adjustment I made.

Thanks! I love it that you followed up on this. Indeed shortcuts have been broken for the longest time on Mac.

Feedback, as requested:

  • The new implementation uses only Ctrl and not Cmd; before I thought that on Mac the "Cmd" (⌘) key would be go-to, and Ctrl is not used (someone commented on that before). From a user perspective, on web applications and desktop applications, what would be the most reasonable shortcuts (for opening a new chat for instance)?

  • The checks for "isMacUser ? false : true" can be replaced with "!isMacUser", for brevity, but not required

  • I wonder if there's a way to not change the shortcuts definitions, but simply ignore all the "Alt" (option) keys on Mac instead; in that case, the changes to AppChat may not be needed, simply the 'alt' flag would have 'altOnPC" semantics

  • There is another function, for single-shortcut handling in useGlobalShortcuts, and may get the same treatment as the multi-shortcuts function, as it has the 'isCtrlOrCmd' treatment. Options could be: 1. actually remove the single-shortcut function and replace it with the multi-shortcuts function where needed (as the two functions do the same, which is a duplication), 2. update the single-shortcut function too

In essence, please let me know if the new shortcuts on Mac are consistent with the user expectations for opening a new chat in other web/apps, and if the requirement for Alt (Option) can be completely dropped on Mac, in which case, it can be checked at the shortcuts function, and no need to replace elsewhere.

@enricoros
Copy link
Owner

@mapringg lmk if you got the feedback and your answers, so I know how to merge

@mapringg
Copy link
Author

  • On Mac, the Cmd key is often a suitable choice for shortcuts. However, within a browser, it conflicts with numerous existing shortcuts. To maintain consistency across platforms, I propose adjusting the modifier key instead of changing the shortcut keys themselves. Specifically, replacing "Cmd" with "Ctrl" achieves this goal.

  • I think it is possible to ignore all the Alt keys only for Mac users.

  • I prefer replacing useGlobalShortcut with useGlobalShortcuts to avoid code duplication. Do you also wish for me to update the filename to reflect the change.

@enricoros
Copy link
Owner

@mapringg great directions, and approved (even file name change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants