Improve hotkey capture and notification settings#3
Conversation
|
Latest update has been tested locally with the Caps-layer mapping case. The PR is mergeable and ready from my side; upstream maintainer permission is required to merge. |
|
Hi @phoenixray2000, thanks for the work here. A lot is going on in this PR, so let me think through it with you before going further. Scope: As far as I can tell, this contains at least three independent changes:
I'd like to split these into separate PRs. The notification toggle looks straightforward and I can probably merge that quickly. The keyboard hook is a much bigger change and I want to understand it properly first. Specific questions:
One housekeeping note: I just added a CONTRIBUTING.md. One thing it asks: if AI tools (Codex, Copilot, Claude Code, etc.) were used for parts of the PR, please mention it, and confirm you've run and tested the code yourself. Not a problem if so, just helps me calibrate the review. Your branch is named Once we agree on the split and I have answers to the above, I'll look at each piece in detail. |
|
Hi @lukmay , thanks for the detailed review. You are right that this PR mixes too many things together. In hindsight, it should have been split earlier. At this point, though, this is an old PR and the code has already moved forward quite a bit, so manually reconstructing it into three clean PRs is probably not practical for me. On the hotkey issue: error 1409 was not the real reason for the keyboard hook. I agree that a normal The issue I was trying to fix is different: in my test environment, the PyQt hotkey capture path could not reliably capture The long commit history was mainly caused by my use of AI tools, mostly Codex / Claude Code, during testing. Some intermediate test iterations were auto-committed when they should not have been committed. That was my process mistake, and I should not have committed during those test iterations. The final version has been tested through the target workflow, including hotkey setting and the recording start/stop flow. Given the current state, I do not think it is practical to split this old PR into three clean PRs now. If you still prefer a cleaner review path, I can instead summarize the final effective changes and the manual test cases in this PR comment, so the review can focus on the final state rather than the noisy intermediate commits. I understand the concern about the low-level keyboard hook. I can also document when it is active, why it was added, and the privacy / CPU implications considered. |
|
Hi @phoenixray2000, thanks for the fast and detailed response! The clarification on the Qt Alt capture issue makes sense. Before I continue with the review, a couple of things: Could you remove |
893586e to
3eadc1f
Compare
3eadc1f to
71fe55d
Compare
|
Cleaned up and tested. |
Summary
Root cause
Qt could miss Alt-based letter events during settings capture, so capture needed a temporary keyboard hook. A later runtime issue came from the trigger side:
keyboard.add_hotkey()can miss AHK-injected Alt combinations, andRegisterHotKeywas not reliable for the reported shortcut becauseAlt+Shift+Ris already registered on this machine (ERROR_HOTKEY_ALREADY_REGISTERED, 1409). The runtime path now uses a low-level Windows hook for parsed hotkeys and keepskeyboardonly as a fallback for unsupported shortcut strings.Impact
Users can configure Alt/Alt+Shift shortcuts reliably, see when the app is waiting for a shortcut, stop recording with the same start shortcut when enabled, choose whether tray popup notifications are shown, and trigger configured recording hotkeys through Caps-layer mappings.
Validation
.\\.venv\\Scripts\\python.exe -B -m unittest discover -s tests -v-> 26 tests passed.\\.venv\\Scripts\\python.exe -m compileall -q main.py gui.py audio_recorder.py clipboard_utils.py tests-> passed.\\.venv\\Scripts\\python.exe -c "import main; print('import main ok')"-> passeddist\\QuickAudioRecorder.exe