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.

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

Effect Blacklisting #1674

Merged
merged 20 commits into from
May 29, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion src/effects/builtin/builtinbackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include "effects/builtin/tremoloeffect.h"

BuiltInBackend::BuiltInBackend(QObject* pParent)
: EffectsBackend(pParent, tr("Built-in")) {
: EffectsBackend(pParent, "Built-in") {
// Keep this list in a reasonable order
// Mixing EQs
registerEffect<Bessel4LVMixEQEffect>();
Expand Down
5 changes: 3 additions & 2 deletions src/effects/defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ enum class EffectEnableState {
Enabling
};

enum class EffectType {
enum class BackendType {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately clear what "BackendType" means without context. I think "EffectType" is a better name. If you don't like that, then how about "EffectBackendType"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, EffectBackendType seems to be better than EffectType to me. I think EffectType should denote Phaser, compressor, etc...

I have changed it to EffectBackendType

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. "EffectBackendType" works well.

BuiltIn,
LV2
LV2,
Unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this Unknown value? Can we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Unknown to handle the case of a manifest not having a backend rather than handling it while setting the effectType in dlgprefeffects. I think we should keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

When would an EffectManifest ever not have a backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EffectManifest would always have a backend currently. But because the backendName is not set in its constructor but through setBackendName() function, I think we should set it to Unknown for the time being rather than setting it to the first value (Built-in) in the EffectBackendType enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought of a use case for this Unknown value. When we parse an XML file specifying an effect chain preset and use the EffectManifest::backendTypeFromString function, we can use the Unknown return value to determine that the effect cannot be loaded.

};

enum class SignalProcessingStage {
Expand Down
30 changes: 28 additions & 2 deletions src/effects/effectmanifest.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,15 @@ class EffectManifest final {
}
}

const BackendType& backendType() const {
return m_backendType;
}
const QString& backendName() const {
return m_backendName;
}
void setBackendName(const QString& name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

m_backendName = name;
m_backendType = backendTypeFromString(name);
m_backendName = backendTypeToString(m_backendType);
}

const QString& author() const {
Expand Down Expand Up @@ -133,6 +137,28 @@ class EffectManifest final {
m_metaknobDefault = metaknobDefault;
}

static QString backendTypeToString(BackendType type) {
switch (type) {
case BackendType::BuiltIn:
//: Used for effects that are built into Mixxx
return QObject::tr("Built-in");
case BackendType::LV2:
return QObject::tr("LV2");
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace QObject::tr with QString. It does not make sense to translate "LV2"

default:
//: Used for effects from unknown sources
return QObject::tr("Unknown");
}
}
static BackendType backendTypeFromString(const QString& name) {
if (name == "Built-in") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail when parsing a string that came from the backendTypeToString function above if the string was translated. So, I think it would be good to rename backendTypeToString to backendTypeToTranslatedString for clarity.

return BackendType::BuiltIn;
} else if (name == "LV2") {
return BackendType::LV2;
} else {
return BackendType::Unknown;
}
}

private:
QString debugString() const {
return QString("EffectManifest(%1)").arg(m_id);
Expand All @@ -141,8 +167,8 @@ class EffectManifest final {
QString m_id;
QString m_name;
QString m_shortName;
BackendType m_backendType;
QString m_backendName;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove m_backendName

// QString m_backendName;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

QString m_author;
QString m_version;
QString m_description;
Expand Down
2 changes: 1 addition & 1 deletion src/effects/lv2/lv2backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#include "effects/lv2/lv2manifest.h"

LV2Backend::LV2Backend(QObject* pParent)
: EffectsBackend(pParent, tr("LV2")) {
: EffectsBackend(pParent, "LV2") {
m_pWorld = lilv_world_new();
initializeProperties();
lilv_world_load_all(m_pWorld);
Expand Down
2 changes: 1 addition & 1 deletion src/preferences/dialog/dlgprefeffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ DlgPrefEffects::DlgPrefEffects(QWidget* pParent,
EffectManifestPointer pManifest = profile->pManifest;

// Blacklisted Non-builtin effects by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's elaborate on this comment to explain why we are choosing to do this. Replace the comment with:
"Users are likely to have lots of external plugins installed and many of them are useless for DJing. To avoid cluttering the list shown in WEffectSelector, blacklist external plugins by default."

bool defaultValue = QString::localeAwareCompare(pManifest->backendName(), tr("Built-in")) == 0;
bool defaultValue = (pManifest->backendType() == BackendType::BuiltIn);
bool visible = m_pConfig->getValue<bool>(ConfigKey("[Visible Effects]",
pManifest->id()), defaultValue);
profile->bIsVisible = visible;
Expand Down