From 1784fc04be57d13b4c2eded9f1fe21d2b187ce24 Mon Sep 17 00:00:00 2001
From: Be <be@mixxx.org>
Date: Sat, 4 Apr 2020 15:11:54 -0500
Subject: [PATCH] add documentation for effects classes in the main thread

---
 src/effects/effectbuttonparameterslot.h     |  3 +--
 src/effects/effectchainslot.h               | 25 +++++++++++++++++++++
 src/effects/effectknobparameterslot.h       |  3 +--
 src/effects/effectmanifest.h                | 13 ++++++-----
 src/effects/effectparameter.h               | 18 ++++++++++-----
 src/effects/effectparameterslotbase.h       |  9 +++++---
 src/effects/effectsbackend.h                | 15 ++++++++++++-
 src/effects/effectslot.h                    | 21 +++++++++++++++++
 src/effects/effectsmanager.h                | 23 ++++++++++++++++---
 src/effects/lv2/lv2backend.h                |  1 +
 src/effects/lv2/lv2effectprocessor.h        |  1 +
 src/effects/lv2/lv2manifest.h               |  1 +
 src/effects/presets/effectchainpreset.h     |  7 +++---
 src/effects/presets/effectparameterpreset.h |  5 +++--
 src/effects/presets/effectpreset.h          |  5 +++--
 15 files changed, 121 insertions(+), 29 deletions(-)

diff --git a/src/effects/effectbuttonparameterslot.h b/src/effects/effectbuttonparameterslot.h
index 8e1aa847361a..b7ca06c7201a 100644
--- a/src/effects/effectbuttonparameterslot.h
+++ b/src/effects/effectbuttonparameterslot.h
@@ -12,8 +12,7 @@
 class ControlObject;
 class ControlPushButton;
 
-// EffectButtonParameterSlot is a wrapper around the parameterX ControlObject
-// that loaded with an EffectParameter into itself by the EffectSlot.
+// Refer to EffectParameterSlotBase for documentation
 class EffectButtonParameterSlot : public EffectParameterSlotBase {
     Q_OBJECT
   public:
diff --git a/src/effects/effectchainslot.h b/src/effects/effectchainslot.h
index ddfaaf7a5358..2b519ead2894 100644
--- a/src/effects/effectchainslot.h
+++ b/src/effects/effectchainslot.h
@@ -21,6 +21,31 @@ class EffectsManager;
 class EffectProcessor;
 class EngineEffectChain;
 
+// EffectChainSlot is the main thread representation of an effect chain which
+// adds/removes exactly one EngineEffectChain from the engine. Unlike EffectSlot,
+// EffectChainSlot does not add/remove EngineEffectChains apart from Mixxx
+// startup and shutdown. The lifetime of EngineEffectChain is coupled with
+// EffectChainSlot. Do not change this relationship; there is no use case for
+// that.
+
+// EffectChainSlot owns the ControlObjects for the routing switches that assign
+// chains to process audio inputs (decks, microphones, auxiliary inputs,
+// master mix). EffectChainSlot also owns the ControlObject for the superknob
+// which manipulates the metaknob of each effect in the chain.
+
+// The state of an EffectChainSlot can be saved to and loaded from an
+// EffectChainPreset, which can serialize/deserialize that state to/from XML.
+// Loading state from an EffectChainPreset is done by
+// EffectsManager::loadEffectChainPreset rather than directly by EffectChainSlot
+// because loading effects requires access to the EffectsBackends and default
+// EffectPresets which are maintained by EffectsManager.
+
+// Currently EffectChainSlot has a fixed number of EffectSlots. In the future
+// this may be extended to create an Effect class to decouple an EngineEffect's
+// state from the ControlObjects so that EffectChainSlot could arbitrarily hide
+// and rearrange Effects by loading/unloading them from EffectSlots. This would
+// be similar to the relationship between EffectSlot and
+// EffectParameterSlotBase/EffectParameter.
 class EffectChainSlot : public QObject {
     Q_OBJECT
   public:
diff --git a/src/effects/effectknobparameterslot.h b/src/effects/effectknobparameterslot.h
index 32d36e7a037f..d622a8d62130 100644
--- a/src/effects/effectknobparameterslot.h
+++ b/src/effects/effectknobparameterslot.h
@@ -15,8 +15,7 @@ class ControlEffectKnob;
 class SoftTakeover;
 class EffectSlot;
 
-// EffectKnobParameterSlot is a wrapper around the parameterX ControlObject
-// that loaded with an EffectParameter into itself by the EffectSlot.
+// Refer to EffectParameterSlotBase for documentation
 class EffectKnobParameterSlot : public EffectParameterSlotBase {
     Q_OBJECT
   public:
diff --git a/src/effects/effectmanifest.h b/src/effects/effectmanifest.h
index f608d7d5a271..4d30650641d1 100644
--- a/src/effects/effectmanifest.h
+++ b/src/effects/effectmanifest.h
@@ -9,11 +9,14 @@
 #include "effects/effectmanifestparameter.h"
 #include "effects/defs.h"
 
-// An EffectManifest is a full description of the metadata associated with an
-// effect (e.g. name, author, version, description, etc.) and the parameters of
-// the effect that are intended to be exposed to the rest of Mixxx for user or
-// script control.
-//
+// An EffectManifest is a description of the metadata associated with an effect
+// (ID, display name, author, description) and all the parameters of the effect.
+// The pair of the ID string and EffectBackendType uniquely identifies an
+// effect. EffectManifests are used by EffectBackends to create EffectProcessors
+// which implement the DSP logic of the effect. The name string of effect
+// parameters in the manifest is used to link EngineEffectParameters
+// with member variables of the EffectProcessorImpl used in the DSP logic.
+
 // EffectManifest is composed purely of simple data types, and when an
 // EffectManifest is const, it should be completely immutable. EffectManifest is
 // meant to be used in most cases as a reference, and in Qt collections, so it
diff --git a/src/effects/effectparameter.h b/src/effects/effectparameter.h
index 48347d3bdf15..d010be4f84b6 100644
--- a/src/effects/effectparameter.h
+++ b/src/effects/effectparameter.h
@@ -13,12 +13,18 @@ class EffectsManager;
 class EngineEffect;
 class EffectParameterPreset;
 
-// An EffectParameter is a wrapper around EffectManifestParameter that tracks a
-// mutable value state and communicates that state to the engine. This class is
-// NOT thread-safe and must only be used from the main thread. Separating this
-// from the parameterX ControlObjects in EffectParameterSlot allows for decoupling
-// the state of the parameters from the ControlObject states, which is required for
-// parameter hiding and rearrangement.
+// An EffectParameter is a main thread representation of the state of an
+// EngineEffectParameter. EffectParameter tracks a mutable value state and
+// communicates that state to the engine. Separating this from the parameterX
+// ControlObjects in EffectParameterSlotBase allows for decoupling the state of
+// the parameters from the ControlObject states, which is required for
+// EffectSlot to do parameter hiding and rearrangement. EffectParameter is
+// responsible for manipulating the value of knob parameters when the metaknob
+// of the EffectSlot is changed (button parameters cannot be linked to the
+// metaknob). For EffectParameter, there is no difference between knobs and
+// buttons; only EffectSlot and
+// EffectKnobParameterSlot/EffectButtonParameterSlot are responsible for taking
+// care of that difference.
 class EffectParameter {
   public:
     EffectParameter(EngineEffect* pEngineEffect, EffectsManager* pEffectsManager, int iParameterNumber, EffectManifestParameterPointer pParameter, EffectParameterPreset preset);
diff --git a/src/effects/effectparameterslotbase.h b/src/effects/effectparameterslotbase.h
index 61f1a7721cde..0ea793b5f340 100644
--- a/src/effects/effectparameterslotbase.h
+++ b/src/effects/effectparameterslotbase.h
@@ -14,9 +14,12 @@ class ControlPushButton;
 class EffectParameter;
 class EffectSlot;
 
-
-// EffectParameterSlotBase is a wrapper around the parameterX ControlObject
-// that loaded with an EffectParameter into itself by the EffectSlot.
+// EffectParameterSlotBase is a wrapper around the parameterX ControlObject.
+// EffectSlot loads/unloads an EffectParameter from the EffectParameterSlotBase.
+// The EffectParameter is responsible for communicating changes in the parameter
+// value to the EngineEffectParameter. The separation of EffectParameter and
+// EffectParameterSlotBase allows EffectSlot to arbitrarily hide and rearrange
+// parameters.
 class EffectParameterSlotBase : public QObject {
     Q_OBJECT
   public:
diff --git a/src/effects/effectsbackend.h b/src/effects/effectsbackend.h
index a9b462000a39..0ff3361820f5 100644
--- a/src/effects/effectsbackend.h
+++ b/src/effects/effectsbackend.h
@@ -14,7 +14,20 @@
 
 class EffectProcessor;
 
-// EffectsBackend enumerates available effects and instantiates EffectProcessors
+// EffectsBackend is an abstract base class that enumerates available effects
+// which are identified by EffectManifests. EffectsBackend creates
+// EffectProcessors when provided with an EffectManifest from EffectsManager
+// indicating which specific EffectProcessor type to create.
+
+// The EffectProcessors implement the DSP logic specific to each effect.
+// EffectManager sends the EffectProcessors down to the EffectChainSlot, which
+// sends it down to the EffectSlot. The EffectSlot uses the EffectProcessor to
+// create an EngineEffect and add/remove the EngineEffect from the engine.
+
+// Currently the implemented EffectsBackend subclasses are for the effects
+// built into Mixxx and LV2 plugins. Other plugin types such as VSTs could be
+// added in the future by creating new subclasses of EffectsBackend,
+// EffectManifest, EffectState, and EffectProcessorImpl.
 class EffectsBackend {
   public:
     virtual ~EffectsBackend() {};
diff --git a/src/effects/effectslot.h b/src/effects/effectslot.h
index dcbaa67f7a63..ff43aabf5557 100644
--- a/src/effects/effectslot.h
+++ b/src/effects/effectslot.h
@@ -32,6 +32,27 @@ class EffectKnobParameterSlot;
 
 typedef QMap<EffectManifestParameter::ParameterType, QList<EffectParameterPointer>> ParameterMap;
 
+// EffectSlot is a main thread class which creates EngineEffects and sends
+// updates to them in response to changes in ControlObjects. It owns the
+// ControlObjects for enabling/disabling effects and the effect metaknob.
+
+// EffectSlot owns a list of EffectParameterSlotBases and EffectParameters.
+// The EffectParameters are the main thread representation of the state of an
+// EngineEffectParameter. EffectSlot creates and destroys EffectParameters
+// together with the EngineEffect as it loads/unloads EngineEffects. The
+// EffectParameterSlotBases own the ControlObjects for manipulating the
+// EffectParameters and showing them in skins. The EffectParameterSlotBases are
+// permanent for the lifetime the EffectSlot; they are not created or destroyed
+// when loading/unloading effects.
+
+// The separation of EffectParameters from EffectParameterSlotBases decouples
+// the parameters available from manipulation via ControlObjects from the
+// parameters of EngineEffects. This allows EffectSlot to arbitrarily hide and
+// rearrange EffectParameters by loading/unloading them from the
+// EffectParameterSlotBases without changing the audio processing in the engine.
+
+// The state of an EffectSlot is loaded from an EffectPreset and a snapshot
+// of EffectSlot's state can be serialized into a EffectPreset.
 class EffectSlot : public QObject {
     Q_OBJECT
   public:
diff --git a/src/effects/effectsmanager.h b/src/effects/effectsmanager.h
index dad6cc5a0967..02936e525fca 100644
--- a/src/effects/effectsmanager.h
+++ b/src/effects/effectsmanager.h
@@ -25,6 +25,26 @@ class EffectManifest;
 
 typedef QMap<EffectManifestParameter::ParameterType, QList<EffectParameterPointer>> ParameterMap;
 
+// EffectsManager is the interface between the parts of the effects system in
+// the main thread and the rest of Mixxx. It creates/destroys a fixed
+// set of EffectChainSlots on Mixxx startup/shutdown. EffectManager uses
+// EffectBackends and EffectManifests to create EffectProcessors. The
+// EffectProcessors are sent down to the EffectChainSlots, then down to the
+// EffectSlots which use the EffectProcessors to create EngineEffects and add
+// them to the audio engine.
+
+// EffectsManager saves/loads EffectChainPresets and EffectPresets.
+// EffectsManager maintains a list of custom EffectChainPresets in the
+// "effects/chains" folder in the user settings folder. The state of loaded
+// effects are saved as EffectChainPresets in the effects.xml file in the user
+// settings folder, which is used to restore the state of effects on startup.
+// Additionally, EffectsManager saves/loads EffectPresets in the
+// "effects/defaults" folder in the user settings folder to allow users to
+// specify default states when each effect is loaded.
+
+// To maintain clear separation of responsibilities, GUI classes should NOT
+// access the EffectChainSlots or EffectSlots directly. They should interface
+// with them indirectly through EffectsManager.
 class EffectsManager : public QObject {
     Q_OBJECT
   public:
@@ -53,9 +73,6 @@ class EffectsManager : public QObject {
     // EffectChainSlotPointer getNextEffectChain(EffectChainSlotPointer pEffectChainSlot);
     // EffectChainSlotPointer getPrevEffectChain(EffectChainSlotPointer pEffectChainSlot);
 
-    // NOTE(Kshitij) : New functions for saving and loading
-    // bool saveEffectChains();
-
     static const int kNumStandardEffectChains = 4;
 
     bool isAdoptMetaknobValueEnabled() const;
diff --git a/src/effects/lv2/lv2backend.h b/src/effects/lv2/lv2backend.h
index bfa7503d8f5b..5b0fb09cd8aa 100644
--- a/src/effects/lv2/lv2backend.h
+++ b/src/effects/lv2/lv2backend.h
@@ -7,6 +7,7 @@
 #include "preferences/usersettings.h"
 #include <lilv-0/lilv/lilv.h>
 
+// Refer to EffectsBackend for documentation
 class LV2Backend : public EffectsBackend {
   public:
     LV2Backend();
diff --git a/src/effects/lv2/lv2effectprocessor.h b/src/effects/lv2/lv2effectprocessor.h
index 923546fa8a8f..92ae7186ae48 100644
--- a/src/effects/lv2/lv2effectprocessor.h
+++ b/src/effects/lv2/lv2effectprocessor.h
@@ -8,6 +8,7 @@
 #include "effects/defs.h"
 #include "engine/engine.h"
 
+// Refer to EffectProcessor for documentation
 class LV2EffectGroupState final: public EffectState {
   public:
     LV2EffectGroupState(const mixxx::EngineParameters& bufferParameters)
diff --git a/src/effects/lv2/lv2manifest.h b/src/effects/lv2/lv2manifest.h
index 869757b1d102..c316e6a53ac8 100644
--- a/src/effects/lv2/lv2manifest.h
+++ b/src/effects/lv2/lv2manifest.h
@@ -7,6 +7,7 @@
 #include "effects/defs.h"
 #include <lilv-0/lilv/lilv.h>
 
+// Refer to EffectManifest for documentation
 class LV2Manifest : public EffectManifest {
   public:
     enum Status {
diff --git a/src/effects/presets/effectchainpreset.h b/src/effects/presets/effectchainpreset.h
index 35b29efb4269..15f6cff492ae 100644
--- a/src/effects/presets/effectchainpreset.h
+++ b/src/effects/presets/effectchainpreset.h
@@ -7,9 +7,9 @@
 class EffectChainSlot;
 
 // EffectChainPreset is a read-only snapshot of the state of an effect chain
-// that can be serialized to/deserialized from XML. It is used to easily
-// save/load user-defined chain presets as well as save the state of loaded
-// effects when Mixxx shuts down and restarts.
+// that can be serialized to/deserialized from XML. It is used by EffectsManager
+// to easily save/load user-defined chain presets as well as save the state of
+// loaded effects when Mixxx shuts down and restarts.
 class EffectChainPreset {
   public:
     EffectChainPreset();
@@ -23,6 +23,7 @@ class EffectChainPreset {
         return m_name.isEmpty();
     }
 
+    // This is the only exception to EffectChainPreset being read-only.
     void setName(const QString& newName) {
         m_name = newName;
     }
diff --git a/src/effects/presets/effectparameterpreset.h b/src/effects/presets/effectparameterpreset.h
index 614e049f6d2c..e629b25c9d42 100644
--- a/src/effects/presets/effectparameterpreset.h
+++ b/src/effects/presets/effectparameterpreset.h
@@ -5,8 +5,9 @@
 #include "effects/defs.h"
 #include "effects/effectmanifestparameter.h"
 
-// EffectParameterPreset is a read-only snapshot of the state of an effect parameter
-// that can be serialized to/deserialized from XML.
+// EffectParameterPreset is a read-only snapshot of the state of an effect
+// parameter that can be serialized to/deserialized from XML. It is only used
+// as a component of an EffectPreset; never on its own.
 class EffectParameterPreset {
   public:
     EffectParameterPreset();
diff --git a/src/effects/presets/effectpreset.h b/src/effects/presets/effectpreset.h
index 589f6fa9e385..930445f96936 100644
--- a/src/effects/presets/effectpreset.h
+++ b/src/effects/presets/effectpreset.h
@@ -6,8 +6,9 @@
 #include "effects/presets/effectparameterpreset.h"
 
 // EffectPreset is a read-only snapshot of the state of an effect that can be
-// serialized to/deserialized from XML. It is used to save/load chain presets
-// as well as save custom defaults for each effect.
+// serialized to/deserialized from XML. It is used by EffectChainPreset to
+// save/load chain presets. It is also used by EffectsManager to save custom
+// defaults for each effect.
 class EffectPreset {
   public:
     EffectPreset();