Skip to content

Improve hotkey capture and notification settings#3

Open
phoenixray2000 wants to merge 2 commits into
lukmay:masterfrom
phoenixray2000:codex-hotkey-notification-settings
Open

Improve hotkey capture and notification settings#3
phoenixray2000 wants to merge 2 commits into
lukmay:masterfrom
phoenixray2000:codex-hotkey-notification-settings

Conversation

@phoenixray2000
Copy link
Copy Markdown

@phoenixray2000 phoenixray2000 commented May 17, 2026

Summary

  • Add visible hotkey-capture feedback, clear/escape behavior, and robust Alt/Alt+Shift hotkey capture during settings edits.
  • Add settings to reuse the start-recording hotkey to stop active recordings and to enable/disable tray notifications.
  • Preserve legacy dedicated stop-hotkey behavior, update README docs, and add regression coverage for hotkey capture, settings migration, tray notifications, and record-hotkey stop behavior.
  • On Windows, run recording hotkeys through an app-owned low-level keyboard hook so mapped/injected shortcuts such as Caps+r -> Alt+Shift+R can trigger recording reliably.

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, and RegisterHotKey was not reliable for the reported shortcut because Alt+Shift+R is already registered on this machine (ERROR_HOTKEY_ALREADY_REGISTERED, 1409). The runtime path now uses a low-level Windows hook for parsed hotkeys and keeps keyboard only 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')" -> passed
  • PyInstaller packaging succeeded
  • Latest packaged artifact: dist\\QuickAudioRecorder.exe

@phoenixray2000 phoenixray2000 marked this pull request as ready for review May 17, 2026 19:54
@phoenixray2000
Copy link
Copy Markdown
Author

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.

@phoenixray2000 phoenixray2000 changed the title [codex] Improve hotkey capture and notification settings Improve hotkey capture and notification settings May 18, 2026
@lukmay
Copy link
Copy Markdown
Owner

lukmay commented May 25, 2026

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:

  • Hotkey capture feedback in the settings dialog
  • Tray notification toggle plus "use start hotkey to stop" option
  • A low-level Windows keyboard hook to handle injected/mapped shortcuts (Caps-layer, AHK)

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:

  1. The root cause mentions Alt+Shift+R was already registered on your machine (error 1409). That's a per-system collision, not a general problem with RegisterHotKey. Couldn't we just catch that error and surface it to the user ("this shortcut is taken, pick another"), instead of replacing the whole hotkey path with a low-level hook?

  2. Running a low-level keyboard hook means the app sees every keystroke the user types. That has implications for privacy, antivirus false positives, and CPU. Did you consider those, and is the hook always active or only while recording is armed?

  3. The docs/ implementation plan commit, was that meant to be part of the PR or a working note that slipped in?

  4. The 12 commits look iterative ("fix: capture Alt", "fix: capture Alt Shift", "fix: capture swapped alt ctrl", "fix: handle mapped hotkeys"). What was your manual test setup, and how confident are you that the final version still handles the earlier cases?

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 codex-hotkey-notification-settings, so I'm assuming yes, but I'd rather hear it from you.

Once we agree on the split and I have answers to the above, I'll look at each piece in detail.

@phoenixray2000
Copy link
Copy Markdown
Author

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 RegisterHotKey collision should just be caught and surfaced to the user as “this shortcut is already taken, please pick another.”

The issue I was trying to fix is different: in my test environment, the PyQt hotkey capture path could not reliably capture Alt. This was not caused by Caps-layer or AHK. I also tested with AHK disabled, and the same issue still happened.

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.

@lukmay
Copy link
Copy Markdown
Owner

lukmay commented May 26, 2026

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 docs/superpowers/plans/2026-05-18-hotkey-notification-settings.md? That file shouldn't be in the repo.
Since part of the commit history came from AI-assisted iterations, it might be worth doing a quick pass to check for any leftover artifacts or code that ended up in the final state but isn't actually needed.
Once cleaned up, could you do a full test run on your end? Since I couldn't reproduce the original issue myself, I want to make sure everything works as expected on your side before I do a final review and merge.
No rush, just want to make sure the final state is clean :)

@phoenixray2000 phoenixray2000 force-pushed the codex-hotkey-notification-settings branch from 893586e to 3eadc1f Compare May 26, 2026 07:25
@phoenixray2000 phoenixray2000 force-pushed the codex-hotkey-notification-settings branch from 3eadc1f to 71fe55d Compare May 26, 2026 07:34
@phoenixray2000
Copy link
Copy Markdown
Author

Cleaned up and tested.

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.

2 participants