Merge pull request #11217 from Holzhaus/fix-serato-track-color-reset-…

…on-import fix(serato): Fix resetting track colors on metadata reimport
mixxxdj · Jan 28, 2023 · d02fdc0 · d02fdc0
2 parents 50b780d + 50c54ce
commit d02fdc0
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 11 deletions.
diff --git a/src/test/seratotagstest.cpp b/src/test/seratotagstest.cpp
@@ -217,7 +217,9 @@ TEST_F(SeratoTagsTest, MarkersParseDumpRoundtrip) {
 
         const auto trackColor = seratoTags.getTrackColor();
         const auto cueInfos = seratoTags.getCueInfos();
-        seratoTags.setTrackColor(trackColor);
+        if (trackColor) {
+            seratoTags.setTrackColor(*trackColor);
+        }
         seratoTags.setCueInfos(cueInfos);
 
         const QByteArray outputData = seratoTags.dumpMarkers(filetype);
@@ -252,7 +254,9 @@ TEST_F(SeratoTagsTest, Markers2RoundTrip) {
         const auto trackColor = seratoTags.getTrackColor();
         const auto cueInfos = seratoTags.getCueInfos();
         seratoTags.setBpmLocked(bpmLocked);
-        seratoTags.setTrackColor(trackColor);
+        if (trackColor) {
+            seratoTags.setTrackColor(*trackColor);
+        }
         seratoTags.setCueInfos(cueInfos);
 
         const QByteArray outputData = seratoTags.dumpMarkers2(filetype);

diff --git a/src/track/serato/markers.h b/src/track/serato/markers.h
@@ -162,6 +162,11 @@ class SeratoMarkers final {
         m_entries = entries;
     }
 
+    /// Always returns a color if the tag is present (i.e. `isEmpty()` is
+    /// false).
+    ///
+    /// Note that the color returned by this function needs to be converted
+    /// into a display color using `SeratoTags::storedToDisplayedTrackColor()`.
     RgbColor::optional_t getTrackColor() const {
         return m_trackColor;
     }

diff --git a/src/track/serato/markers2.h b/src/track/serato/markers2.h
@@ -430,6 +430,12 @@ class SeratoMarkers2 final {
     QList<CueInfo> getCues() const;
     void setCues(const QList<CueInfo>& cueInfos);
 
+    /// Returns a color if the tag is present and contains a `COLOR` entry.
+    /// Usually, such an entry should always exist, even if the track has no
+    /// color assigned to it in Serato (in that case the color is `0xFFFFFF`).
+    ///
+    /// Note that the color returned by this function needs to be converted
+    /// into a display color using `SeratoTags::storedToDisplayedTrackColor()`.
     RgbColor::optional_t getTrackColor() const;
     void setTrackColor(RgbColor color);
 

diff --git a/src/track/serato/tags.cpp b/src/track/serato/tags.cpp
@@ -427,19 +427,19 @@ void SeratoTags::setCueInfos(const QList<CueInfo>& cueInfos, double timingOffset
     m_seratoMarkers2.setCues(cueInfoList);
 }
 
-RgbColor::optional_t SeratoTags::getTrackColor() const {
+std::optional<RgbColor::optional_t> SeratoTags::getTrackColor() const {
     RgbColor::optional_t color = m_seratoMarkers.getTrackColor();
 
     if (!color) {
         // Markers_ is empty, but we may have a color in Markers2
         color = m_seratoMarkers2.getTrackColor();
     }
 
-    if (color) {
-        color = SeratoTags::storedToDisplayedTrackColor(*color);
+    if (!color) {
+        return std::nullopt;
     }
 
-    return color;
+    return std::optional<RgbColor::optional_t>{SeratoTags::storedToDisplayedTrackColor(*color)};
 }
 
 void SeratoTags::setTrackColor(RgbColor::optional_t color) {

diff --git a/src/track/serato/tags.h b/src/track/serato/tags.h
@@ -1,5 +1,7 @@
 #pragma once
 
+#include <optional>
+
 #include "audio/signalinfo.h"
 #include "track/beatsimporter.h"
 #include "track/cueinfoimporter.h"
@@ -99,7 +101,18 @@ class SeratoTags final {
         m_seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffset);
     }
 
-    RgbColor::optional_t getTrackColor() const;
+    /// Return the track color.
+    ///
+    /// If no Serato tags are present in the file or if tags contain no
+    /// information about the track color (which should never happen when
+    /// writing the tags with Serato DJ software), then the outer optional will
+    /// be `nullopt`. This means that the track color should be left unchanged
+    /// on metadata reimport.
+    ///
+    /// In all other cases, the track color has been specified in the Serato
+    /// tags successfully, and the inner optional value can be used as new
+    /// track color when (re-)importing metadata.
+    std::optional<RgbColor::optional_t> getTrackColor() const;
     void setTrackColor(RgbColor::optional_t color);
 
     bool isBpmLocked() const;

diff --git a/src/track/track.cpp b/src/track/track.cpp
@@ -206,10 +206,14 @@ void Track::importMetadata(
         const auto newKey = m_record.getGlobalKey();
 
         // Import track color from Serato tags if available
-        const auto newColor = m_record.getMetadata().getTrackInfo().getSeratoTags().getTrackColor();
-        const bool colorModified = compareAndSet(m_record.ptrColor(), newColor);
+        const std::optional<mixxx::RgbColor::optional_t> newColor =
+                m_record.getMetadata()
+                        .getTrackInfo()
+                        .getSeratoTags()
+                        .getTrackColor();
+        const bool colorModified = newColor && compareAndSet(m_record.ptrColor(), *newColor);
         modified |= colorModified;
-        DEBUG_ASSERT(!colorModified || m_record.getColor() == newColor);
+        DEBUG_ASSERT(!colorModified || m_record.getColor() == *newColor);
 
         if (!modified) {
             // Unmodified, nothing todo
@@ -228,7 +232,8 @@ void Track::importMetadata(
             emit replayGainUpdated(newReplayGain);
         }
         if (colorModified) {
-            emit colorUpdated(newColor);
+            DEBUG_ASSERT(newColor);
+            emit colorUpdated(*newColor);
         }
     }