Revamp audio plugins module#33535
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new AudioPluginsAppConfigModule (implementation, header, build inclusion) and registers it in app initialization. The module sets runtime default audio-resource attributes and registers a v2→v3 migration that moves legacy top-level hasNativeEditorSupport into attributes. The changebase replaces direct resource-type and attribute accesses with helper APIs (isResourceType, resourceTypeFromString, intAttribute, hasNativeEditorSupport), updates VST attribute keys, and makes ProjectAudioSettings use audioplugins::AudioResourceType and corresponding JSON import/export adjustments. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
src/app/internal/audiopluginsappconfigmodule.cpp (2)
71-77: 💤 Low valueMinor: migration overwrites any pre-existing
hasNativeEditorSupportinattributes.If a v2 record already has the key in
meta.attributes(e.g., from manual editing or partial migration), this code unconditionally replaces it with the legacy top-level value. Probably acceptable since legacy v2 data shouldn't have populated this attribute, but consider guarding withif (!attrs.contains("hasNativeEditorSupport"))to be defensive.🤖 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 `@src/app/internal/audiopluginsappconfigmodule.cpp` around lines 71 - 77, The migration unconditionally overwrites meta.attributes["hasNativeEditorSupport"]; modify the block that reads meta and sets attrs (variables meta and attrs, and the bool b derived from meta.value("hasNativeEditorSupport")) to only set attrs.set("hasNativeEditorSupport", ...) when attrs does not already contain that key (i.e., wrap the attrs.set call in if (!attrs.contains("hasNativeEditorSupport")) so existing v2 attributes are preserved), then write attrs back to meta as before.
65-92: ⚡ Quick winUse
audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTEfor the JSON key in the v2→v3 migration
src/app/internal/audiopluginsappconfigmodule.cppwrites the attribute under the literal"hasNativeEditorSupport", whilesrc/project/internal/projectaudiosettings.cppreads/migrates it usingaudio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE. Replace the hard-coded occurrences in the migration with the shared constant (using the appropriate conversion/type formuse::JsonObjectkeys) to prevent silent drift.🤖 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 `@src/app/internal/audiopluginsappconfigmodule.cpp` around lines 65 - 92, The migration lambda registered via migrations->registerMigration (v2→v3) currently uses the hard-coded key "hasNativeEditorSupport"; change all occurrences to use the shared constant audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE (convert it to the appropriate std::string type if necessary) when calling meta.contains(...), meta.value(...).toBool(), attrs.set(...), and when filtering keys into metaWithoutLegacy so the legacy key is removed by comparing against that constant rather than the literal.
🤖 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.
Nitpick comments:
In `@src/app/internal/audiopluginsappconfigmodule.cpp`:
- Around line 71-77: The migration unconditionally overwrites
meta.attributes["hasNativeEditorSupport"]; modify the block that reads meta and
sets attrs (variables meta and attrs, and the bool b derived from
meta.value("hasNativeEditorSupport")) to only set
attrs.set("hasNativeEditorSupport", ...) when attrs does not already contain
that key (i.e., wrap the attrs.set call in if
(!attrs.contains("hasNativeEditorSupport")) so existing v2 attributes are
preserved), then write attrs back to meta as before.
- Around line 65-92: The migration lambda registered via
migrations->registerMigration (v2→v3) currently uses the hard-coded key
"hasNativeEditorSupport"; change all occurrences to use the shared constant
audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE (convert it to the appropriate
std::string type if necessary) when calling meta.contains(...),
meta.value(...).toBool(), attrs.set(...), and when filtering keys into
metaWithoutLegacy so the legacy key is removed by comparing against that
constant rather than the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8146db70-bf03-4abb-ba77-be7a898068bc
📒 Files selected for processing (14)
musesrc/app/CMakeLists.txtsrc/app/appfactory.cppsrc/app/internal/audiopluginsappconfigmodule.cppsrc/app/internal/audiopluginsappconfigmodule.hsrc/converter/internal/compat/notationmeta.cppsrc/notationscene/qml/MuseScore/NotationScene/percussionpanel/percussionpanelmodel.cppsrc/playback/internal/drumsetloader.cppsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/soundprofilesrepository.cppsrc/playback/qml/MuseScore/Playback/inputresourceitem.cppsrc/playback/qml/MuseScore/Playback/outputresourceitem.cppsrc/project/internal/projectaudiosettings.cppsrc/project/internal/projectaudiosettings.h
d75af1c to
aab1ac6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/internal/audiopluginsappconfigmodule.cpp (1)
70-77: ⚡ Quick winUse the shared attribute key constant instead of hard-coded
"hasNativeEditorSupport".This migration should use the same shared key constant used elsewhere to avoid future key drift between migrations and runtime reads.
♻️ Proposed fix
- if (meta.contains("hasNativeEditorSupport")) { + if (meta.contains(muse::audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE)) { muse::JsonObject attrs; if (meta.contains("attributes")) { attrs = meta.value("attributes").toObject(); } - const bool b = meta.value("hasNativeEditorSupport").toBool(); - attrs.set("hasNativeEditorSupport", b ? std::string("true") : std::string("false")); + const bool b = meta.value(muse::audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE).toBool(); + attrs.set(muse::audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE, b ? std::string("true") : std::string("false")); meta.set("attributes", attrs); @@ - if (k == "hasNativeEditorSupport") { + if (k == muse::audio::HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE) { continue; }Also applies to: 82-83
🤖 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 `@src/app/internal/audiopluginsappconfigmodule.cpp` around lines 70 - 77, The code uses the literal "hasNativeEditorSupport" in meta.contains, attrs.set and meta.set; replace that hard-coded string with the shared attribute key constant used by the project (the same symbol other modules use for this attribute) — import or reference that constant (e.g. SharedAttributeKeys::HasNativeEditorSupport or the existing HAS_NATIVE_EDITOR_SUPPORT constant used elsewhere) and use it in meta.contains(...), attrs.set(..., ...), and wherever the literal appears (including the other occurrence around the attrs handling) so migration and runtime use the same key.
🤖 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.
Nitpick comments:
In `@src/app/internal/audiopluginsappconfigmodule.cpp`:
- Around line 70-77: The code uses the literal "hasNativeEditorSupport" in
meta.contains, attrs.set and meta.set; replace that hard-coded string with the
shared attribute key constant used by the project (the same symbol other modules
use for this attribute) — import or reference that constant (e.g.
SharedAttributeKeys::HasNativeEditorSupport or the existing
HAS_NATIVE_EDITOR_SUPPORT constant used elsewhere) and use it in
meta.contains(...), attrs.set(..., ...), and wherever the literal appears
(including the other occurrence around the attrs handling) so migration and
runtime use the same key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b93e90cf-916e-4c3f-96e4-6bb63f3694ac
📒 Files selected for processing (14)
musesrc/app/CMakeLists.txtsrc/app/appfactory.cppsrc/app/internal/audiopluginsappconfigmodule.cppsrc/app/internal/audiopluginsappconfigmodule.hsrc/converter/internal/compat/notationmeta.cppsrc/notationscene/qml/MuseScore/NotationScene/percussionpanel/percussionpanelmodel.cppsrc/playback/internal/drumsetloader.cppsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/soundprofilesrepository.cppsrc/playback/qml/MuseScore/Playback/inputresourceitem.cppsrc/playback/qml/MuseScore/Playback/outputresourceitem.cppsrc/project/internal/projectaudiosettings.cppsrc/project/internal/projectaudiosettings.h
✅ Files skipped from review due to trivial changes (2)
- src/playback/internal/soundprofilesrepository.cpp
- src/project/internal/projectaudiosettings.cpp
aab1ac6 to
a37d491
Compare
c0d59b1 to
d0e050a
Compare
Companion to the muse_framework audioplugins revamp: - New AudioPluginsAppConfigModule wires v0->v1 (hasNativeEditorSupport into attributes) and v1->v2 (enabled boolean -> state string) cache migrations plus the runtime attribute defaults. - Adopts the new typed plugin format helpers (audio::isResourceType, audio::resourceTypeFromString, audio::hasNativeEditorSupport) at call sites in playback, project, notationscene, converter. - Consumes vst::CATEGORIES_ATTRIBUTE (moved out of the audio module).
Replaces the remaining literal-string plugin-type comparisons (e.g. meta.type == "MuseSamplerSoundPack") with audio::isResourceType(meta, AudioResourceType::X). Single source of truth for the wire strings now lives in the framework enum's bridge, plus a unit test pinning them. Bumps muse/ to pick up the framework changes.
KnownAudioPluginsMigrationRegister's constructor now pre-registers the
framework-owned migrations so apps don't have to copy-paste them:
v0 -> v1: structural (envelope intro, no-op callback)
v1 -> v2: enabled boolean -> state string (AudioPluginState is a
framework enum, so this transformation belongs here)
Apps register only their own field migrations on top. Bumps
CURRENT_KNOWN_AUDIO_PLUGINS_VERSION 2 -> 3 since the MuseScore-specific
hasNativeEditorSupport migration moves to v2 -> v3 (app-owned).
inputresourceitem and outputresourceitem parse a plugin id out of the menu-item string the UI emits, then use it to look up plugin metadata. That value is plugin identity, not an audio-pipeline resource id; type it accordingly now that the framework distinguishes them. Three local sites only; everything else in src/ reaches the id via resourceMeta.id (struct field access, type-resolved automatically).
New file pair src/playback/internal/audiometabridge.{h,cpp} with
toAudioMeta / toPluginMeta plus list variants. Converts field-wise
between audio::AudioResourceMeta (engine-domain) and audioplugins::
PluginMeta (cache-domain) - the two struct types the framework now
keeps independent.
No call sites changed yet; the next commit retypes all meta usage in
MuseScore (rename audioplugins::AudioResourceMeta references to
audioplugins::PluginMeta, insert bridge calls at the natural seams in
playbackcontroller, projectaudiosettings, soundprofilesrepository).
…plugins split After the framework decoupling: audio::AudioResourceMeta and audioplugins::PluginMeta are distinct struct types. MuseScore code that handles meta in the audio domain keeps using AudioResourceMeta (unqualified, resolves to audio); the few sites that called audioplugins::*Attribute helpers on audio-side meta now use the audio:: local helpers (audio gained its own boolAttribute / intAttribute in the same framework commit). - drumsetloader and percussionpanelmodel switch from audioplugins::intAttribute(meta, ...) to audio::intAttribute(...) since the meta they read is audio-domain (received from playback controller / audio settings). - projectaudiosettings retypes the qualified audioplugins:: AudioResourceType to its new spelling audioplugins::PluginType. - audiopluginsappconfigmodule constructs PluginAttributes (the audioplugins runtime-defaults map) using the renamed type name. The new audiometabridge from the previous commit is in place for future use when the two struct types diverge; no MuseScore call site needs it right now because the app keeps meta within one domain at a time.
4c30918 to
39cc81d
Compare
Framework PR: musescore/muse_framework#47
Audacity PR: audacity/audacity#10989
Adapt MuseScore to the revamped
muse::audiopluginsframework.Call sites match the framework's audio↔audioplugins split. Code that handles plugin-cache values uses
audioplugins::PluginMeta/PluginResourceId(project audio settings, the audioplugins app-config module, the playback menu-id parsers); code that handles audio-engine values usesaudio::AudioResourceMetaand the localaudio::intAttribute/boolAttributehelpers (drumsetloader, percussionpanelmodel).App-side bridge
src/playback/internal/audiometabridge.{h,cpp}—toAudioMeta(const audioplugins::PluginMeta&)/toPluginMeta(const audio::AudioResourceMeta&)and list variants, documenting the audio↔audioplugins seam. Current call sites work within a single domain at a time so the bridge isn't exercised yet; it's there for future divergence between the two struct types.New app-side
AudioPluginsAppConfigModuleregisters the v2→v3known_audio_plugins.jsonmigration — lifts the legacyhasNativeEditorSupportboolean intometa.attributes. (The framework auto-registers v0→v1 and v1→v2.)Uses
audio::isResourceType()/ typed helpers across playback, project, and notation-scene instead of direct comparisons against theAudioResourceTypeenum.hasNativeEditorSupportis not a struct field — it lives inmeta.attributes.projectaudiosettingsdoesn't write it to the saved JSON and migrates the legacy field on load, so existing project files still open.CATEGORIES_ATTRIBUTElives in thevstnamespace (it is VST-specific).Bumps the
musesubmodule to the shared framework branch.I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)