Add M shortcut toggle for audio stream#123
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an M-key shortcut to toggle audio mute/unmute: tracks mute state in GuiManager, handles the key in the frame loop to stop/restart the engine, shows a red "MUTED" label in master controls, updates the Audio menu shortcut label, registers a placeholder test, and documents the shortcut in README. ChangesAudio Mute Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant ImGuiIO
participant GuiManager
participant Engine
User->>ImGuiIO: press 'M'
ImGuiIO->>GuiManager: run_frame() detects key when not in text/item
GuiManager->>GuiManager: toggle_audio_mute_state()
alt now muted
GuiManager->>Engine: stop()
else now unmuted
GuiManager->>Engine: restart()
end
GuiManager->>GuiManager: update audio_muted_ flag and UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gui/gui_manager_frame.cpp`:
- Around line 134-147: The code flips audio_muted_ before consulting the engine,
which can desync if the engine was already stopped; instead, query the real
state with engine_.is_running(), call engine_.stop() if it was running or
engine_.start() if it was not, and then set audio_muted_ based on the resulting
engine state (preferably set audio_muted_ = !engine_.is_running() after the
start/stop call to reflect the true state). Apply this change to the ImGui key
handler block that currently toggles audio_muted_ and to the other identical
block noted in the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90ce4c57-a7a4-4ae3-a85b-bbaa1f4a3111
📒 Files selected for processing (4)
README.mdsrc/gui/gui_manager.hsrc/gui/gui_manager_frame.cppsrc/gui/gui_manager_menu.cpp
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
|
@Ketandora please add test coverage |
|
Hey @Ketandora, thanks for the contribution! The feature works on the PR preview and the overall approach is solid, but there are a few things to address before this can be merged: 🔴 Bug — Mute state can desync from engine stateIn audio_muted_ = !audio_muted_;
if (audio_muted_) {
engine_.stop();
} else {
engine_.start();
}If the engine was already stopped via the menu (or another path), The fix is to drive the toggle from if (engine_.is_running()) {
engine_.stop();
audio_muted_ = true;
} else {
engine_.restart();
audio_muted_ = false;
}This same block appears twice — around line 134 and again around line 216 — both need the fix. 🔴 Missing test coverageNo tests were added for the new functionality. Please add tests covering at least:
🟡 Minor — Unrelated whitespace changesThe diff includes indentation/formatting changes to the existing undo/redo/snapshot shortcut block that aren't related to this feature. Please revert those to keep the diff focused and easier to review. Once these are addressed this should be good to go! |
|
@sudip-mondal-2002 I’ll add the required test coverage asap and update the PR shortly. |
|
Thanks @idrwfanjan for the detailed review and suggestions. I’ll update the mute toggle logic to use the actual engine running state directly, add the requested test coverage, and clean up the unrelated whitespace changes as well. |
|
Hii, @sudip-mondal-2002 @ionfwsrijan ,Updated the PR with the requested changes:
Please check it out and let me know if any further changes or improvements are needed. |
Closes #72
Summary
Implemented an
Mkeyboard shortcut to quickly toggle the audio stream on/off from the main window.Changes Made
Mkey shortcut handling in the main ImGui render loopM) in Audio menu itemsTesting
MScreenshot
(Attached below)

Summary by CodeRabbit
New Features
Documentation