From f0333598bb042025103decfb10cf24aa3ef9f6f4 Mon Sep 17 00:00:00 2001
From: Jan Holthuis <jan.holthuis@ruhr-uni-bochum.de>
Date: Wed, 25 Jan 2023 12:05:30 +0100
Subject: [PATCH] fix(serato): Fix resetting track colors on metadata reimport

When metadata for a file without Serato tags is reimported, the track
color is reset. This happens, because there is currently no way to
distinguish the case where no track color is set because the file does
not contain any Serato tags from the case where the file contains Serato
tags and the track color has been set to "no color" in Serato (i.e. the
stored color value is `0xFFFFFF`.

This fixes the issue by replacing the `RgbColor::optional_t` value with
a `std::optional<RgbColor::optional_t>` to separate the 3 cases:

1. If the outer optional is `nullopt` , there is no track color present in
  the tags.
2. If the inner optional is `nullopt`, there is a track color present in
  the tags, and that color is "no color".
3. If the inner option holds a value, there is a track color with that
   RGB color value present in the tags.

Fixes #11213.
---
 src/test/seratotagstest.cpp |  8 ++++++--
 src/track/serato/tags.cpp   |  8 ++++----
 src/track/serato/tags.h     |  4 +++-
 src/track/track.cpp         | 13 +++++++++----
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/test/seratotagstest.cpp b/src/test/seratotagstest.cpp
index f9356ff1e3c..a932f8d020c 100644
--- 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/tags.cpp b/src/track/serato/tags.cpp
index abc96951964..5e15bb6fb19 100644
--- a/src/track/serato/tags.cpp
+++ b/src/track/serato/tags.cpp
@@ -427,7 +427,7 @@ 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) {
@@ -435,11 +435,11 @@ RgbColor::optional_t SeratoTags::getTrackColor() const {
         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
index 5aa480d7661..378ea38dc32 100644
--- 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,7 @@ class SeratoTags final {
         m_seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffset);
     }
 
-    RgbColor::optional_t getTrackColor() const;
+    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
index f4acaaa54ff..fdd07c6edf5 100644
--- 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);
         }
     }