Add ability to rescan all plugins#125
Conversation
Added instruments: - China: Guqin, Pipa, Yangqin - Japan: Hichiriki, Sho - Korea (melodic): Ajaeng, Daegeum, Gayageum - Korea (percussion): Samul Buk, Samul Janggu, Sanjo Janggu, Sori Buk Also fix typos in Guzheng and Baryton IDs
* ASIO reset() now fires availableOutputDevicesChanged instead of self-handling close/reopen, matching the WASAPI behavior * AudioDriverController::checkOutputDevice() falls back to the default audio driver (WASAPI) if reopen fails
Add MPE support for East Asian instruments
…to_system_default Fix auto switch to system default
…later The dock widget may still hold a pointer to such a frame in m_lastPositions Restoring it there causes FrameQuick::~FrameQuick() to delete the dock widget via qDeleteAll(), leading to a crash later
📝 WalkthroughWalkthroughThis pull request adds a clear() method to the audio plugins registry interface and implementation, which empties plugin data, persists the change, and notifies listeners. A new rescanAllPlugins() method is added to the register scenario interface and implementation, which clears the registry and then updates it. The test mock is updated to include the new clear() method. Separately, dock window code in both dockwindow and dockwindow_v2 modules now connects to the destroyed signal of custom menu models to nullify stale pointers, and KDDockWidgets' LayoutWidget adds a guard to skip placeholder restoration on frames marked for deletion. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RegisterAudioPluginsScenario
participant KnownAudioPluginsRegister
Caller->>RegisterAudioPluginsScenario: rescanAllPlugins()
RegisterAudioPluginsScenario->>KnownAudioPluginsRegister: clear()
KnownAudioPluginsRegister->>KnownAudioPluginsRegister: empty maps, writePluginsInfo(), notify
KnownAudioPluginsRegister-->>RegisterAudioPluginsScenario: Ret
RegisterAudioPluginsScenario->>RegisterAudioPluginsScenario: updatePluginsRegistry()
Estimated code review effort: 2 (Simple) Related issues: None provided in context. Related PRs: None provided in context. Suggested labels: audioplugins, dockwindow, bugfix Suggested reviewers: None provided in context. Poem A rabbit hopped through registry code, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/dockwindow_v2/qml/Muse/Dock/dockpanelview.cpp (1)
82-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame stale-model closure issue as in
dockwindow'sdockpanelview.cpp.The
destroyedlambda unconditionally nullsm_customMenuModelwithout checking whether the destroyed object is still the currently-assigned model. IfsetCustomMenuModel()is called again before the previous model is destroyed, the previous model'sdestroyedconnection survives (bound tothis) and can later null out a reference to a newer, still-valid model.🐛 Proposed fix
- connect(model, &AbstractMenuModel::destroyed, this, [this]() { - m_customMenuModel = nullptr; - }); + connect(model, &AbstractMenuModel::destroyed, this, [this, model]() { + if (m_customMenuModel == model) { + m_customMenuModel = nullptr; + } + });🤖 Prompt for 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. In `@framework/dockwindow_v2/qml/Muse/Dock/dockpanelview.cpp` around lines 82 - 101, The stale-model reset in setCustomMenuModel is unsafe because the destroyed handler can clear m_customMenuModel even after a newer model has been assigned. Update the destroyed connection in dockpanelview.cpp so it only nulls m_customMenuModel when the destroyed object matches the currently stored model, using the setCustomMenuModel lambda and the AbstractMenuModel pointer captured there to guard against overwriting a newer model.framework/dockwindow/qml/Muse/Dock/dockpanelview.cpp (1)
80-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStale-model closure can null out a newer, still-valid
m_customMenuModel.The
destroyedlambda captures no reference tomodel, so it unconditionally clearsm_customMenuModel. IfsetCustomMenuModel()is called again with a new model before the old one is destroyed, the old model'sdestroyedconnection is never removed (it lives as long asthis). When the old model is eventually destroyed, this stale connection fires and wipes out the pointer to the current, still-valid model — silently dropping custom context-menu items even though the new model is alive.🐛 Proposed fix: only clear if the destroyed model is still the current one
- connect(model, &AbstractMenuModel::destroyed, this, [this]() { - m_customMenuModel = nullptr; - }); + connect(model, &AbstractMenuModel::destroyed, this, [this, model]() { + if (m_customMenuModel == model) { + m_customMenuModel = nullptr; + } + });🤖 Prompt for 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. In `@framework/dockwindow/qml/Muse/Dock/dockpanelview.cpp` around lines 80 - 99, The destroyed-handler in setCustomMenuModel() can clear a newer model by accident, because it unconditionally resets m_customMenuModel when any previously connected AbstractMenuModel is destroyed. Update the lambda connected to AbstractMenuModel::destroyed so it identifies the specific model instance it was created for and only sets m_customMenuModel to nullptr if that destroyed instance is still the current m_customMenuModel; keep the existing itemsChanged and itemChanged connections in setCustomMenuModel() unchanged.
🤖 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 `@framework/audioplugins/internal/knownaudiopluginsregister.cpp`:
- Around line 156-165: The clear() logic in KnownAudioPluginsRegister is wiping
all AudioPluginInfo state, so rescanAllPlugins() recreates plugins with default
values and loses persisted enabled/errorCode settings. Update clear() so it
preserves existing plugin metadata across rescans, and only refresh the plugin
inventory/paths as needed; use KnownAudioPluginsRegister::clear and
rescanAllPlugins as the main places to keep disabled status and stored errorCode
intact when rebuilding the list.
---
Outside diff comments:
In `@framework/dockwindow_v2/qml/Muse/Dock/dockpanelview.cpp`:
- Around line 82-101: The stale-model reset in setCustomMenuModel is unsafe
because the destroyed handler can clear m_customMenuModel even after a newer
model has been assigned. Update the destroyed connection in dockpanelview.cpp so
it only nulls m_customMenuModel when the destroyed object matches the currently
stored model, using the setCustomMenuModel lambda and the AbstractMenuModel
pointer captured there to guard against overwriting a newer model.
In `@framework/dockwindow/qml/Muse/Dock/dockpanelview.cpp`:
- Around line 80-99: The destroyed-handler in setCustomMenuModel() can clear a
newer model by accident, because it unconditionally resets m_customMenuModel
when any previously connected AbstractMenuModel is destroyed. Update the lambda
connected to AbstractMenuModel::destroyed so it identifies the specific model
instance it was created for and only sets m_customMenuModel to nullptr if that
destroyed instance is still the current m_customMenuModel; keep the existing
itemsChanged and itemChanged connections in setCustomMenuModel() unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bc5de1f-7d99-4dfa-bfed-a939542cfce0
📒 Files selected for processing (10)
framework/audioplugins/iknownaudiopluginsregister.hframework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/knownaudiopluginsregister.hframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/mocks/knownaudiopluginsregistermock.hframework/dockwindow/qml/Muse/Dock/dockpanelview.cppframework/dockwindow/thirdparty/KDDockWidgets/src/private/LayoutWidget.cppframework/dockwindow_v2/qml/Muse/Dock/dockpanelview.cpp
| Ret KnownAudioPluginsRegister::clear() | ||
| { | ||
| m_pluginInfoMap.clear(); | ||
| m_pluginPaths.clear(); | ||
|
|
||
| Ret ret = writePluginsInfo(); | ||
| m_pluginInfoListChanged.notify(); | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect registerNewPlugins/registerPlugin to see default enabled/errorCode handling
rg -n -B2 -A20 'Ret RegisterAudioPluginsScenario::registerNewPlugins|Ret RegisterAudioPluginsScenario::registerPlugin\(' framework/audioplugins/internal/registeraudiopluginsscenario.cppRepository: musescore/muse_framework
Length of output: 1438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the registration path and persistence handling around plugin state.
sed -n '150,280p' framework/audioplugins/internal/registeraudiopluginsscenario.cpp
printf '\n====\n'
sed -n '240,340p' framework/audioplugins/internal/knownaudiopluginsregister.cpp
printf '\n====\n'
rg -n 'enabled|errorCode|AudioPluginInfo' framework/audioplugins/internal/registeraudiopluginsscenario.cpp framework/audioplugins/internal/knownaudiopluginsregister.cppRepository: musescore/muse_framework
Length of output: 6160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,240p' framework/audioplugins/internal/knownaudiopluginsregister.cpp
printf '\n====\n'
sed -n '1,140p' framework/audioplugins/internal/registeraudiopluginsscenario.cpp
printf '\n====\n'
rg -n 'rescanAllPlugins|updatePluginsRegistry|clear\(\)' framework/audioplugins/internal/*.cpp framework/audioplugins/internal/*.hRepository: musescore/muse_framework
Length of output: 8328
Rescan preserves no plugin state
clear() drops every AudioPluginInfo, so rescanAllPlugins() rebuilds entries with fresh defaults (enabled = true for newly registered plugins). That can silently re-enable disabled plugins and erase stored errorCode values.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 156-156: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 Prompt for 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.
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp` around lines
156 - 165, The clear() logic in KnownAudioPluginsRegister is wiping all
AudioPluginInfo state, so rescanAllPlugins() recreates plugins with default
values and loses persisted enabled/errorCode settings. Update clear() so it
preserves existing plugin metadata across rescans, and only refresh the plugin
inventory/paths as needed; use KnownAudioPluginsRegister::clear and
rescanAllPlugins as the main places to keep disabled status and stored errorCode
intact when rebuilding the list.
1 participant
Ports (changes in the framework): musescore/MuseScore#33855
Also ports: musescore/MuseScore#33848