Presets bug fixes and refactoring (WIP)

include/ClapPresetDatabase.h

@@ -37,6 +37,7 @@
 #include <clap/factory/preset-discovery.h>
 
 #include "ClapExtension.h"
+#include "lmms_filesystem.h"
 #include "NoCopyNoMove.h"
 #include "PresetDatabase.h"
 
@@ -53,8 +54,6 @@ class ClapPresetDatabase : public PresetDatabase
 	auto init(const clap_plugin_entry* entry) -> bool;
 	void deinit();
 
-	auto discover() -> bool override;
-
 	auto supported() const -> bool { return m_factory; }
 
 	static auto toClapLocation(std::string_view location, std::string& ref)
@@ -64,7 +63,13 @@ class ClapPresetDatabase : public PresetDatabase
 		const char* location, const char* loadKey) -> std::optional<PresetLoadData>;
 
 private:
-	auto createPreset(const Preset::LoadData& loadData) const -> std::optional<Preset> override;
+	auto discoverSetup() -> bool override;
+	auto discoverFiletypes(std::vector<Filetype>& filetypes) -> bool override;
+	auto discoverLocations(const SetLocations& func) -> bool override;
+	auto discoverPresets(const Location& location, std::set<Preset>& presets) -> bool override;
+
+	auto loadPresets(const Location& location, std::string_view file, std::set<Preset>& presets)
+		-> std::vector<const Preset*> override;
 
 	class Indexer : public NoCopyNoMove
 	{
@@ -77,19 +82,17 @@ class ClapPresetDatabase : public PresetDatabase
 			const clap_preset_discovery_provider_descriptor& descriptor);
 		~Indexer() = default;
 
-		auto query(std::string_view location, PresetMetadata::Flags flags = PresetMetadata::Flag::None)
+		//! For PLUGIN presets
+		auto query(PresetMetadata::Flags flags = PresetMetadata::Flag::None)
 			-> std::optional<std::vector<Preset>>;
 
-		auto provider() const -> const clap_preset_discovery_provider* { return m_provider.get(); }
+		//! For FILE presets; `file` is the full path of the preset file
+		auto query(std::string_view file,
+			PresetMetadata::Flags flags = PresetMetadata::Flag::None) -> std::optional<std::vector<Preset>>;
 
-		//! Like clap_preset_discovery_location, for storing declare_location() data
-		struct Location
-		{
-			PresetMetadata::Flags flags;
-			std::string name;
-			clap_preset_discovery_location_kind kind;
-			std::string location; //!< PathUtil-compatible
-		};
+		auto filetypeSupported(fs::path path) const -> bool;
+
+		auto provider() const -> const clap_preset_discovery_provider* { return m_provider.get(); }
 
 		auto locations() -> auto& { return m_locations; }
 		auto filetypes() -> auto& { return m_filetypes; }
@@ -118,10 +121,12 @@ class ClapPresetDatabase : public PresetDatabase
 		clap_preset_discovery_indexer m_indexer;
 		std::unique_ptr<const clap_preset_discovery_provider, ProviderDeleter> m_provider;
 
-		std::vector<Location> m_locations;
+		std::vector<PresetDatabase::Location> m_locations;
 		std::vector<PresetDatabase::Filetype> m_filetypes;
 	};
 
+	auto getIndexerFor(const Location& location) const -> Indexer*;
+
 	class MetadataReceiver;
 
 	const clap_preset_discovery_factory* m_factory = nullptr;

include/Preset.h

@@ -32,7 +32,6 @@
 
 #include "Flags.h"
 #include "NoCopyNoMove.h"
-//#include "Plugin.h"
 
 namespace lmms
 {
@@ -42,26 +41,22 @@ struct PresetLoadData
 {
 	/**
 	 * A string that can be parsed by PathUtil.
-	 * Its meaning depends on the plugin, but it typically represents a preset directory.
+	 * Its meaning depends on the plugin, but it typically represents a preset file path.
 	 *
 	 * Valid examples:
-	 * - "userprojects:"
-	 * - "preset:MyPlugin/MyFavoritePresets"
-	 * - "/my/absolute/directory"
-	 * - "preset:my_plugin/my_preset_database.txt" (depending on the plugin, it could even be a file!)
+	 * - "preset:MyPlugin/MyFavoritePresets/foo.preset"
+	 * - "/my/absolute/directory/bar.ext"
+	 * - "preset:foo/preset_database.txt" (could be a container of presets, in which case `loadKey` is non-empty)
 	 * - "internal:" (for presets stored within the plugin's DSO rather than on disk)
 	 */
 	std::string location;
 
 	/**
-	 * Could be a preset filename, sub-path + preset filename, file offset, or any other kind of unique ID.
+	 * If non-empty, could be a file offset or any other kind of unique ID.
 	 * Again, it's entirely up to the plugin.
 	 */
 	std::string loadKey;
 
-	// Whether `location` and `loadKey` can be concatenated together
-	// to form a single string depends entirely on the plugin.
-
 	friend auto operator==(const PresetLoadData& lhs, const PresetLoadData& rhs) -> bool
 	{
 		return lhs.location == rhs.location && lhs.loadKey == rhs.loadKey;
@@ -95,23 +90,6 @@ LMMS_DECLARE_OPERATORS_FOR_FLAGS(PresetMetadata::Flag)
 class Preset
 {
 public:
-	/**
-	 * Same as `PresetLoadData`, but `location` is a std::string_view that references a key
-	 * within `PresetDatabase::m_presets` in order to save space when many presets are loaded.
-	 */
-	struct LoadData
-	{
-		std::string_view location;
-		std::string loadKey;
-
-		friend auto operator==(const LoadData& lhs, const PresetLoadData& rhs) noexcept -> bool
-		{
-			return lhs.location == rhs.location && lhs.loadKey == rhs.loadKey;
-		}
-
-		operator PresetLoadData() const { return {std::string{location}, loadKey}; }
-	};
-
 	auto metadata() -> auto& { return m_metadata; }
 	auto metadata() const -> auto& { return m_metadata; }
 
@@ -135,7 +113,7 @@ class Preset
 
 private:
 	PresetMetadata m_metadata;
-	LoadData m_loadData;
+	PresetLoadData m_loadData;
 
 	//! Subplugin keys that support this preset
 	//!   Empty if all subplugins support the preset or if there are no subplugins

include/PresetDatabase.h

@@ -42,48 +42,71 @@ namespace lmms
  * Plugins are expected to inherit this class to implement preset discovery,
  *   metadata loading, and other functionality.
  */
-class PresetDatabase : public NoCopyNoMove
+class PresetDatabase
 {
 public:
-	using PresetMap = std::map<std::string, std::set<Preset>, std::less<>>;
-
 	struct Filetype
 	{
 		std::string name;
 		std::string description;
-		std::string extension;
+		std::string extension; //< without dot; if empty, any extension is supported
+	};
+
+	//! Represents a preset directory or internal location where presets can be found
+	struct Location
+	{
+		std::string name;
+		std::string location; //!< PathUtil-compatible
+		PresetMetadata::Flags flags = PresetMetadata::Flag::None;
+
+		friend auto operator==(const Location& lhs, const Location& rhs) noexcept -> bool
+		{
+			return lhs.location == rhs.location;
+		}
+
+		friend auto operator<(const Location& lhs, const Location& rhs) noexcept -> bool
+		{
+			return lhs.location < rhs.location;
+		}
+
+		friend auto operator<(std::string_view location, const Location& rhs) noexcept -> bool
+		{
+			return location < rhs.location;
+		}
+
+		friend auto operator<(const Location& lhs, std::string_view location) noexcept -> bool
+		{
+			return lhs.location < location;
+		}
 	};
 
+	using PresetMap = std::map<Location, std::set<Preset>, std::less<>>;
+
 	PresetDatabase() = default;
-	//using QObject::QObject;
-	//PresetDatabase(std::string_view key);
 	virtual ~PresetDatabase() = default;
 
 	//! Discover presets and populate database; Returns true when successful
-	virtual auto discover() -> bool = 0;
+	auto discover() -> bool;
 
-	//! Load a preset file from disk; Returns nullptr upon failure or if preset was already added
-	auto addPreset(std::string_view path) -> const Preset*;
+	//! Load a preset file from disk; Returns empty vector upon failure or if preset(s) were already added
+	auto addPresets(std::string_view path) -> std::vector<const Preset*>;
 
 	/**
 	 * Accessors
 	 */
 
 	auto presets() const -> auto& { return m_presets; }
 
-	auto presets(const std::string& location) const -> const std::set<Preset>&
-	{
-		return m_presets.at(location);
-	}
-
-	auto presets(const std::string& location) -> std::set<Preset>&
+	auto presets(std::string_view location) const -> const std::set<Preset>*
 	{
-		return m_presets.at(location);
+		const auto it = m_presets.find(location);
+		return it != m_presets.end() ? &it->second : nullptr;
 	}
 
-	auto presets(std::string_view location) -> PresetMap::iterator
+	auto presets(std::string_view location) -> std::set<Preset>*
 	{
-		return m_presets.find(location);
+		const auto it = m_presets.find(location);
+		return it != m_presets.end() ? &it->second : nullptr;
 	}
 
 	auto filetypes() const -> auto& { return m_filetypes; }
@@ -94,7 +117,7 @@ class PresetDatabase : public NoCopyNoMove
 	auto findPreset(const PresetLoadData& loadData, std::string_view key = std::string_view{}) const -> const Preset*;
 
 	//! Open preset dialog TODO: Move to an lmms::gui class?
-	auto openPresetFile(std::string_view previousFile) -> const Preset*;
+	auto openPresetFile(std::string_view previousFile) -> std::vector<const Preset*>;
 
 	//! Save preset dialog TODO: Move to an lmms::gui class?
 	auto savePresetFile(const Preset& preset) -> bool;
@@ -106,43 +129,70 @@ class PresetDatabase : public NoCopyNoMove
 
 protected:
 
-	//! Creates a populated Preset object from Preset::LoadData.
-	//! Plugins should override this to provide better preset metadata.
-	virtual auto createPreset(const Preset::LoadData& loadData) const -> std::optional<Preset>;
+	/**
+	 * 1st step of `discover()` - optional setup
+	 *
+	 * Return true if successful
+	 */
+	virtual auto discoverSetup() -> bool { return true; }
+
+	/**
+	 * 2nd step of `discover()` - declare all supported file types
+	 *
+	 * Return true if successful
+	 */
+	virtual auto discoverFiletypes(std::vector<Filetype>& filetypes) -> bool = 0;
 
-	//! Creates a populated Preset object from Preset::LoadData and key(s).
-	//! Plugins should override this to provide better preset metadata.
-	virtual auto createPreset(const Preset::LoadData& loadData,
-		const std::vector<std::string>& keys) const -> std::optional<Preset>;
+	//! Function object to make `discoverLocations()` implementation simpler and hide implementation details
+	class SetLocations
+	{
+	public:
+		friend class PresetDatabase;
+		void operator()(const std::vector<Location>& locations) const;
+		void operator()(Location location) const;
+	private:
+		SetLocations(PresetMap& presets) : m_map{&presets} {}
+		PresetMap* m_map = nullptr;
+	};
 
 	/**
-	 * Adds a preset file to `m_presets` if it doesn't exist, and returns the load data.
+	 * 3rd step of `discover()` - declare all pre-established preset locations
 	 *
-	 * Checks all the combinations for splitting `path` into location and load key before adding.
-	 * TODO: Return map iterator so that `m_presets` does not need to be searched again?
+	 * Return true if successful
 	 */
-	auto addFile(std::string_view path) -> std::optional<Preset::LoadData>;
+	virtual auto discoverLocations(const SetLocations& func) -> bool = 0;
 
-	//! Use during discover() call; The returned iterator is used to add presets at the location
-	auto addLocation(std::string_view path) -> PresetMap::iterator;
+	/**
+	 * 4th and final step of `discover()` - populate set of presets at the given location
+	 *
+	 * Return true if successful
+	 */
+	virtual auto discoverPresets(const Location& location, std::set<Preset>& presets) -> bool = 0;
 
-	//! Use during discover() call
-	void addFiletype(Filetype filetype)
-	{
-		m_filetypes.push_back(std::move(filetype));
-	}
+	/**
+	 * Loads presets for the given location from `file`, retrieves any metadata,
+	 * and adds the new presets to `presets`. Returns all the new presets.
+	 *
+	 * Used when the user loads a new preset from disk.
+	 * May also be used by `discoverPresets()`.
+	 *
+	 * The default implementation only works for simple preset files.
+	 * Plugins that support preset containers (where multiple presets could potentially be returned
+	 * from this method) or provide additional preset metadata should reimplement this method.
+	 */
+	virtual auto loadPresets(const Location& location, std::string_view file, std::set<Preset>& presets)
+		-> std::vector<const Preset*>;
 
-	//! Use during discover() call
-	void setFiletypes(std::vector<Filetype> filetypes)
-	{
-		m_filetypes = std::move(filetypes);
-	}
+	/**
+	 * Gets preset location from `m_presets` which contains `path`, optionally adding it if it doesn't exist
+	 *
+	 * Returns iterator to the `m_presets` location
+	 */
+	auto getLocation(std::string_view path, bool add = true) -> PresetMap::iterator;
 
 private:
 
-	//! Maps locations to presets.
-	//! PresetLoadData references this map's key (location), so this map must remain stable after construction.
-	//! See `PresetLoadData::location` for more info.
+	//! Maps locations to presets
 	PresetMap m_presets;
 
 	std::vector<Filetype> m_filetypes;

src/core/PluginPresets.cpp

@@ -154,16 +154,23 @@ void PluginPresets::saveActivePreset(QDomDocument& doc, QDomElement& element)
 
 		QDomElement presetElement = doc.createElement(presetNodeName());
 		element.appendChild(presetElement);
+		qDebug().nospace() << "Saving active preset: location: \"" << location.data() << "\" loadKey: \""
+			<< loadKey.data() << "\"";
 		presetElement.setAttribute("location", QString::fromUtf8(location.data(), location.size()));
 		presetElement.setAttribute("loadKey", QString::fromUtf8(loadKey.data(), loadKey.size()));
 	}
+	else
+	{
+		qDebug() << "No active preset to save";
+	}
 }
 
 void PluginPresets::loadActivePreset(const QDomElement& element)
 {
 	QDomElement presetElement = element.firstChildElement(presetNodeName());
 	if (presetElement.isNull())
 	{
+		qDebug() << "Loading active preset: None found.";
 		setActivePreset(std::nullopt);
 		return;
 	}
@@ -173,10 +180,14 @@ void PluginPresets::loadActivePreset(const QDomElement& element)
 
 	if (location.isEmpty() && loadKey.isEmpty())
 	{
+		qDebug() << "Loading active preset: Active preset data empty.";
 		setActivePreset(std::nullopt);
 		return;
 	}
 
+	qDebug().nospace() << "Loading active preset: location: \"" << location.data() << "\" loadKey: \""
+			<< loadKey.data() << "\"";
+
 	// TODO: The needed preset may not be discovered at this point
 
 	const auto loadData = PresetLoadData{location.toStdString(), loadKey.toStdString()};