Only re-import Serato tags if Serato metadata export is enabled

uklotzde · Sep 15, 2021 · 278ec4f · 278ec4f
1 parent d0f45fc
commit 278ec4f
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 7 deletions.
diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp
@@ -853,6 +853,7 @@ TrackPointer TrackDAO::addTracksAddFile(
     // Initially (re-)import the metadata for the newly created track
     // from the file.
     SoundSourceProxy(pTrack).updateTrackFromSource(
+            m_pConfig,
             SoundSourceProxy::UpdateTrackFromSourceMode::Once);
     if (!pTrack->checkSourceSynchronized()) {
         qWarning() << "TrackDAO::addTracksAddFile:"
@@ -1504,7 +1505,9 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const {
             updateTrackFromSourceMode =
                     SoundSourceProxy::UpdateTrackFromSourceMode::Newer;
         }
-        SoundSourceProxy(pTrack).updateTrackFromSource(updateTrackFromSourceMode);
+        SoundSourceProxy(pTrack).updateTrackFromSource(
+                m_pConfig,
+                updateTrackFromSourceMode);
         if (kLogger.debugEnabled() && pTrack->isDirty()) {
             kLogger.debug()
                     << "Updated track metadata from file tags:"

diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp
@@ -3,6 +3,7 @@
 #include <QApplication>
 #include <QStandardPaths>
 
+#include "library/library_prefs.h"
 #include "sources/audiosourcetrackproxy.h"
 
 #ifdef __MAD__
@@ -518,7 +519,7 @@ SoundSourceProxy::importTrackMetadataAndCoverImage(
 
 namespace {
 
-inline bool shouldUpdateTrackMetadatafromSource(
+inline bool shouldUpdateTrackMetadataFromSource(
         SoundSourceProxy::UpdateTrackFromSourceMode mode,
         mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) {
     return mode == SoundSourceProxy::UpdateTrackFromSourceMode::Always ||
@@ -528,9 +529,23 @@ inline bool shouldUpdateTrackMetadatafromSource(
                     sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void);
 }
 
+inline bool shouldImportSeratoTagsFromSource(
+        const UserSettingsPointer& pConfig,
+        mixxx::TrackRecord::SourceSyncStatus sourceSyncStatus) {
+    // 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
+    // modified in Mixxx.
+    // A reimport of metadata happens if
+    // sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Outdated
+    return sourceSyncStatus != mixxx::TrackRecord::SourceSyncStatus::Outdated ||
+            pConfig->getValue<bool>(mixxx::library::prefs::kSeratoMetadataExportConfigKey);
+}
+
 } // namespace
 
 bool SoundSourceProxy::updateTrackFromSource(
+        const UserSettingsPointer& pConfig,
         UpdateTrackFromSourceMode mode) {
     DEBUG_ASSERT(m_pTrack);
 
@@ -569,7 +584,7 @@ bool SoundSourceProxy::updateTrackFromSource(
     QImage* pCoverImg = nullptr; // pointer also serves as a flag
 
     const bool updateMetadataFromSource =
-            shouldUpdateTrackMetadatafromSource(mode, sourceSyncStatus);
+            shouldUpdateTrackMetadataFromSource(mode, sourceSyncStatus);
 
     // Decide if cover art needs to be re-imported
     if (updateMetadataFromSource) {
@@ -631,6 +646,12 @@ bool SoundSourceProxy::updateTrackFromSource(
 
     // Full import
     DEBUG_ASSERT(updateMetadataFromSource);
+    if (!shouldImportSeratoTagsFromSource(
+                pConfig,
+                sourceSyncStatus)) {
+        // Reset Serato tags to disable the (re-)import
+        trackMetadata.refTrackInfo().refSeratoTags() = {};
+    }
     if (sourceSyncStatus == mixxx::TrackRecord::SourceSyncStatus::Void) {
         DEBUG_ASSERT(pCoverImg);
         if (kLogger.debugEnabled()) {

diff --git a/src/sources/soundsourceproxy.h b/src/sources/soundsourceproxy.h
@@ -149,6 +149,7 @@ class SoundSourceProxy {
     ///
     /// Returns true if the track has been modified and false otherwise.
     bool updateTrackFromSource(
+            const UserSettingsPointer& pConfig,
             UpdateTrackFromSourceMode mode);
 
     /// Opening the audio source through the proxy will update the

diff --git a/src/test/autodjprocessor_test.cpp b/src/test/autodjprocessor_test.cpp
@@ -167,10 +167,12 @@ class AutoDJProcessorTest : public LibraryTest {
     static TrackId nextTrackId(TrackId trackId) {
         return TrackId(trackId.value() + 1);
     }
-    static TrackPointer newTestTrack(TrackId trackId) {
+
+    TrackPointer newTestTrack(TrackId trackId) {
         TrackPointer pTrack(
                 Track::newDummy(kTrackLocationTest, trackId));
         EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+                config(),
                 SoundSourceProxy::UpdateTrackFromSourceMode::Once));
         return pTrack;
     }

diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp
@@ -115,6 +115,7 @@ TEST_F(CoverArtUtilTest, searchImage) {
     // Looking for a track with embedded cover.
     pTrack = Track::newTemporary(kTrackLocationTest);
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
     CoverInfo result = pTrack->getCoverInfoWithLocation();
     EXPECT_EQ(result.type, CoverInfo::METADATA);

diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp
@@ -214,6 +214,7 @@ TEST_F(SoundSourceProxyTest, readArtist) {
     auto pTrack = Track::newTemporary(kTestDir, "artist.mp3");
     SoundSourceProxy proxy(pTrack);
     EXPECT_TRUE(proxy.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
     EXPECT_EQ("Test Artist", pTrack->getArtist());
 }
@@ -226,12 +227,14 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "empty.mp3");
     SoundSourceProxy proxy1(pTrack1);
     EXPECT_TRUE(proxy1.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
     EXPECT_EQ("empty", pTrack1->getTitle());
 
     // Test a reload also works
     pTrack1->setTitle("");
     EXPECT_TRUE(proxy1.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Always));
     EXPECT_EQ("empty", pTrack1->getTitle());
 
@@ -240,12 +243,14 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "cover-test-png.mp3");
     SoundSourceProxy proxy2(pTrack2);
     EXPECT_TRUE(proxy2.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
     EXPECT_EQ("cover-test-png", pTrack2->getTitle());
 
     // Test a reload also works
     pTrack2->setTitle("");
     EXPECT_TRUE(proxy2.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Always));
     EXPECT_EQ("cover-test-png", pTrack2->getTitle());
 
@@ -254,6 +259,7 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
             kTestDir, "cover-test-jpg.mp3");
     SoundSourceProxy proxy3(pTrack3);
     EXPECT_TRUE(proxy3.updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
     EXPECT_EQ("test22kMono", pTrack3->getTitle());
 }

diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp
@@ -27,9 +27,10 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration {
         return Track::newTemporary(kTestDir, "TOAL_TPE2.mp3");
     }
 
-    static TrackPointer newTestTrackParsed() {
+    TrackPointer newTestTrackParsed() {
         auto pTrack = newTestTrack();
         EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+                config(),
                 SoundSourceProxy::UpdateTrackFromSourceMode::Once));
         EXPECT_TRUE(pTrack->checkSourceSynchronized());
         EXPECT_TRUE(hasTrackMetadata(pTrack));
@@ -39,7 +40,7 @@ class TrackUpdateTest : public MixxxTest, SoundSourceProviderRegistration {
         return pTrack;
     }
 
-    static TrackPointer newTestTrackParsedModified() {
+    TrackPointer newTestTrackParsedModified() {
         auto pTrack = newTestTrackParsed();
         pTrack->setArtist(pTrack->getArtist() + pTrack->getArtist());
         auto coverInfo = pTrack->getCoverInfo();
@@ -61,6 +62,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) {
 
     // Re-update from source should have no effect
     ASSERT_FALSE(SoundSourceProxy(pTrack).updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Once));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
@@ -81,6 +83,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Always));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
@@ -105,6 +108,7 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Always));
 
     const auto trackMetadataAfter = pTrack->getMetadata();
@@ -124,6 +128,7 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) {
     const auto coverInfoBefore = pTrack->getCoverInfo();
 
     EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
+            config(),
             SoundSourceProxy::UpdateTrackFromSourceMode::Always));
 
     const auto trackMetadataAfter = pTrack->getMetadata();

diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp
@@ -902,15 +902,24 @@ void WTrackMenu::slotOpenInFileBrowser() {
 namespace {
 
 class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPointerOperation {
+  public:
+    explicit ImportMetadataFromFileTagsTrackPointerOperation(
+            UserSettingsPointer pConfig)
+            : m_pConfig(std::move(pConfig)) {
+    }
+
   private:
     void doApply(
             const TrackPointer& pTrack) const override {
         // The user has explicitly requested to reload metadata from the file
         // to override the information within Mixxx! Custom cover art must be
         // reloaded separately.
         SoundSourceProxy(pTrack).updateTrackFromSource(
+                m_pConfig,
                 SoundSourceProxy::UpdateTrackFromSourceMode::Always);
     }
+
+    const UserSettingsPointer m_pConfig;
 };
 
 } // anonymous namespace
@@ -935,7 +944,7 @@ void WTrackMenu::slotImportMetadataFromFileTags() {
     const auto progressLabelText =
             tr("Importing metadata of %n track(s) from file tags", "", getTrackCount());
     const auto trackOperator =
-            ImportMetadataFromFileTagsTrackPointerOperation();
+            ImportMetadataFromFileTagsTrackPointerOperation(m_pConfig);
     applyTrackPointerOperation(
             progressLabelText,
             &trackOperator,