Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle outdated effect IDs #4691

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Gracefully handle outdated effect IDs #4691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90c62ea
Gracefully handle outdated effect IDs
daschuer Feb 28, 2022
4d76a30
Improve comments for getManifest functions
daschuer Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/effects/backends/builtin/builtinbackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,6 @@ const QList<QString> BuiltInBackend::getEffectIds() const {
}

EffectManifestPointer BuiltInBackend::getManifest(const QString& effectId) const {
VERIFY_OR_DEBUG_ASSERT(m_registeredEffects.contains(effectId)) {
return EffectManifestPointer();
}
return m_registeredEffects.value(effectId).pManifest;
}

Expand Down
2 changes: 2 additions & 0 deletions src/effects/backends/effectsbackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ class EffectsBackend {

/// returns a list sorted like it should be displayed in the GUI
virtual const QList<QString> getEffectIds() const = 0;
/// returns a pointer to the manifest or a null pointer in case a
/// previously stored effect is no longer available
virtual EffectManifestPointer getManifest(const QString& effectId) const = 0;
virtual const QList<EffectManifestPointer> getManifests() const = 0;
virtual bool canInstantiateEffect(const QString& effectId) const = 0;
Expand Down
20 changes: 15 additions & 5 deletions src/effects/backends/effectsbackendmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,27 @@ EffectManifestPointer EffectsBackendManager::getManifestFromUniqueId(
// Do not manipulate the string passed to this function, just pass
// it directly to BuiltInBackend.
if (delimiterIndex == -1) {
return m_effectsBackends.value(EffectBackendType::BuiltIn)
->getManifest(uid);
auto pEffectsBackend = m_effectsBackends.value(EffectBackendType::BuiltIn);
VERIFY_OR_DEBUG_ASSERT(pEffectsBackend) {
return {};
}
return pEffectsBackend->getManifest(uid);
}
backendType = EffectsBackend::backendTypeFromString(uid.mid(delimiterIndex + 1));
return m_effectsBackends.value(backendType)
->getManifest(uid.mid(-1, delimiterIndex + 1));
auto pEffectsBackend = m_effectsBackends.value(backendType);
VERIFY_OR_DEBUG_ASSERT(pEffectsBackend) {
return {};
}
return pEffectsBackend->getManifest(uid.mid(-1, delimiterIndex + 1));
}

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All future backbends will default to "Built-In" here:

mixxx/src/effects/backends/effectsbackend.cpp

Line 10 in 5158e62

EffectBackendType EffectsBackend::backendTypeFromString(const QString& typeName) {

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?

return {};
}
return pEffectsBackend->getManifest(id);
}

const QString EffectsBackendManager::getDisplayNameForEffectPreset(
Expand Down
2 changes: 2 additions & 0 deletions src/effects/backends/effectsbackendmanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, based on this documentation I think just returning nullptr is fine, it shouldn't be a debug assert.

EffectManifestPointer getManifest(const QString& id, EffectBackendType backendType) const;
const QString getDisplayNameForEffectPreset(EffectPresetPointer pPreset) const;

Expand Down
3 changes: 0 additions & 3 deletions src/effects/backends/lv2/lv2backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,6 @@ bool LV2Backend::canInstantiateEffect(const QString& effectId) const {
}

EffectManifestPointer LV2Backend::getManifest(const QString& effectId) const {
VERIFY_OR_DEBUG_ASSERT(m_registeredEffects.contains(effectId)) {
return EffectManifestPointer();
}
return m_registeredEffects.value(effectId);
}

Expand Down
6 changes: 4 additions & 2 deletions src/effects/presets/effectchainpresetmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,8 +554,10 @@ EffectsXmlData EffectChainPresetManager::readEffectsXml(
const QDomDocument& doc, const QStringList& deckStrings) {
EffectManifestPointer pDefaultQuickEffectManifest = m_pBackendManager->getManifest(
FilterEffect::getId(), EffectBackendType::BuiltIn);
auto defaultQuickEffectChainPreset = EffectChainPresetPointer(
new EffectChainPreset(pDefaultQuickEffectManifest));
auto defaultQuickEffectChainPreset =
EffectChainPresetPointer(pDefaultQuickEffectManifest
? new EffectChainPreset(pDefaultQuickEffectManifest)
: new EffectChainPreset());

QList<EffectChainPresetPointer> standardEffectChainPresets;
QHash<QString, EffectChainPresetPointer> quickEffectPresets;
Expand Down
7 changes: 6 additions & 1 deletion src/effects/presets/effectpresetmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ void EffectPresetManager::loadDefaultEffectPresets() {
if (!pEffectPreset->isEmpty()) {
EffectManifestPointer pManifest = m_pBackendManager->getManifest(
pEffectPreset->id(), pEffectPreset->backendType());
m_defaultPresets.insert(pManifest, pEffectPreset);
if (pManifest) {
m_defaultPresets.insert(pManifest, pEffectPreset);
}
}
file.close();
}
Expand All @@ -69,6 +71,9 @@ void EffectPresetManager::saveDefaultForEffect(EffectPresetPointer pEffectPreset

const auto pManifest = m_pBackendManager->getManifest(
pEffectPreset->id(), pEffectPreset->backendType());
VERIFY_OR_DEBUG_ASSERT(pManifest) {
return;
}
m_defaultPresets.insert(pManifest, pEffectPreset);

QDomDocument doc(EffectXml::kEffect);
Expand Down