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

Holzhaus · 2023-01-25T11:17:02Z

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:

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

Fixes #11213.

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.

Holzhaus · 2023-01-25T11:18:19Z

@istepaniuk Please verify if this fixes the issue.

daschuer · 2023-01-25T13:03:44Z

src/track/serato/tags.cpp

    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) {


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.

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".

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`

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.

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.

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").

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.

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.

istepaniuk · 2023-01-25T13:24:07Z

I'm not immediately able to build your branch, cmake stops at -- Could NOT find KeyFinder (missing: KeyFinder_LIBRARY KeyFinder_INCLUDE_DIR) (Required is at least version "2.2.6"). I'll look into it later (or pick your changes onto master to test this)

daschuer · 2023-01-25T13:46:15Z

Try to build from a new build directory. If this issue still succours. Please post the CMake output here.
Normally it should fall back to the internal keyfinder library.

Alternatively build with -DKEYFINDER=OFF

istepaniuk · 2023-01-25T14:24:44Z

Nevermind, I was just missing qtscript5-dev.

I tested this and can confirm that colors stay assigned when reimporting metadata. I also tested several combinations of set color, modify externally, reload and export and the behavior is the expected.

Enabling the Serato options seems to store/load the color as expected too, at least from Mixxx's perspective.

Thanks for looking into this 🥇

daschuer · 2023-01-28T10:49:57Z

After a private discussion @Holzhaus and I came to the conclusion that it is best to merge this known working PR as a band aid and fix the underlying type safety issue by introducing SeratoStoredTrackColor und SeratoStoredHotcueColor in a separate PR.
Thank you for this PR and the short responds time to the original bug report.

JoergAtGithub · 2023-01-28T11:00:04Z

@daschuer @Holzhaus Thank you for resolving the matter together!

Holzhaus added bug metadata serato labels Jan 25, 2023

Holzhaus added this to the 2.3.4 milestone Jan 25, 2023

github-actions bot added the code quality label Jan 25, 2023

daschuer requested changes Jan 25, 2023

View reviewed changes

docs(serato): Explain Serato track color return values in comments

50c54ce

Holzhaus closed this Jan 27, 2023

Holzhaus deleted the fix-serato-track-color-reset-on-import branch January 27, 2023 19:20

Holzhaus restored the fix-serato-track-color-reset-on-import branch January 28, 2023 09:49

Holzhaus reopened this Jan 28, 2023

daschuer approved these changes Jan 28, 2023

View reviewed changes

daschuer merged commit d02fdc0 into mixxxdj:2.3 Jan 28, 2023

Holzhaus mentioned this pull request Jan 31, 2023

Serato color types #11228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Holzhaus commented Jan 25, 2023

Holzhaus commented Jan 25, 2023

daschuer Jan 25, 2023 •

edited by Swiftb0y

Loading

Holzhaus Jan 26, 2023

daschuer Jan 26, 2023

Holzhaus Jan 27, 2023

daschuer Jan 27, 2023

Holzhaus Jan 27, 2023

daschuer Jan 27, 2023

Holzhaus Jan 27, 2023

istepaniuk commented Jan 25, 2023

daschuer commented Jan 25, 2023 •

edited

Loading

istepaniuk commented Jan 25, 2023

daschuer commented Jan 28, 2023 •

edited

Loading

JoergAtGithub commented Jan 28, 2023

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

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

Conversation

Holzhaus commented Jan 25, 2023

Holzhaus commented Jan 25, 2023

daschuer Jan 25, 2023 • edited by Swiftb0y Loading

Choose a reason for hiding this comment

Holzhaus Jan 26, 2023

Choose a reason for hiding this comment

daschuer Jan 26, 2023

Choose a reason for hiding this comment

Holzhaus Jan 27, 2023

Choose a reason for hiding this comment

daschuer Jan 27, 2023

Choose a reason for hiding this comment

Holzhaus Jan 27, 2023

Choose a reason for hiding this comment

daschuer Jan 27, 2023

Choose a reason for hiding this comment

Holzhaus Jan 27, 2023

Choose a reason for hiding this comment

istepaniuk commented Jan 25, 2023

daschuer commented Jan 25, 2023 • edited Loading

istepaniuk commented Jan 25, 2023

daschuer commented Jan 28, 2023 • edited Loading

JoergAtGithub commented Jan 28, 2023

daschuer Jan 25, 2023 •

edited by Swiftb0y

Loading

daschuer commented Jan 25, 2023 •

edited

Loading

daschuer commented Jan 28, 2023 •

edited

Loading