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
20 changes: 9 additions & 11 deletions src/effects/effectmanifest.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
class EffectManifest final {
public:
EffectManifest()
: m_isVisible(true),
m_isMixingEQ(false),
: m_isMixingEQ(false),
m_isMasterEQ(false),
m_effectRampsFromDry(false),
m_metaknobDefault(0.5) {
Expand Down Expand Up @@ -59,6 +58,13 @@ class EffectManifest final {
}
}

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;
}

const QString& author() const {
return m_author;
}
Expand All @@ -77,14 +83,6 @@ class EffectManifest final {
return m_description;
}

const bool& isVisible() const {
return m_isVisible;
}

void setVisibility(const bool value) {
m_isVisible = value;
}

const bool& isMixingEQ() const {
return m_isMixingEQ;
}
Expand Down Expand Up @@ -142,10 +140,10 @@ class EffectManifest final {
QString m_id;
QString m_name;
QString m_shortName;
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_author;
QString m_version;
QString m_description;
bool m_isVisible;
// This helps us at DlgPrefEQ's basic selection of Equalizers
bool m_isMixingEQ;
bool m_isMasterEQ;
Expand Down
14 changes: 13 additions & 1 deletion src/effects/effectsbackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ EffectsBackend::EffectsBackend(UserSettingsPointer pConfig,

EffectsBackend::~EffectsBackend() {
m_registeredEffects.clear();
m_visibleEffectIds.clear();
m_effectIds.clear();
}

Expand All @@ -30,9 +31,12 @@ void EffectsBackend::registerEffect(const QString& id,

const bool visible = m_pConfig->getValue<bool>(ConfigKey("[Visible Effects]",
pManifest->id()), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

User preference code should be in the preference dialog classes, not scattered throughout the rest of the code. Reading the configuration values should be handled by DlgPrefEffects on initialization, which should make calls to EffectsManager::setEffectVisibility.

pManifest->setVisibility(visible);
pManifest->setBackendName(m_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

pManifest->setBackendType


m_registeredEffects[id] = RegisteredEffect(pManifest, pInstantiator);
if (visible) {
addVisibleEffect(id);
}
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_effectIds.append(id);
emit(effectRegistered(pManifest));
}
Expand All @@ -49,6 +53,14 @@ EffectManifestPointer EffectsBackend::getManifest(const QString& effectId) const
return m_registeredEffects[effectId].manifest();
}

bool EffectsBackend::getVisibility(const QString& effectId) const {
return m_visibleEffectIds.contains(effectId);
}

void EffectsBackend::addVisibleEffect(const QString& effectId) {
m_visibleEffectIds.append(effectId);
}

bool EffectsBackend::canInstantiateEffect(const QString& effectId) const {
return m_registeredEffects.contains(effectId);
}
Expand Down
4 changes: 3 additions & 1 deletion src/effects/effectsbackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class EffectsBackend : public QObject {
// returns a list sorted like it should be displayed in the GUI
virtual const QList<QString> getEffectIds() const;
virtual EffectManifestPointer getManifest(const QString& effectId) const;
virtual bool getVisibility(const QString& effectId) const;
virtual void addVisibleEffect(const QString& effectId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these functions.

virtual bool canInstantiateEffect(const QString& effectId) const;
virtual EffectPointer instantiateEffect(
EffectsManager* pEffectsManager, const QString& effectId);
Expand Down Expand Up @@ -64,7 +66,6 @@ class EffectsBackend : public QObject {

EffectManifestPointer manifest() const { return m_pManifest; };
EffectInstantiatorPointer initiator() const { return m_pInitator; };

Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep this blank line to separate private and public members.

private:
EffectManifestPointer m_pManifest;
EffectInstantiatorPointer m_pInitator;
Expand All @@ -73,6 +74,7 @@ class EffectsBackend : public QObject {
QString m_name;
QMap<QString, RegisteredEffect> m_registeredEffects;
QList<QString> m_effectIds;
QList<QString> m_visibleEffectIds;
Copy link
Contributor

Choose a reason for hiding this comment

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

EffectsBackend should not track which effects are visible, only EffectsManager.

};

#endif /* EFFECTSBACKEND_H */
31 changes: 27 additions & 4 deletions src/effects/effectsmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ EffectsManager::~EffectsManager() {

bool alphabetizeEffectManifests(EffectManifestPointer pManifest1,
EffectManifestPointer pManifest2) {
return QString::localeAwareCompare(pManifest1->displayName(), pManifest2->displayName()) < 0;

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 blank line

int dNameComp = QString::localeAwareCompare(pManifest1->displayName(), pManifest2->displayName());
Copy link
Contributor

@Be-ing Be-ing May 29, 2018

Choose a reason for hiding this comment

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

What do b and d before NameComp mean here? Backend and... ? Let's use variable names that explain themselves better. How about "backendNameComparison" and "displayNameComparison"?

int bNameComp = QString::localeAwareCompare(pManifest1->backendName(), pManifest2->backendName());
// Add an exception for "Native" backends, to keep the Native effects in the beginning
return (bNameComp ? (bNameComp > 0) : (dNameComp < 0));
}

void EffectsManager::addEffectsBackend(EffectsBackend* pBackend) {
Expand All @@ -81,6 +85,9 @@ void EffectsManager::addEffectsBackend(EffectsBackend* pBackend) {
for (const QString& effectId : backendEffects) {
const EffectManifestPointer pManifest = pBackend->getManifest(effectId);
m_availableEffectManifests.append(pManifest);
if (pBackend->getVisibility(effectId)) {
setEffectVisibility(pManifest, true);
}
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_pNumEffectsAvailable->forceSet(m_availableEffectManifests.size());
Expand All @@ -96,9 +103,9 @@ void EffectsManager::addEffectsBackend(EffectsBackend* pBackend) {
}

void EffectsManager::slotBackendRegisteredEffect(EffectManifestPointer pManifest) {
auto insertion_point = qLowerBound(m_availableEffectManifests.begin(),
m_availableEffectManifests.end(),
pManifest, alphabetizeEffectManifests);
auto insertion_point = std::lower_bound(m_availableEffectManifests.begin(),
m_availableEffectManifests.end(),
pManifest, alphabetizeEffectManifests);
m_availableEffectManifests.insert(insertion_point, pManifest);
m_pNumEffectsAvailable->forceSet(m_availableEffectManifests.size());
}
Expand Down Expand Up @@ -311,6 +318,22 @@ EffectButtonParameterSlotPointer EffectsManager::getEffectButtonParameterSlot(
return pParameterSlot;
}

void EffectsManager::setEffectVisibility(EffectManifestPointer pManifest, bool visible) {
if (visible && !m_visibleEffectManifests.contains(pManifest)) {
auto insertion_point = std::lower_bound(m_visibleEffectManifests.begin(),
m_visibleEffectManifests.end(),
pManifest, alphabetizeEffectManifests);
m_visibleEffectManifests.insert(insertion_point, pManifest);
emit visibleEffectsUpdated();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I thought you needed parentheses around the signal name: emit(visibleEffectsUpdated())

} else if (!visible) {
m_visibleEffectManifests.removeOne(pManifest);
emit visibleEffectsUpdated();
}
}

bool EffectsManager::getEffectVisibility(EffectManifestPointer pManifest) {
return m_visibleEffectManifests.contains(pManifest);
}

void EffectsManager::setup() {
// These controls are used inside EQ Effects
Expand Down
10 changes: 10 additions & 0 deletions src/effects/effectsmanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
#include <QScopedPointer>
#include <QPair>

#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

This #include is only needed in the .cpp file, so move it there.


#include "preferences/usersettings.h"
#include "control/controlpotmeter.h"
#include "control/controlpushbutton.h"
Expand Down Expand Up @@ -79,6 +81,9 @@ class EffectsManager : public QObject {
inline const QList<EffectManifestPointer>& getAvailableEffectManifests() const {
return m_availableEffectManifests;
};
inline const QList<EffectManifestPointer>& getVisibleEffectManifests() const {
return m_visibleEffectManifests;
};
const QList<EffectManifestPointer> getAvailableEffectManifestsFiltered(
EffectManifestFilterFnc filter) const;
bool isEQ(const QString& effectId) const;
Expand All @@ -88,6 +93,9 @@ class EffectsManager : public QObject {
EffectManifestPointer getEffectManifest(const QString& effectId) const;
EffectPointer instantiateEffect(const QString& effectId);

void setEffectVisibility(EffectManifestPointer pManifest, bool visibility);
bool getEffectVisibility(EffectManifestPointer pManifest);

// Temporary, but for setting up all the default EffectChains and EffectRacks
void setup();

Expand All @@ -101,6 +109,7 @@ class EffectsManager : public QObject {
signals:
// TODO() Not connected. Can be used when we implement effect PlugIn loading at runtime
void availableEffectsUpdated(EffectManifestPointer);
void visibleEffectsUpdated();

private slots:
void slotBackendRegisteredEffect(EffectManifestPointer pManifest);
Expand All @@ -118,6 +127,7 @@ class EffectsManager : public QObject {
EffectChainManager* m_pEffectChainManager;
QList<EffectsBackend*> m_effectsBackends;
QList<EffectManifestPointer> m_availableEffectManifests;
QList<EffectManifestPointer> m_visibleEffectManifests;

EngineEffectsManager* m_pEngineEffectsManager;

Expand Down
5 changes: 4 additions & 1 deletion src/effects/lv2/lv2backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ void LV2Backend::enumeratePlugins() {

const bool visible = m_pConfig->getValue<bool>(ConfigKey("[Visible Effects]",
lv2Manifest->getEffectManifest()->id()), false);
lv2Manifest->getEffectManifest()->setVisibility(visible);
lv2Manifest->getEffectManifest()->setBackendName(this->getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this->getName() with simply getName()

if (visible) {
addVisibleEffect(lv2Manifest->getEffectManifest()->id());
}

m_registeredEffects.insert(lv2Manifest->getEffectManifest()->id(),
lv2Manifest);
Expand Down
6 changes: 3 additions & 3 deletions src/preferences/dialog/dlgprefeffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ DlgPrefEffects::DlgPrefEffects(QWidget* pParent,
// Highlight first row
availableEffectsList->selectRow(0);

availableEffectsList->setColumnWidth(0, 50);
availableEffectsList->horizontalHeader()->setResizeMode(QHeaderView::ResizeToContents);
}

DlgPrefEffects::~DlgPrefEffects() {
Expand All @@ -44,8 +44,8 @@ void DlgPrefEffects::slotUpdate() {

void DlgPrefEffects::slotApply() {
for (EffectProfilePtr profile : m_pAvailableEffectsModel->profiles()) {
EffectManifest* pManifest = profile->getManifest();
pManifest->setVisibility(profile->isVisible());
EffectManifestPointer pManifest = profile->getManifest();
m_pEffectsManager->setEffectVisibility(pManifest, profile->isVisible());
m_pConfig->set(ConfigKey("[Visible Effects]", pManifest->id()), ConfigValue(profile->isVisible()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I was initially thinking of using a comma or semicolon separated list for the hidden effects, but this approach has simpler code and is more robust in case the effect name has the delimiter in it. 👍

}
}
Expand Down
37 changes: 7 additions & 30 deletions src/preferences/effectprofile.cpp
Original file line number Diff line number Diff line change
@@ -1,34 +1,11 @@
#include <QEventLoop>
#include <QFile>
#include <QFileInfo>
#include <QTextStream>
#include <QRegExp>
#include <QString>
#include <QStringList>

#ifdef __QTKEYCHAIN__
#include <qtkeychain/keychain.h>
using namespace QKeychain;
#endif

// #include "broadcast/defs_broadcast.h"
#include "defs_urls.h"
#include "util/xml.h"
#include "util/memory.h"
#include "util/logger.h"

#include "preferences/effectprofile.h"

namespace {
const mixxx::Logger kLogger("EffectProfile");
} // anonymous namespace

EffectProfile::EffectProfile(EffectManifest &pManifest,
QObject* parent)
: QObject(parent) {

m_isVisible = pManifest.isVisible();
m_pManifest = &pManifest;
EffectProfile::EffectProfile(EffectManifestPointer pManifest,
bool visibility,
QObject* parent)
: QObject(parent),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a QObject subclass? I think a simple struct will work.

m_pManifest(pManifest),
m_isVisible(visibility) {
}

QString EffectProfile::getEffectId() const {
Expand All @@ -47,6 +24,6 @@ void EffectProfile::setVisibility(bool value) {
m_isVisible = value;
}

EffectManifest* EffectProfile::getManifest() const {
EffectManifestPointer EffectProfile::getManifest() const {
return m_pManifest;
}
12 changes: 7 additions & 5 deletions src/preferences/effectprofile.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
#include <QObject>
#include <QString>

#include "preferences/usersettings.h"
#include "effects/defs.h"
#include "effects/effectmanifest.h"
#include "preferences/usersettings.h"

class EffectProfile;
typedef QSharedPointer<EffectProfile> EffectProfilePtr;
Expand All @@ -16,18 +17,19 @@ class EffectProfile : public QObject {
Q_OBJECT

public:
EffectProfile(EffectManifest &pManifest,
QObject* parent = NULL);
EffectProfile(EffectManifestPointer pManifest,
bool visibility,
QObject* parent = NULL);

QString getEffectId() const;
QString getDisplayName() const;
bool isVisible() const;
void setVisibility(bool value);
EffectManifest* getManifest() const;
EffectManifestPointer getManifest() const;

private:
EffectManifestPointer m_pManifest;
bool m_isVisible;
EffectManifest* m_pManifest;
};

#endif // EFFECTPROFILE_H
16 changes: 7 additions & 9 deletions src/preferences/effectsettingsmodel.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include <preferences/effectsettingsmodel.h>

// #include <preferences/effectsettings.h>
#include "effects/effectmanifest.h"

namespace {
const int kColumnEnabled = 0;
Expand All @@ -24,8 +22,8 @@ void EffectSettingsModel::resetFromEffectManager(EffectsManager* pEffectsManager
m_profiles.clear();

for(EffectManifestPointer pManifest : pEffectsManager->getAvailableEffectManifests()) {
// qDebug() << "Manifest: " << pManifest->id();
addProfileToModel(EffectProfilePtr(new EffectProfile(*pManifest)));
const bool visibility = pEffectsManager->getEffectVisibility(pManifest);
addProfileToModel(EffectProfilePtr(new EffectProfile(pManifest, visibility)));
}
}

Expand All @@ -36,7 +34,7 @@ bool EffectSettingsModel::addProfileToModel(EffectProfilePtr profile) {
int position = m_profiles.size();
beginInsertRows(QModelIndex(), position, position);

m_profiles.insert(profile->getEffectId(), EffectProfilePtr(profile));
m_profiles.push_back(EffectProfilePtr(profile));

endInsertRows();
return true;
Expand All @@ -46,12 +44,12 @@ void EffectSettingsModel::deleteProfileFromModel(EffectProfilePtr profile) {
if(!profile)
return;

int position = m_profiles.keys().indexOf(profile->getEffectId());
int position = m_profiles.indexOf(profile);
if(position > -1) {
beginRemoveRows(QModelIndex(), position, position);
endRemoveRows();
}
m_profiles.remove(profile->getEffectId());
m_profiles.removeAll(profile);
}

int EffectSettingsModel::rowCount(const QModelIndex& parent) const {
Expand All @@ -69,7 +67,7 @@ QVariant EffectSettingsModel::data(const QModelIndex& index, int role) const {
if (!index.isValid() || rowIndex >= m_profiles.size())
return QVariant();

EffectProfilePtr profile = m_profiles.values().at(rowIndex);
EffectProfilePtr profile = m_profiles.at(rowIndex);
if (profile) {
if (role == Qt::UserRole)
return profile->getEffectId();
Expand Down Expand Up @@ -116,7 +114,7 @@ Qt::ItemFlags EffectSettingsModel::flags(const QModelIndex& index) const {

bool EffectSettingsModel::setData(const QModelIndex& index, const QVariant& value, int role) {
if(index.isValid()) {
EffectProfilePtr profile = m_profiles.values().at(index.row());
EffectProfilePtr profile = m_profiles.at(index.row());
if(profile) {
if(index.column() == kColumnEnabled && role == Qt::CheckStateRole) {
profile->setVisibility(value.toBool());
Expand Down
Loading