Skip to content

Commit

Permalink
fix(serato): Fix resetting track colors on metadata reimport
Browse files Browse the repository at this point in the history
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 mixxxdj#11213.
  • Loading branch information
Holzhaus committed Jan 25, 2023
1 parent 5a3e9b8 commit f033359
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 11 deletions.
8 changes: 6 additions & 2 deletions src/test/seratotagstest.cpp
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 4 additions & 4 deletions src/track/serato/tags.cpp
Expand Up @@ -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) {
Expand Down
4 changes: 3 additions & 1 deletion 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"
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 9 additions & 4 deletions src/track/track.cpp
Expand Up @@ -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
Expand All @@ -228,7 +232,8 @@ void Track::importMetadata(
emit replayGainUpdated(newReplayGain);
}
if (colorModified) {
emit colorUpdated(newColor);
DEBUG_ASSERT(newColor);
emit colorUpdated(*newColor);
}
}

Expand Down

0 comments on commit f033359

Please sign in to comment.