FEAT(client): Disable shortcuts while session is locked#7039
FEAT(client): Disable shortcuts while session is locked#7039Krzmbrzl wants to merge 5 commits intomumble-voip:masterfrom
Conversation
WalkthroughThis pull request adds OS session lock detection to disable global shortcuts when the operating system session is locked. The changes introduce the os-events third-party library as a submodule and integrate it into the build system. The CMake configuration is updated to recognize OBJECT_LIBRARY targets. GlobalShortcutEngine is enhanced with an atomic flag that tracks whether shortcuts should be processed, monitored by a session lock watcher callback. When the OS session locks, the flag prevents keyboard and mouse button events from being processed as shortcuts. Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitmodules3rdparty/os-eventscmake/project-utils.cmakesrc/mumble/CMakeLists.txtsrc/mumble/GlobalShortcut.cppsrc/mumble/GlobalShortcut.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: PR (Translations)
- GitHub Check: PR (Linux)
- GitHub Check: PR (Docs)
- GitHub Check: PR (Windows_x64)
- GitHub Check: PR (macOS)
- GitHub Check: build (macos-14, static, x86_64)
- GitHub Check: build (ubuntu-22.04, shared, x86_64)
- GitHub Check: build (ubuntu-24.04, shared, x86_64)
- GitHub Check: build (ubuntu-24.04, static, x86_64)
- GitHub Check: build (windows-2022, static, x86_64)
- GitHub Check: CodeQL-Build (ubuntu-latest)
- GitHub Check: pr-checks (ubuntu-latest)
🔇 Additional comments (10)
.gitmodules (1)
40-42: LGTM!The submodule entry follows the standard format and is consistent with other third-party dependencies in the repository.
cmake/project-utils.cmake (1)
69-69: LGTM!The addition of
OBJECT_LIBRARYto the regex pattern correctly expands support for object library targets. This change ensures that object libraries (such as those potentially used by the os-events dependency) are properly recognized as compilable targets and can have warnings disabled through thedisable_warnings_for_all_targets_infunction.src/mumble/CMakeLists.txt (1)
734-738: os-events library integration with GlobalShortcut is properly implemented.The os-events library is correctly integrated and actively used in the GlobalShortcut implementation. The code monitors session lock events via
osevents::SessionLockand controls shortcut processing through theallowShortcutProcessingatomic flag, ensuring shortcuts are disabled when the OS session is locked.src/mumble/GlobalShortcut.cpp (4)
38-38: LGTM!The static atomic member definition is correctly placed and initialized. Starting with
trueensures shortcuts work by default (unlocked state), which is the expected behavior.
1003-1006: LGTM!The early return correctly prevents shortcut processing when the session is locked. Returning
false(not suppressed) is appropriate since we don't want to interfere with button events when shortcuts are disabled due to session lock.
31-36: The include is properly integrated.The
osevents/session_lock.hppheader is from the bundled os-events library added viaadd_subdirectory()in CMakeLists.txt with appropriate compiler settings (exports disabled, warnings suppressed). The linking is correctly configured in line 737. No changes needed.
911-919: Consider error handling for SessionLock initialization if osevents::SessionLock can throw exceptions.The static local pattern correctly ensures the session lock watcher is created only once. However, without access to osevents library documentation, it cannot be verified whether the
SessionLockconstructor throws exceptions or has failure modes that require handling. If the library can throw, consider wrapping the initialization in a try-catch block.src/mumble/GlobalShortcut.h (3)
19-19: LGTM!The
<atomic>include is necessary for thestd::atomic<bool>member declaration and is correctly placed.
263-263: LGTM! This change improves error messages.Moving
Q_DISABLE_COPYto the public section is good practice. While it doesn't change functionality (copy operations remain deleted), it provides clearer compiler errors when copy attempts occur—users will see "function is deleted" rather than "private member access," which better communicates the intent.
283-284: LGTM!The private static atomic flag is correctly declared and appropriately scoped. The atomic type ensures thread-safe access from both the callback and the
handleButtonmethod.
37de902 to
4d4a200
Compare
This is to ensure that nobody is able to mess with a running Mumble instance without having to unlock the OS first.
4d4a200 to
12faea0
Compare
277927f to
0743e06
Compare
This is to ensure that nobody is able to mess with a running Mumble
instance without having to unlock the OS first.
Checks