Merge pull request #4605 from uklotzde/export-track-metadata-params

Sync track metadata: Pass parameters explicitly

12 files changed, 75 insertions, 58 deletions

diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp
index 98c532f507..8cba6a9b8c 100644
--- a/src/library/dao/trackdao.cpp
+++ b/src/library/dao/trackdao.cpp
@@ -866,8 +866,8 @@ TrackPointer TrackDAO::addTracksAddFile(
     // Initially (re-)import the metadata for the newly created track
     // from the file.
     SoundSourceProxy(pTrack).updateTrackFromSource(
-            m_pConfig,
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once);
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams::readFromUserSettings(*m_pConfig));
     if (!pTrack->checkSourceSynchronized()) {
         qWarning() << "TrackDAO::addTracksAddFile:"
                 << "Failed to parse track metadata from file"
@@ -1524,8 +1524,8 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
                     SoundSourceProxy::UpdateTrackFromSourceMode::Newer;
         }
         SoundSourceProxy(pTrack).updateTrackFromSource(
-                m_pConfig,
-                updateTrackFromSourceMode);
+                updateTrackFromSourceMode,
+                SyncTrackMetadataParams::readFromUserSettings(*m_pConfig));
         if (kLogger.debugEnabled() && pTrack->isDirty()) {
             kLogger.debug()
                     << "Updated track metadata from file tags:"
diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp
index 645323918d..23372368e4 100644
--- a/src/library/trackcollectionmanager.cpp
+++ b/src/library/trackcollectionmanager.cpp
@@ -307,7 +307,9 @@ void TrackCollectionManager::exportTrackMetadata(
         switch (mode) {
         case TrackMetadataExportMode::Immediate:
             // Export track metadata now by saving as file tags.
-            SoundSourceProxy::exportTrackMetadataBeforeSaving(pTrack, m_pConfig);
+            SoundSourceProxy::exportTrackMetadataBeforeSaving(
+                    pTrack,
+                    SyncTrackMetadataParams::readFromUserSettings(*m_pConfig));
             break;
         case TrackMetadataExportMode::Deferred:
             // Export track metadata later when the track object goes out
diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp
index a0d8ac6d62..9276346c75 100644
--- a/src/sources/soundsourceproxy.cpp
+++ b/src/sources/soundsourceproxy.cpp
@@ -4,7 +4,6 @@
 #include <QRegularExpression>
 #include <QStandardPaths>
 
-#include "library/library_prefs.h"
 #include "sources/audiosourcetrackproxy.h"
 
 #ifdef __MAD__
@@ -308,7 +307,7 @@ SoundSourceProxy::allProviderRegistrationsForUrl(
 ExportTrackMetadataResult
 SoundSourceProxy::exportTrackMetadataBeforeSaving(
         Track* pTrack,
-        const UserSettingsPointer& pConfig) {
+        const SyncTrackMetadataParams& syncParams) {
     DEBUG_ASSERT(pTrack);
     const auto fileInfo = pTrack->getFileInfo();
     mixxx::SoundSourcePointer pSoundSource;
@@ -342,7 +341,7 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(
                 << fileInfo;
         return ExportTrackMetadataResult::Skipped;
     }
-    return pTrack->exportMetadata(*pSoundSource, pConfig);
+    return pTrack->exportMetadata(*pSoundSource, syncParams);
 }
 
 // Used during tests only
@@ -524,8 +523,8 @@ SoundSourceProxy::importTrackMetadataAndCoverImage(
 namespace {
 
 inline bool shouldUpdateTrackMetadataFromSource(
-        SoundSourceProxy::UpdateTrackFromSourceMode mode,
-        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) {
+        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus,
+        SoundSourceProxy::UpdateTrackFromSourceMode mode) {
     return mode == SoundSourceProxy::UpdateTrackFromSourceMode::Always ||
             (mode == SoundSourceProxy::UpdateTrackFromSourceMode::Newer &&
                     sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated) ||
@@ -534,8 +533,8 @@ inline bool shouldUpdateTrackMetadataFromSource(
 }
 
 inline bool shouldImportSeratoTagsFromSource(
-        const UserSettingsPointer& pConfig,
-        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) {
+        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus,
+        const SyncTrackMetadataParams& syncParams) {
     // Only reimport track metadata from Serato markers if export of
     // Serato markers is enabled. This should ensure that track color,
     // cue points, and loops do not get lost after they have been
@@ -543,14 +542,14 @@ inline bool shouldImportSeratoTagsFromSource(
     // A reimport of metadata happens if
     // sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated
     return sourceSyncStatus != mixxx::TrackRecord::SourceSyncStatus::Outdated ||
-            pConfig->getValue<bool>(mixxx::library::prefs::kSyncSeratoMetadataConfigKey);
+            syncParams.syncSeratoMetadata;
 }
 
 } // namespace
 
 bool SoundSourceProxy::updateTrackFromSource(
-        const UserSettingsPointer& pConfig,
-        UpdateTrackFromSourceMode mode) {
+        UpdateTrackFromSourceMode mode,
+        const SyncTrackMetadataParams& syncParams) {
     DEBUG_ASSERT(m_pTrack);
 
     if (getUrl().isEmpty()) {
@@ -588,7 +587,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     QImage* pCoverImg = nullptr; // pointer also serves as a flag
 
     const bool updateMetadataFromSource =
-            shouldUpdateTrackMetadataFromSource(mode, sourceSyncStatus);
+            shouldUpdateTrackMetadataFromSource(sourceSyncStatus, mode);
 
     // Decide if cover art needs to be re-imported
     if (updateMetadataFromSource) {
@@ -650,9 +649,7 @@ bool SoundSourceProxy::updateTrackFromSource(
 
     // Full import
     DEBUG_ASSERT(updateMetadataFromSource);
-    if (!shouldImportSeratoTagsFromSource(
-                pConfig,
-                sourceSyncStatus)) {
+    if (!shouldImportSeratoTagsFromSource(sourceSyncStatus, syncParams)) {
         // Reset Serato tags to disable the (re-)import
         trackMetadata.refTrackInfo().refSeratoTags() = {};
     }
diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h
index 54ab9b608d..14c09b580d 100644
--- a/src/sources/soundsourceproxy.h
+++ b/src/sources/soundsourceproxy.h
@@ -1,6 +1,5 @@
 #pragma once
 
-#include "preferences/usersettings.h"
 #include "sources/soundsourceproviderregistry.h"
 #include "track/track_decl.h"
 #include "util/sandbox.h"
@@ -152,8 +151,8 @@ class SoundSourceProxy {
     ///
     /// Returns true if the track has been modified and false otherwise.
     bool updateTrackFromSource(
-            const UserSettingsPointer& pConfig,
-            UpdateTrackFromSourceMode mode);
+            UpdateTrackFromSourceMode mode,
+            const SyncTrackMetadataParams& syncParams);
 
     /// Opening the audio source through the proxy will update the
     /// audio properties of the corresponding track object. Returns
@@ -180,7 +179,7 @@ class SoundSourceProxy {
     friend class TrackCollectionManager;
     static ExportTrackMetadataResult exportTrackMetadataBeforeSaving(
             Track* pTrack,
-            const UserSettingsPointer& pConfig);
+            const SyncTrackMetadataParams& syncParams);
 
     // Special case: Construction from a url is needed
     // for writing metadata immediately before the TIO is destroyed.
diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp
index 9ec3ae33d0..2a2299ab28 100644
--- a/src/test/autodjprocessor_test.cpp
+++ b/src/test/autodjprocessor_test.cpp
@@ -172,8 +172,8 @@ class AutoDJProcessorTest : public LibraryTest {
         TrackPointer pTrack(
                 Track::newDummy(kTrackLocationTest, trackId));
         EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-                config(),
-                SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+                SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+                SyncTrackMetadataParams{}));
         return pTrack;
     }
 
diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp
index 0bec9234b9..bfe0217c91 100644
--- a/src/test/coverartutils_test.cpp
+++ b/src/test/coverartutils_test.cpp
@@ -115,8 +115,8 @@ TEST_F(CoverArtUtilTest, searchImage) {
     // Looking for a track with embedded cover.
     pTrack = Track::newTemporary(kTrackLocationTest);
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
     CoverInfo result = pTrack->getCoverInfoWithLocation();
     EXPECT_EQ(result.type, CoverInfo::METADATA);
     EXPECT_EQ(result.source, CoverInfo::GUESSED);
diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp
index fe75fec0e8..ce1be68cde 100644
--- a/src/test/soundproxy_test.cpp
+++ b/src/test/soundproxy_test.cpp
@@ -222,8 +222,8 @@ TEST_F(SoundSourceProxyTest, readArtist) {
     auto pTrack = Track::newTemporary(kTestDir, "artist.mp3");
     SoundSourceProxy proxy(pTrack);
     EXPECT_TRUE(proxy.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("Test Artist", pTrack->getArtist());
 }
 
@@ -235,15 +235,15 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "empty.mp3");
     SoundSourceProxy proxy1(pTrack1);
     EXPECT_TRUE(proxy1.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("empty", pTrack1->getTitle());
 
     // Test a reload also works
     pTrack1->setTitle("");
     EXPECT_TRUE(proxy1.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Always));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("empty", pTrack1->getTitle());
 
     // Test a file with other metadata but no title
@@ -251,15 +251,15 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "cover-test-png.mp3");
     SoundSourceProxy proxy2(pTrack2);
     EXPECT_TRUE(proxy2.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("cover-test-png", pTrack2->getTitle());
 
     // Test a reload also works
     pTrack2->setTitle("");
     EXPECT_TRUE(proxy2.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Always));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("cover-test-png", pTrack2->getTitle());
 
     // Test a file with a title
@@ -267,8 +267,8 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "cover-test-jpg.mp3");
     SoundSourceProxy proxy3(pTrack3);
     EXPECT_TRUE(proxy3.updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
     EXPECT_EQ("test22kMono", pTrack3->getTitle());
 }
 
diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp
index 4cd0a44e30..7a639edd69 100644
--- a/src/test/trackupdate_test.cpp
+++ b/src/test/trackupdate_test.cpp
@@ -30,8 +30,8 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration {
     TrackPointer newTestTrackParsed() const {
         auto pTrack = newTestTrack();
         EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-                config(),
-                SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+                SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+                SyncTrackMetadataParams{}));
         EXPECT_TRUE(pTrack->checkSourceSynchronized());
         EXPECT_TRUE(hasTrackMetadata(pTrack));
         EXPECT_TRUE(hasCoverArt(pTrack));
@@ -62,8 +62,8 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) {
 
     // Re-update from source should have no effect
     ASSERT_FALSE(SoundSourceProxy(pTrack).updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Once));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Once,
+            SyncTrackMetadataParams{}));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
     const auto coverInfoAfter = pTrack->getCoverInfo();
@@ -83,8 +83,8 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Always));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+            SyncTrackMetadataParams{}));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
     const auto coverInfoAfter = pTrack->getCoverInfo();
@@ -108,8 +108,8 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Always));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+            SyncTrackMetadataParams{}));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
     const auto coverInfoAfter = pTrack->getCoverInfo();
@@ -128,8 +128,8 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
-            config(),
-            SoundSourceProxy::UpdateTrackFromSourceMode::Always));
+            SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+            SyncTrackMetadataParams{}));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
     const auto coverInfoAfter = pTrack->getCoverInfo();
diff --git a/src/track/track.cpp b/src/track/track.cpp
index 54043b3298..eb45b6fb54 100644
--- a/src/track/track.cpp
+++ b/src/track/track.cpp
@@ -67,6 +67,16 @@ inline mixxx::Bpm getBeatsPointerBpm(
 //static
 const QString Track::kArtistTitleSeparator = QStringLiteral(" - ");
 
+//static
+SyncTrackMetadataParams SyncTrackMetadataParams::readFromUserSettings(
+        const UserSettings& userSettings) {
+    const auto syncSeratoMetadata =
+            userSettings.getValue<bool>(mixxx::library::prefs::kSyncSeratoMetadataConfigKey);
+    return SyncTrackMetadataParams{
+            syncSeratoMetadata,
+    };
+}
+
 Track::Track(
         mixxx::FileAccess fileAccess,
         TrackId trackId)
@@ -1446,7 +1456,7 @@ CoverInfo Track::getCoverInfoWithLocation() const {
 
 ExportTrackMetadataResult Track::exportMetadata(
         const mixxx::MetadataSource& metadataSource,
-        const UserSettingsPointer& pConfig) {
+        const SyncTrackMetadataParams& syncParams) {
     // Locking shouldn't be necessary here, because this function will
     // be called after all references to the object have been dropped.
     // But it doesn't hurt much, so let's play it safe ;)
@@ -1470,7 +1480,7 @@ ExportTrackMetadataResult Track::exportMetadata(
         return ExportTrackMetadataResult::Skipped;
     }
 
-    if (pConfig->getValue<bool>(mixxx::library::prefs::kSyncSeratoMetadataConfigKey)) {
+    if (syncParams.syncSeratoMetadata) {
         const auto streamInfo = m_record.getStreamInfoFromSource();
         VERIFY_OR_DEBUG_ASSERT(streamInfo &&
                 streamInfo->getSignalInfo().isValid() &&
diff --git a/src/track/track.h b/src/track/track.h
index e2c3b2e2f5..d12d2b03b1 100644
--- a/src/track/track.h
+++ b/src/track/track.h
@@ -534,7 +534,7 @@ class Track : public QObject {
 
     ExportTrackMetadataResult exportMetadata(
             const mixxx::MetadataSource& metadataSource,
-            const UserSettingsPointer& pConfig);
+            const SyncTrackMetadataParams& syncParams);
 
     // Information about the actual properties of the
     // audio stream is only available after opening the
diff --git a/src/track/track_decl.h b/src/track/track_decl.h
index 12a03187cc..f81aab24cc 100644
--- a/src/track/track_decl.h
+++ b/src/track/track_decl.h
@@ -6,12 +6,21 @@
 #include <QMetaType>
 #include <memory>
 
+#include "preferences/usersettings.h"
+
 class Track;
 
 typedef std::shared_ptr<Track> TrackPointer;
 typedef std::weak_ptr<Track> TrackWeakPointer;
 typedef QList<TrackPointer> TrackPointerList;
 
+struct SyncTrackMetadataParams {
+    bool syncSeratoMetadata = false;
+
+    static SyncTrackMetadataParams readFromUserSettings(
+            const UserSettings& userSettings);
+};
+
 enum class ExportTrackMetadataResult {
     Succeeded,
     Failed,
diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp
index 9baacfb36a..f3ecc1766b 100644
--- a/src/widget/wtrackmenu.cpp
+++ b/src/widget/wtrackmenu.cpp
@@ -953,8 +953,8 @@ namespace {
 class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPointerOperation {
   public:
     explicit ImportMetadataFromFileTagsTrackPointerOperation(
-            UserSettingsPointer pConfig)
-            : m_pConfig(std::move(pConfig)) {
+            const UserSettings& userSettings)
+            : m_params(SyncTrackMetadataParams::readFromUserSettings(userSettings)) {
     }
 
   private:
@@ -964,11 +964,11 @@ class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPoint
         // to override the information within Mixxx! Custom cover art must be
         // reloaded separately.
         SoundSourceProxy(pTrack).updateTrackFromSource(
-                m_pConfig,
-                SoundSourceProxy::UpdateTrackFromSourceMode::Always);
+                SoundSourceProxy::UpdateTrackFromSourceMode::Always,
+                m_params);
     }
 
-    const UserSettingsPointer m_pConfig;
+    const SyncTrackMetadataParams m_params;
 };
 
 } // anonymous namespace
@@ -993,7 +993,7 @@ void WTrackMenu::slotImportMetadataFromFileTags() {
     const auto progressLabelText =
             tr("Importing metadata of %n track(s) from file tags", "", getTrackCount());
     const auto trackOperator =
-            ImportMetadataFromFileTagsTrackPointerOperation(m_pConfig);
+            ImportMetadataFromFileTagsTrackPointerOperation(*m_pConfig);
     applyTrackPointerOperation(
             progressLabelText,
             &trackOperator,