From f0333598bb042025103decfb10cf24aa3ef9f6f4 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Wed, 25 Jan 2023 12:05:30 +0100 Subject: [PATCH 1/2] 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` 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& cueInfos, double timingOffset m_seratoMarkers2.setCues(cueInfoList); } -RgbColor::optional_t SeratoTags::getTrackColor() const { +std::optional 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{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 + #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 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 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); } } From 50c54ce6cc18eea51ea758e5b11d5ab3c5381a6a Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Fri, 27 Jan 2023 15:15:14 +0100 Subject: [PATCH 2/2] docs(serato): Explain Serato track color return values in comments --- src/track/serato/markers.h | 5 +++++ src/track/serato/markers2.h | 6 ++++++ src/track/serato/tags.h | 11 +++++++++++ 3 files changed, 22 insertions(+) diff --git a/src/track/serato/markers.h b/src/track/serato/markers.h index 370cbd48c30..45018025463 100644 --- 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 index f38e3ca59c4..d041924a58b 100644 --- a/src/track/serato/markers2.h +++ b/src/track/serato/markers2.h @@ -430,6 +430,12 @@ class SeratoMarkers2 final { QList getCues() const; void setCues(const QList& 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.h b/src/track/serato/tags.h index 378ea38dc32..8d41380f208 100644 --- a/src/track/serato/tags.h +++ b/src/track/serato/tags.h @@ -101,6 +101,17 @@ class SeratoTags final { m_seratoBeatGrid.setBeats(pBeats, signalInfo, duration, timingOffset); } + /// 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 getTrackColor() const; void setTrackColor(RgbColor::optional_t color);