diff --git a/framework/audioplugins/internal/knownaudiopluginsregister.cpp b/framework/audioplugins/internal/knownaudiopluginsregister.cpp index d1ee69603a..a6e10cb39e 100644 --- a/framework/audioplugins/internal/knownaudiopluginsregister.cpp +++ b/framework/audioplugins/internal/knownaudiopluginsregister.cpp @@ -207,20 +207,27 @@ Ret KnownAudioPluginsRegister::registerPlugins(const AudioPluginInfoList& list) return make_ok(); } + bool changed = false; + for (const AudioPluginInfo& info : list) { auto it = m_pluginInfoMap.find(info.meta.id); if (it != m_pluginInfoMap.end()) { - IF_ASSERT_FAILED(it->second.path != info.path) { - return false; + if (it->second.path == info.path) { + LOGW() << "Plugin is already registered: " << info.path << ", ID: " << info.meta.id; + continue; } } m_pluginInfoMap.emplace(info.meta.id, info); m_pluginPaths.insert(info.path); + changed = true; } - Ret ret = writePluginsInfo(); - return ret; + if (changed) { + return writePluginsInfo(); + } + + return make_ok(); } Ret KnownAudioPluginsRegister::unregisterPlugins(const AudioResourceIdList& resourceIds) @@ -233,6 +240,8 @@ Ret KnownAudioPluginsRegister::unregisterPlugins(const AudioResourceIdList& reso return make_ok(); } + bool changed = false; + for (const AudioResourceId& resourceId : resourceIds) { if (!exists(resourceId)) { continue; @@ -245,10 +254,14 @@ Ret KnownAudioPluginsRegister::unregisterPlugins(const AudioResourceIdList& reso } m_pluginInfoMap.erase(resourceId); + changed = true; } - Ret ret = writePluginsInfo(); - return ret; + if (changed) { + return writePluginsInfo(); + } + + return make_ok(); } Ret KnownAudioPluginsRegister::writePluginsInfo() diff --git a/framework/audioplugins/internal/registeraudiopluginsscenario.cpp b/framework/audioplugins/internal/registeraudiopluginsscenario.cpp index 892f1e514a..3008d64c61 100644 --- a/framework/audioplugins/internal/registeraudiopluginsscenario.cpp +++ b/framework/audioplugins/internal/registeraudiopluginsscenario.cpp @@ -23,7 +23,7 @@ #include "registeraudiopluginsscenario.h" #include -#include +#include #include "global/translation.h" @@ -55,51 +55,54 @@ PluginScanResult RegisterAudioPluginsScenario::scanPlugins(Progress* progress) c TRACEFUNC; PluginScanResult result; + std::set scannedPaths; - std::map registered; - for (const auto& info : knownPluginsRegister()->pluginInfoList()) { - registered[info.path] = info.meta.id; - } - - for (const auto& scanner : scannerRegister()->scanners()) { - for (const auto& path : scanner->scanPlugins(progress)) { - if (auto it = registered.find(path); it != registered.end()) { - registered.erase(it); - } else { + for (const IAudioPluginsScannerPtr& scanner : scannerRegister()->scanners()) { + for (const io::path_t& path : scanner->scanPlugins(progress)) { + if (!knownPluginsRegister()->exists(path)) { result.newPluginPaths.push_back(path); } + + scannedPaths.insert(path); } } - for (const auto& [path, id] : registered) { - result.missingPluginIds.push_back(id); + for (const AudioPluginInfo& info : knownPluginsRegister()->pluginInfoList()) { + if (!muse::contains(scannedPaths, info.path)) { + result.missingPluginIds.push_back(info.meta.id); + } } return result; } -Ret RegisterAudioPluginsScenario::updatePluginsRegistry() +void RegisterAudioPluginsScenario::updatePluginsRegistry() { TRACEFUNC; - PluginScanResult result = scanPlugins(); + const PluginScanResult result = scanPlugins(); - unregisterRemovedPlugins(result.missingPluginIds); - registerNewPlugins(result.newPluginPaths); + Ret ret = unregisterRemovedPlugins(result.missingPluginIds); + if (!ret) { + LOGE() << "Failed to unregister plugins: " << ret.toString(); + } - return knownPluginsRegister()->load(); + ret = registerNewPlugins(result.newPluginPaths); + if (!ret) { + LOGE() << "Failed to register plugins: " << ret.toString(); + } } -void RegisterAudioPluginsScenario::registerNewPlugins(const io::paths_t& pluginPaths) +Ret RegisterAudioPluginsScenario::registerNewPlugins(const io::paths_t& pluginPaths) { TRACEFUNC; if (pluginPaths.empty()) { - return; + return make_ok(); } processPluginsRegistration(pluginPaths); - knownPluginsRegister()->load(); + return knownPluginsRegister()->load(); } Ret RegisterAudioPluginsScenario::unregisterRemovedPlugins(const audio::AudioResourceIdList& pluginIds) @@ -110,12 +113,7 @@ Ret RegisterAudioPluginsScenario::unregisterRemovedPlugins(const audio::AudioRes return make_ok(); } - Ret ret = knownPluginsRegister()->unregisterPlugins(pluginIds); - if (!ret) { - LOGE() << "Failed to unregister removed plugins: " << ret.toString(); - } - - return ret; + return knownPluginsRegister()->unregisterPlugins(pluginIds); } void RegisterAudioPluginsScenario::processPluginsRegistration(const io::paths_t& pluginPaths) @@ -125,8 +123,8 @@ void RegisterAudioPluginsScenario::processPluginsRegistration(const io::paths_t& m_aborted = false; m_progress.start(); - std::string appPath = globalConfiguration()->appBinPath().toStdString(); - int64_t pluginCount = static_cast(pluginPaths.size()); + const std::string appPath = globalConfiguration()->appBinPath().toStdString(); + const int64_t pluginCount = static_cast(pluginPaths.size()); for (int64_t i = 0; i < pluginCount; ++i) { if (m_aborted) { @@ -134,12 +132,11 @@ void RegisterAudioPluginsScenario::processPluginsRegistration(const io::paths_t& } const io::path_t& pluginPath = pluginPaths[i]; - std::string pluginPathStr = pluginPath.toStdString(); + const std::string pluginPathStr = pluginPath.toStdString(); m_progress.progress(i, pluginCount, io::filename(pluginPath).toStdString()); qApp->processEvents(); - LOGD() << "--register-audio-plugin " << pluginPathStr; int code = process()->execute(appPath, { "--register-audio-plugin", pluginPathStr }); if (code != 0) { code = process()->execute(appPath, { "--register-failed-audio-plugin", pluginPathStr, "--", std::to_string(code) }); diff --git a/framework/audioplugins/internal/registeraudiopluginsscenario.h b/framework/audioplugins/internal/registeraudiopluginsscenario.h index a3c23c3e52..86c8836d64 100644 --- a/framework/audioplugins/internal/registeraudiopluginsscenario.h +++ b/framework/audioplugins/internal/registeraudiopluginsscenario.h @@ -52,8 +52,9 @@ class RegisterAudioPluginsScenario : public IRegisterAudioPluginsScenario, publi PluginScanResult scanPlugins(Progress* progress = nullptr) const override; - Ret updatePluginsRegistry() override; - void registerNewPlugins(const io::paths_t& pluginPaths) override; + void updatePluginsRegistry() override; + + Ret registerNewPlugins(const io::paths_t& pluginPaths) override; Ret unregisterRemovedPlugins(const audio::AudioResourceIdList& pluginIds) override; Ret registerPlugin(const io::path_t& pluginPath) override; diff --git a/framework/audioplugins/iregisteraudiopluginsscenario.h b/framework/audioplugins/iregisteraudiopluginsscenario.h index 3a2cf19c13..92e9e3841b 100644 --- a/framework/audioplugins/iregisteraudiopluginsscenario.h +++ b/framework/audioplugins/iregisteraudiopluginsscenario.h @@ -42,8 +42,9 @@ class IRegisterAudioPluginsScenario : MODULE_CONTEXT_INTERFACE virtual PluginScanResult scanPlugins(Progress* progress = nullptr) const = 0; - virtual Ret updatePluginsRegistry() = 0; - virtual void registerNewPlugins(const io::paths_t& pluginPaths) = 0; + virtual void updatePluginsRegistry() = 0; + + virtual Ret registerNewPlugins(const io::paths_t& pluginPaths) = 0; virtual Ret unregisterRemovedPlugins(const audio::AudioResourceIdList& pluginIds) = 0; virtual Ret registerPlugin(const io::path_t& pluginPath) = 0; diff --git a/framework/audioplugins/tests/registeraudiopluginsscenariotest.cpp b/framework/audioplugins/tests/registeraudiopluginsscenariotest.cpp index d1945ab63f..afd4076fe8 100644 --- a/framework/audioplugins/tests/registeraudiopluginsscenariotest.cpp +++ b/framework/audioplugins/tests/registeraudiopluginsscenariotest.cpp @@ -140,12 +140,6 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry) .WillByDefault(Return(foundPluginPaths)); } - AudioPluginInfo incompatiblePluginInfo; - incompatiblePluginInfo.path = foundPluginPaths[4]; - incompatiblePluginInfo.meta.id = io::filename(incompatiblePluginInfo.path).toStdString(); - incompatiblePluginInfo.enabled = false; - incompatiblePluginInfo.errorCode = -1; - // [GIVEN] Some plugins already exist in the register AudioPluginInfoList alreadyRegisteredPlugins; for (size_t i = 0; i < 2; ++i) { @@ -158,49 +152,45 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry) ON_CALL(*m_knownPlugins, pluginInfoList(_)) .WillByDefault(Return(alreadyRegisteredPlugins)); + for (const AudioPluginInfo& info : alreadyRegisteredPlugins) { + ON_CALL(*m_knownPlugins, exists(info.path)) + .WillByDefault(Return(true)); + } + // [THEN] The progress bar is shown EXPECT_CALL(*m_interactive, showProgress(muse::trc("audio", "Scanning audio plugins"), _)) .Times(1); - // [THEN] Processes started only for unregistered plugins - paths_t alreadyRegisteredPaths { foundPluginPaths[0], foundPluginPaths[1] }; - for (const path_t& pluginPath : foundPluginPaths) { - std::vector args = { "--register-audio-plugin", pluginPath.toStdString() }; - - if (muse::contains(alreadyRegisteredPaths, pluginPath)) { - // Ignore already registered plugins - EXPECT_CALL(*m_process, execute(_, args)) - .Times(0); - } else if (incompatiblePluginInfo.path == pluginPath) { - // Incompatible plugin detected - EXPECT_CALL(*m_process, execute(m_appPath, args)) - .WillOnce(Return(-1)); - - args = { "--register-failed-audio-plugin", pluginPath.toStdString(), "--", "-1" }; - - EXPECT_CALL(*m_process, execute(m_appPath, args)) - .WillOnce(Return(0)); - } else { - // Successfully registered plugins - EXPECT_CALL(*m_process, execute(m_appPath, args)) - .WillOnce(Return(0)); - } - } + // [THEN] Already registered plugins are not processed again + EXPECT_CALL(*m_process, execute(_, std::vector { "--register-audio-plugin", foundPluginPaths[0].toStdString() })) + .Times(0); + EXPECT_CALL(*m_process, execute(_, std::vector { "--register-audio-plugin", foundPluginPaths[1].toStdString() })) + .Times(0); + + // [THEN] New compatible plugins are registered + EXPECT_CALL(*m_process, execute(m_appPath, std::vector { "--register-audio-plugin", foundPluginPaths[2].toStdString() })) + .WillOnce(Return(0)); + EXPECT_CALL(*m_process, execute(m_appPath, std::vector { "--register-audio-plugin", foundPluginPaths[3].toStdString() })) + .WillOnce(Return(0)); + + // [THEN] The incompatible plugin is registered as failed + EXPECT_CALL(*m_process, execute(m_appPath, std::vector { "--register-audio-plugin", foundPluginPaths[4].toStdString() })) + .WillOnce(Return(-1)); + EXPECT_CALL(*m_process, + execute(m_appPath, + std::vector { "--register-failed-audio-plugin", foundPluginPaths[4].toStdString(), "--", "-1" })) + .WillOnce(Return(0)); // [THEN] All plugins remain in the register EXPECT_CALL(*m_knownPlugins, unregisterPlugins(_)) .Times(0); - // [THEN] The register is refreshed + // [THEN] Reloaded once inside registerNewPlugins, after subprocesses finish writing to disk EXPECT_CALL(*m_knownPlugins, load()) - .Times(2) - .WillRepeatedly(Return(muse::make_ok())); + .WillOnce(Return(muse::make_ok())); // [WHEN] Register new plugins - Ret ret = m_scenario->updatePluginsRegistry(); - - // [THEN] Plugins successfully registered - EXPECT_TRUE(ret); + m_scenario->updatePluginsRegistry(); } TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_NoNewPlugins) @@ -231,6 +221,11 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_NoNe ON_CALL(*m_knownPlugins, pluginInfoList(_)) .WillByDefault(Return(alreadyRegisteredPlugins)); + for (const path_t& pluginPath : foundPluginPaths) { + ON_CALL(*m_knownPlugins, exists(pluginPath)) + .WillByDefault(Return(true)); + } + // [THEN] Don't register the plugins again EXPECT_CALL(*m_process, execute(_, _)) .Times(0); @@ -238,14 +233,14 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_NoNe EXPECT_CALL(*m_interactive, showProgress(_, _)) .Times(0); + EXPECT_CALL(*m_knownPlugins, unregisterPlugins(_)) + .Times(0); + EXPECT_CALL(*m_knownPlugins, load()) - .WillOnce(Return(muse::make_ok())); + .Times(0); // [WHEN] Try to register the plugins again - Ret ret = m_scenario->updatePluginsRegistry(); - - // [THEN] No error - EXPECT_TRUE(ret); + m_scenario->updatePluginsRegistry(); } //! See: https://github.com/musescore/MuseScore/issues/16458 @@ -285,6 +280,11 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_Unre .WillByDefault(Return(foundPluginPaths)); } + for (const path_t& path : foundPluginPaths) { + ON_CALL(*m_knownPlugins, exists(path)) + .WillByDefault(Return(true)); + } + // [THEN] Unreg the uninstalled plugins AudioResourceIdList uninstalledPluginIdList { knownPlugins[0].meta.id, knownPlugins[1].meta.id @@ -293,14 +293,117 @@ TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_Unre EXPECT_CALL(*m_knownPlugins, unregisterPlugins(uninstalledPluginIdList)) .WillOnce(Return(make_ok())); + // [THEN] No new plugins to process + EXPECT_CALL(*m_process, execute(_, _)) + .Times(0); + + EXPECT_CALL(*m_interactive, showProgress(_, _)) + .Times(0); + EXPECT_CALL(*m_knownPlugins, load()) - .WillOnce(Return(muse::make_ok())); + .Times(0); // [WHEN] Update registry - Ret ret = m_scenario->updatePluginsRegistry(); + m_scenario->updatePluginsRegistry(); +} - // [THEN] Successfully unregistered - EXPECT_TRUE(ret); +//! A multi-component VST can expose several plugins (different IDs) from a single .vst3 path +//! When that path is still present on disk, none of the components should be re-registered +TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_SamePathDifferentIds_PluginPresent) +{ + path_t sharedPath = "/some/path/MultiPlugin.vst3"; + + // [GIVEN] Two already registered components that share the same path + AudioPluginInfoList knownPlugins; + + AudioPluginInfo mono; + mono.path = sharedPath; + mono.meta.id = "MultiPlugin_Mono"; + knownPlugins.push_back(mono); + + AudioPluginInfo stereo; + stereo.path = sharedPath; + stereo.meta.id = "MultiPlugin_Stereo"; + knownPlugins.push_back(stereo); + + ON_CALL(*m_knownPlugins, pluginInfoList(_)) + .WillByDefault(Return(knownPlugins)); + + // [GIVEN] Scanner still finds the shared path + for (const IAudioPluginsScannerPtr& scanner : m_scanners) { + AudioPluginsScannerMock* mock = dynamic_cast(scanner.get()); + ASSERT_TRUE(mock); + + ON_CALL(*mock, scanPlugins(_)) + .WillByDefault(Return(paths_t { sharedPath })); + } + + ON_CALL(*m_knownPlugins, exists(sharedPath)) + .WillByDefault(Return(true)); + + // [THEN] Neither component is re-registered or unregistered + EXPECT_CALL(*m_process, execute(_, _)) + .Times(0); + + EXPECT_CALL(*m_interactive, showProgress(_, _)) + .Times(0); + + EXPECT_CALL(*m_knownPlugins, unregisterPlugins(_)) + .Times(0); + + EXPECT_CALL(*m_knownPlugins, load()) + .Times(0); + + // [WHEN] Update registry + m_scenario->updatePluginsRegistry(); +} + +//! When a multi-component plugin is uninstalled, ALL of its component IDs must be unregistered +TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, UpdatePluginsRegistry_SamePathDifferentIds_PluginMissing) +{ + path_t sharedPath = "/some/path/MultiPlugin.vst3"; + + // [GIVEN] Two already registered components that share the same path + AudioPluginInfoList knownPlugins; + + AudioPluginInfo mono; + mono.path = sharedPath; + mono.meta.id = "MultiPlugin_Mono"; + knownPlugins.push_back(mono); + + AudioPluginInfo stereo; + stereo.path = sharedPath; + stereo.meta.id = "MultiPlugin_Stereo"; + knownPlugins.push_back(stereo); + + ON_CALL(*m_knownPlugins, pluginInfoList(_)) + .WillByDefault(Return(knownPlugins)); + + // [GIVEN] Scanner finds nothing — the plugin has been uninstalled + for (const IAudioPluginsScannerPtr& scanner : m_scanners) { + AudioPluginsScannerMock* mock = dynamic_cast(scanner.get()); + ASSERT_TRUE(mock); + + ON_CALL(*mock, scanPlugins(_)) + .WillByDefault(Return(paths_t {})); + } + + // [THEN] Both component IDs are unregistered + AudioResourceIdList expectedIds { mono.meta.id, stereo.meta.id }; + EXPECT_CALL(*m_knownPlugins, unregisterPlugins(expectedIds)) + .WillOnce(Return(make_ok())); + + EXPECT_CALL(*m_process, execute(_, _)) + .Times(0); + + EXPECT_CALL(*m_interactive, showProgress(_, _)) + .Times(0); + + EXPECT_CALL(*m_knownPlugins, load()) + .Times(0); + + // [WHEN] Update registry + m_scenario->updatePluginsRegistry(); } TEST_F(AudioPlugins_RegisterAudioPluginsScenarioTest, RegisterPlugin) diff --git a/framework/vst/internal/vstpluginsscanner.cpp b/framework/vst/internal/vstpluginsscanner.cpp index c5cdc67b1f..5fc2c059ce 100644 --- a/framework/vst/internal/vstpluginsscanner.cpp +++ b/framework/vst/internal/vstpluginsscanner.cpp @@ -34,15 +34,14 @@ using namespace muse::vst; * @see https://developer.steinberg.help/pages/viewpage.action?pageId=9798275 **/ namespace muse::vst { -static io::paths_t pluginPathsFromDefaultLocation() +static std::set pluginPathsFromDefaultLocation() { - io::paths_t result; + std::set result; try { PluginModule::PathList paths = PluginModule::getModulePaths(); - for (const std::string& path : paths) { - result.push_back(path); + result.insert(path); } } catch (...) { LOGE() << "Unable to get module paths"; @@ -56,16 +55,16 @@ io::paths_t VstPluginsScanner::scanPlugins(Progress*) const { TRACEFUNC; - io::paths_t result = pluginPathsFromDefaultLocation(); - io::paths_t plugins = pluginPathsFromCustomLocations(configuration()->userVstDirectories()); - result.insert(result.end(), std::make_move_iterator(plugins.begin()), std::make_move_iterator(plugins.end())); + std::set unique = pluginPathsFromDefaultLocation(); + std::set custom = pluginPathsFromCustomLocations(configuration()->userVstDirectories()); + unique.insert(custom.begin(), custom.end()); - return result; + return { unique.begin(), unique.end() }; } -io::paths_t VstPluginsScanner::pluginPathsFromCustomLocations(const io::paths_t& customPaths) const +std::set VstPluginsScanner::pluginPathsFromCustomLocations(const io::paths_t& customPaths) const { - io::paths_t result; + std::set result; for (const io::path_t& path : customPaths) { RetVal paths = fileSystem()->scanFiles(path, { VST3_PACKAGE_FILTER }); @@ -74,7 +73,7 @@ io::paths_t VstPluginsScanner::pluginPathsFromCustomLocations(const io::paths_t& continue; } - result.insert(result.end(), std::make_move_iterator(paths.val.begin()), std::make_move_iterator(paths.val.end())); + result.insert(paths.val.begin(), paths.val.end()); } return result; diff --git a/framework/vst/internal/vstpluginsscanner.h b/framework/vst/internal/vstpluginsscanner.h index 1611e3f85a..1e2af547d2 100644 --- a/framework/vst/internal/vstpluginsscanner.h +++ b/framework/vst/internal/vstpluginsscanner.h @@ -19,8 +19,8 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see . */ -#ifndef MUSE_VST_VSTPLUGINSSCANNER_H -#define MUSE_VST_VSTPLUGINSSCANNER_H + +#pragma once #include "audioplugins/iaudiopluginsscanner.h" @@ -28,6 +28,8 @@ #include "ivstconfiguration.h" #include "io/ifilesystem.h" +#include + namespace muse::vst { class VstPluginsScanner : public audioplugins::IAudioPluginsScanner { @@ -38,8 +40,6 @@ class VstPluginsScanner : public audioplugins::IAudioPluginsScanner io::paths_t scanPlugins(Progress* progress = nullptr) const override; private: - io::paths_t pluginPathsFromCustomLocations(const io::paths_t& customPaths) const; + std::set pluginPathsFromCustomLocations(const io::paths_t& customPaths) const; }; } - -#endif // MUSE_VST_VSTPLUGINSSCANNER_H