Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(serato): Fix resetting track colors on metadata reimport #11217

Merged
merged 2 commits into from
Jan 28, 2023
Merged

fix(serato): Fix resetting track colors on metadata reimport #11217

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f033359
fix(serato): Fix resetting track colors on metadata reimport
Holzhaus Jan 25, 2023
50c54ce
docs(serato): Explain Serato track color return values in comments
Holzhaus Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/test/seratotagstest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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) {
Copy link
Member

@daschuer daschuer Jan 25, 2023 •
edited by Swiftb0y
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code above happens a shift in the meaning of
RgbColor::optional_t == std::nullopt_t
In the rest of Mixxx it means "No color", here it means "marker empty" = "Don't change" but this PR introduces std::optional<std::nullopt_t> for this.

So we need to make std::optional<std::nullopt_t> the default in an empty SeratoMarkers object.

All this is quite hard to understand. Do you see a way to make the meaning explicit?
Like a wrapper isNoColor() and isValid().
At least we need to document both meanings as a comment in the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to make std::optional<std::nullopt_t> the default in an empty SeratoMarkers object.

I suppose you meant std::optional<mixxx::RgbColor::optional_t>? But that wouldn't make sense. The inner RGB value can never be nullopt, if a color is specified it's always an RGB value. If it's not specified, it's nullopt.

The duplicate meaning of nullopt only happens in the stored-to-displayed color conversion, where 0xFFFFFF is mapped to "no color".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue is that m_seratoMarkers.getTrackColor(); returns the type: mixxx::RgbColor::optional_t
Which can have either a color value or std::nullopt_t. In the rest of Mixxx the later means "delete color" but we need actually "keep current color" which is now defined as std::optional<mixxx::RgbColor::optional_t> color = std::nullopt_t in contrast to std::optional<mixxx::RgbColor::optional_t> color = mixxx::RgbColor::optional_t(std::nullopt_t);

Conclusion: m_seratoMarkers.getTrackColor();' must return returns the type: std::optionalmixxx::RgbColor::optional_t`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue is that m_seratoMarkers.getTrackColor(); returns the type: mixxx::RgbColor::optional_t Which can have either a color value or std::nullopt_t. In the rest of Mixxx the later means "delete color" but we need actually "keep current color"

No, it doesnt. It means "no color set". This is literally what the optional type means. If the intention was to always assign the meaning "delete color" to the empty optional value, we should have written the type in that way and defined a constant mixxx::RgbColor::kDeleteColor or something. But that wasn't the intention, it was to make it easier to define a value that is either an RGB color or no color.

And indeed, if the Markers2 tag specifies a track color, then the track color field in the object is never std::nullopt. The same goes for Markers (which always specifies a color if the tag exists btw). This is because the track color in the tag is always an RGB value, even for "no color" (= 0xFFFFFF).

Conclusion: m_seratoMarkers.getTrackColor();' must return returns the type: std::optionalmixxx::RgbColor::optional_t`

For the reasons I explained above it's pointless to nest optionals for these tags. The only thing that archieves is to allow invalid states that cannot be serialized to a tag value. You cannot use the track color from the Markers or Markers2 tag verbatim in mixxx anyway, because those are internal serato color values. We convert those colors into values suitable for use in mixxx by calling SeratoTags::storedToDisplayedTrackColor(RgbColor), and only then the color can be std::nullopt despite being present in the tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And indeed, if the Markers2 tag specifies a track color, then the track color field in the object is never std::nullopt.

This is correct for the data source. But my issue is the middle ground build in this function:

mixxx/src/track/serato/markers2.cpp

Line 799 in f2d670e

RgbColor::optional_t SeratoMarkers2::getTrackColor() const {

std::optional<mixxx::RgbColor::optional_t> color == std::nullopt has the meaning "delete color". This was true in the 2.3 branch and the cause of the error was IMHO returning the wrong type from "SeratoMarkers2::getTrackColor()".

This PR invents a new type for this case. So lt's use it consistently there as well. Consequently the SeratoTags::storedToDisplayedTrackColor() call needs also to be moved to use the Mixxx colour domain from the beginning.

Than we need some comments about this hard to understand nested optional solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And indeed, if the Markers2 tag specifies a track color, then the track color field in the object is never std::nullopt.

This is correct for the data source. But my issue is the middle ground build in this function:

mixxx/src/track/serato/markers2.cpp

Line 799 in f2d670e

RgbColor::optional_t SeratoMarkers2::getTrackColor() const {

std::optional<mixxx::RgbColor::optional_t> color == std::nullopt has the meaning "delete color". This was true in the 2.3 branch and the cause of the error was IMHO returning the wrong type from "SeratoMarkers2::getTrackColor()".

No, it never meant that and repeating this statement does not make it true.
It simply means "no value" and nothing else.

You can also see it when looking at the Track::getColor function:

mixxx/src/track/track.h

Line 183 in 0f63426

mixxx::RgbColor::optional_t getColor() const;

When the function returns nullopt, this does not mean "delete color" (what would that even imply? what should be "deleted"?), it simply means that this track has no color set.

And in the same manner, Markers2::getTrackColor returns nullopt when the corresponding tag has no track color set. By the way, this should never happen when the tag has been created by Serato DJ. If the color was set to "no color" in Serato, then Markers2::getTrackColor will return 0xFFFFFF.

Than we need some comments about this hard to understand nested optional solution.

I added comments, although the nested optional is not really hard to understand IMHO. The outer optional tells us if we have information about the track color, inner optional is the track color itself (which may be "no color").

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Track::getColor() confirms my view. In Mixxx, the track has either a color or not = deleted. When you delete the color in Mixxx it becomes nullopt.

The same happens In Seato. The color is either set or transparent. Transparent becomes nullopt when converting it.

In the code here we have another state where we have no info at all.

The the source of the bug was that SeratoMarker:::getTrackColor(); returns RgbColor::optional_t, the Mixxxx color type without a conversion and with an ambiguous meaning for nullopt.

This PR fixes the bug, but keeps the root cause.

The same type and the equal named variable is used as

  • Deleted = 0xffffffff or nullopt
  • Not available = nullopt or wrapped with nullopt.

A clean solution would be to use the type consistency. This way the description of the SeratoMarkers.getTrackColor()
And
SeratoTags::getTrackColor()
Would be the same.

Alternative we need a new type for sera to style colors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clean solution would be to use the type consistency. This way the description of the SeratoMarkers.getTrackColor()
And
SeratoTags::getTrackColor()
Would be the same.

Returning an optional that can never ever be nullopt does not make any sense.

But whatever, I give up.

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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
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