[macOS] Fix global shortcuts: F-key keycodes and headless event processing#39
[macOS] Fix global shortcuts: F-key keycodes and headless event processing#39
Conversation
…hread Co-authored-by: lijy91 <3889523+lijy91@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes macOS global shortcut handling by correcting function-key virtual keycode mapping and enabling hotkey event processing even when the app doesn’t run the main Cocoa event loop.
Changes:
- Replace incorrect arithmetic F-key mapping with an explicit F1–F20 keycode table.
- Start a dedicated background thread to drain/process Carbon hotkey events (no
[NSApp run]required). - Add mutex/atomic/thread lifecycle plumbing for the event thread.
Comments suppressed due to low confidence (1)
src/platform/macos/shortcut_manager_macos.mm:370
- With the new background event thread,
HandleHotKey()(and thusshortcut->Invoke()) can now run off the main thread.Shortcutis documented as not thread-safe, and itsenabled_flag/callback access are not synchronized, so this introduces potential data races and a behavior change for macOS consumers. Consider marshalling the activation + callback onto a single known thread (e.g., main run loop / a dedicated dispatcher thread) or adding synchronization/documentation so callers don’t assume main-thread invocation.
void HandleHotKey(ShortcutId shortcut_id) {
auto shortcut = manager_->Get(shortcut_id);
if (!shortcut) {
return;
}
if (!manager_->IsEnabled() || !shortcut->IsEnabled()) {
return;
}
manager_->EmitShortcutActivated(shortcut_id, shortcut->GetAccelerator());
shortcut->Invoke();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <string> | ||
| #include <thread> | ||
| #include <unordered_map> | ||
| #include <vector> |
There was a problem hiding this comment.
ParseAcceleratorMac uses std::size(kFKeyCodes) but this file doesn't include a header that guarantees std::size is declared (C++17 places it in <iterator>). This can fail to compile on some toolchains; include the appropriate header (e.g. <iterator>/<array>) or avoid std::size here (e.g. std::extent_v / sizeof-based length).
| #include <vector> | |
| #include <vector> | |
| #include <iterator> |
| std::lock_guard<std::mutex> lock(thread_mutex_); | ||
| if (!running_.load()) { | ||
| return; | ||
| } | ||
|
|
||
| running_.store(false); | ||
| // Unlock before joining so the thread can complete any in-progress work. | ||
| // Note: the thread only touches running_ and Carbon APIs, so this is safe. |
There was a problem hiding this comment.
StopThread() only flips running_ and returns, leaving thread_ joinable. If EnsureThread() is ever called after StopThread() but before JoinThread(), thread_ = std::thread(...) would overwrite a joinable std::thread and call std::terminate. Consider making shutdown atomic by moving/swapping thread_ into a local under thread_mutex_, then joining that local outside the lock; also the comment about joining is currently misleading since StopThread() doesn’t join.
| std::lock_guard<std::mutex> lock(thread_mutex_); | |
| if (!running_.load()) { | |
| return; | |
| } | |
| running_.store(false); | |
| // Unlock before joining so the thread can complete any in-progress work. | |
| // Note: the thread only touches running_ and Carbon APIs, so this is safe. | |
| std::thread local_thread; | |
| { | |
| std::lock_guard<std::mutex> lock(thread_mutex_); | |
| if (!running_.load()) { | |
| return; | |
| } | |
| running_.store(false); | |
| // Move the thread object out while holding the lock so that thread_ | |
| // is no longer joinable when EnsureThread() next runs. | |
| local_thread = std::move(thread_); | |
| } | |
| // Join outside the lock so the worker can finish any in-progress work. | |
| if (local_thread.joinable()) { | |
| local_thread.join(); | |
| } |
The macOS global shortcut implementation had two bugs: function key shortcuts (F2–F20) were silently broken due to incorrect keycode arithmetic, and shortcuts would never fire in apps that don't run
[NSApp run].Changes
Fix F-key virtual keycodes: Replaced
kVK_F1 + (fnum - 1)with an explicit lookup table for F1–F20. Carbon's VK codes are non-contiguous (kVK_F1=0x7A,kVK_F2=0x78,kVK_F3=0x63, …), so the arithmetic formula only worked for F1.Add background event thread: Mirrors the Windows (WM_HOTKEY loop) and Linux (XNextEvent loop) pattern. On first shortcut registration, starts a dedicated thread calling
ReceiveNextEventto drain Carbon hotkey events from the process-wide queue — enabling global shortcuts without requiring[NSApp run]on the main thread.Thread safety:
EnsureThreadandStopThreadboth holdthread_mutex_when checking/modifyingrunning_, eliminating a start/stop race.JoinThreadis called separately (outside the lock) to avoid blocking while holding the mutex.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.