Gracefully handle outdated effect IDs #4691
Conversation
| VERIFY_OR_DEBUG_ASSERT(m_registeredEffects.contains(effectId)) { | ||
| return EffectManifestPointer(); | ||
| } | ||
| // This may return a null pointer in case a previously stored effect is no longer available |
There was a problem hiding this comment.
Please move this to a /// documentation comment in the header so this information is easier to find for the caller.
| } | ||
|
|
||
| EffectManifestPointer EffectsBackendManager::getManifest( | ||
| const QString& id, EffectBackendType backendType) const { | ||
| return m_effectsBackends.value(backendType)->getManifest(id); | ||
| auto pEffectsBackend = m_effectsBackends.value(backendType); | ||
| VERIFY_OR_DEBUG_ASSERT(pEffectsBackend) { |
There was a problem hiding this comment.
is a debug assert correct? What if the user uninstalls a plugin between launches of mixxx? I think silently ignoring the bad plugin is fine, no need to assert. (Unless I'm wrong about what this code does)
There was a problem hiding this comment.
This asserts for an backend enum that is not in the list if backends.
This should never happen, because old Mixxx versions without a new backend will not even have an enum value for it.
There was a problem hiding this comment.
All future backbends will default to "Built-In" here:
There is an not documented side effect, when future backend has the same effect as the build in. In this case, instead of removing the new effect from the preset, it will be replaced by the build in type of the same name.
Is this desired or a bug?
| @@ -16,6 +16,8 @@ class EffectsBackendManager { | |||
| }; | |||
| const QList<EffectManifestPointer> getManifestsForBackend(EffectBackendType backendType) const; | |||
| EffectManifestPointer getManifestFromUniqueId(const QString& uid) const; | |||
| /// returns a pointer to the manifest or a null pointer in case a | |||
| /// the previously stored backend or effect is no longer available | |||
There was a problem hiding this comment.
yes, based on this documentation I think just returning nullptr is fine, it shouldn't be a debug assert.
|
When in doubt, we should assert. At least we notice it if the assertion is wrong, rather than the other way around. |
|
I have used asserts for conditions that are impossible as far as I understand the code. |
I am talking about the case where a user was using an external effect in mixxx, and then uninstalls the effect. Mixxx shouldn't asset, it should be able to gracefully handle the missing effect. It sounds like you've covered the uninstallation case, so we're all good here 👍 |
This happens when the user uninstalls LV2 effects and may also happen after a downgrade from a Mixxx version that includes new effects.