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.

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

Optional and opaque RGB colors #2472

Merged
merged 35 commits into from
Feb 10, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5055657
Add RgbColor class
uklotzde Jan 29, 2020
a831e39
Add equality comparison operators
uklotzde Jan 29, 2020
2144b51
Add note about RgbColorCode vs. QRgb
uklotzde Jan 29, 2020
e3b5353
Add note about immutability
uklotzde Jan 29, 2020
cfd14f1
Add more documentation
uklotzde Jan 29, 2020
c657a9f
Use std::optional for color code conversions
uklotzde Jan 29, 2020
af1fdb1
Reduce variations of conversion operations
uklotzde Jan 29, 2020
7b6acf0
Declare Qt type information
uklotzde Jan 29, 2020
ef4ce69
Replace internal QColor with RgbColorCode
uklotzde Jan 29, 2020
cc2e262
Treat RgbColor as a POD
uklotzde Jan 29, 2020
a991d23
Register global meta type for RgbColor
uklotzde Jan 29, 2020
c298ab3
Fix build for ancient GCC version 5.4.0
uklotzde Jan 29, 2020
1b52a67
Update documentation
uklotzde Jan 30, 2020
cac064b
Replace isValid() with operator bool()
uklotzde Jan 30, 2020
3326c92
Prefer explicit conversion and fix various issues
uklotzde Jan 30, 2020
ec9a027
Remove constexpr
uklotzde Jan 30, 2020
30f203d
Fix type inference in tests
uklotzde Jan 30, 2020
b6f37c1
Add comment about use cases for this class
uklotzde Jan 30, 2020
3f4ca14
Split OptionalRgbColor from RgbColor
uklotzde Jan 30, 2020
d565a89
Use constexpr
uklotzde Jan 30, 2020
439b985
Fix typos in documentation
uklotzde Jan 31, 2020
6184663
Declare a Qt meta type for std::optional<mixxx::RgbColor>>
uklotzde Jan 31, 2020
666422c
Replace OptionalRgbColor with std::optional<RgbColor>
uklotzde Jan 31, 2020
bcf18b9
Remove obsolete validation
uklotzde Jan 31, 2020
5e41678
Add a debug assertion
uklotzde Jan 31, 2020
b31edfd
Move RgbColorCode typedef into RgbColor
uklotzde Jan 31, 2020
bc2fed7
Add missing constexpr
uklotzde Jan 31, 2020
a864454
Add comment about constexpr ctor
uklotzde Jan 31, 2020
18b6ffe
Implicitly validate color codes in RgbColor ctor
uklotzde Jan 31, 2020
fc6a36a
Update class documentation
uklotzde Jan 31, 2020
3df8889
Add debug output stream operators
uklotzde Jan 31, 2020
dc597b0
Fix invalid constexpr
uklotzde Jan 31, 2020
14a29f8
Add conversions from/to QVariant
uklotzde Jan 31, 2020
f6b781c
Run clang-format
uklotzde Feb 1, 2020
06d393e
Add missing RgbColor tests
uklotzde Feb 7, 2020
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
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,7 @@ add_executable(mixxx-test
src/test/readaheadmanager_test.cpp
src/test/replaygaintest.cpp
src/test/rescalertest.cpp
src/test/rgbcolor_test.cpp
src/test/samplebuffertest.cpp
src/test/sampleutiltest.cpp
src/test/schemamanager_test.cpp
Expand Down
66 changes: 66 additions & 0 deletions src/test/rgbcolor_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#include "util/color/rgbcolor.h"

#include <gtest/gtest.h>

#include "test/mixxxtest.h"

namespace mixxx {

TEST(RgbColorTest, DefaultIsInvalid) {
EXPECT_FALSE(RgbColor().isValid());
EXPECT_FALSE(RgbColor().optionalCode().has_value());
}

TEST(RgbColorTest, RgbColorCodeCtor) {
EXPECT_TRUE(RgbColor(0x000000).isValid());
EXPECT_EQ(0x000000, RgbColor(0x000000).optionalCode());
EXPECT_TRUE(RgbColor(0xFF0000).isValid());
EXPECT_EQ(0xFF0000, RgbColor(0xFF0000).optionalCode());
EXPECT_TRUE(RgbColor(0x00FF00).isValid());
EXPECT_EQ(0x00FF00, RgbColor(0x00FF00).optionalCode());
EXPECT_TRUE(RgbColor(0x0000FF).isValid());
EXPECT_EQ(0x0000FF, RgbColor(0x0000FF).optionalCode());
EXPECT_TRUE(RgbColor(0xFFFFFF).isValid());
EXPECT_EQ(0xFFFFFF, RgbColor(0xFFFFFF).optionalCode());
}

TEST(RgbColorTest, QColorCtor) {
EXPECT_TRUE(RgbColor(QColor::fromRgb(0x000000)).isValid());
EXPECT_EQ(0x000000, RgbColor(QColor::fromRgb(0x000000)).optionalCode());
EXPECT_TRUE(RgbColor(QColor::fromRgb(0xFF0000)).isValid());
EXPECT_EQ(0xFF0000, RgbColor(QColor::fromRgb(0xFF0000)).optionalCode());
EXPECT_TRUE(RgbColor(QColor::fromRgb(0x00FF00)).isValid());
EXPECT_EQ(0x00FF00, RgbColor(QColor::fromRgb(0x00FF00)).optionalCode());
EXPECT_TRUE(RgbColor(QColor::fromRgb(0x0000FF)).isValid());
EXPECT_EQ(0x0000FF, RgbColor(QColor::fromRgb(0x0000FF)).optionalCode());
EXPECT_TRUE(RgbColor(QColor::fromRgb(0xFFFFFF)).isValid());
EXPECT_EQ(0xFFFFFF, RgbColor(QColor::fromRgb(0xFFFFFF)).optionalCode());
}

TEST(RgbColorTest, FromRgbColorCodeWithAlpha) {
EXPECT_TRUE(RgbColor(0xF0000000).isValid());
EXPECT_EQ(0x000000, RgbColor(0xF0000000).optionalCode());
EXPECT_TRUE(RgbColor(0xF0FF0000).isValid());
EXPECT_EQ(0xFF0000, RgbColor(0xF0FF0000).optionalCode());
EXPECT_TRUE(RgbColor(0xF000FF00).isValid());
EXPECT_EQ(0x00FF00, RgbColor(0xF000FF00).optionalCode());
EXPECT_TRUE(RgbColor(0xF00000FF).isValid());
EXPECT_EQ(0x0000FF, RgbColor(0xF00000FF).optionalCode());
EXPECT_TRUE(RgbColor(0xF0FFFFFF).isValid());
EXPECT_EQ(0xFFFFFF, RgbColor(0xF0FFFFFF).optionalCode());
}

TEST(RgbColorTest, FromQColorWithAlpha) {
EXPECT_TRUE(RgbColor(0xF0000000).isValid());
EXPECT_EQ(0x000000, RgbColor(0xF0000000).optionalCode());
EXPECT_TRUE(RgbColor(0xF0FF0000).isValid());
EXPECT_EQ(0xFF0000, RgbColor(0xF0FF0000).optionalCode());
EXPECT_TRUE(RgbColor(0xF000FF00).isValid());
EXPECT_EQ(0x00FF00, RgbColor(0xF000FF00).optionalCode());
EXPECT_TRUE(RgbColor(0xF00000FF).isValid());
EXPECT_EQ(0x0000FF, RgbColor(0xF00000FF).optionalCode());
EXPECT_TRUE(RgbColor(0xF0FFFFFF).isValid());
EXPECT_EQ(0xFFFFFF, RgbColor(0xF0FFFFFF).optionalCode());
}

} // namespace mixxx
120 changes: 120 additions & 0 deletions src/util/color/rgbcolor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
#pragma once

#include <QColor>
#include <QtGlobal>

#include <optional>

#include "util/assert.h"

namespace mixxx {
Copy link
Contributor

Choose a reason for hiding this comment

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

@uklotzde Is it ok if I move this class into the Color namespace in my cue color branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please coordinate this with @Holzhaus to avoid merge conflicts. Better do it in a separate PR that is expected to be merged quickly.`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please also note that namespace names should be lowercase:

https://google.github.io/styleguide/cppguide.html#Namespace_Names

The Color namespace violates this rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferranpujolcamins Do we actually need a separate color namespace within mixxx for just a bunch of classes, all with color in their class name?

Copy link
Contributor

Choose a reason for hiding this comment

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

The color namespace was there to group some free functions. We can reuse it to include your class. But it doesn't really matter to me. Just let me know what you think it's best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With argument dependent lookup and function overloading available in C++ there is no need to hide free functions in artificial namespaces. Or do I miss a use case?

We are free to chose whatever works best for us. However keep an eye on discoverability and developer experience in general.


// A pure 24-bit 0xxRRGGBB color code without an alpha channel.
// We are using a separate typedef, because QRgb implicitly
// includes an alpha channel whereas this type does not!
typedef quint32 RgbColorCode;

// A thin wrapper around QColor to represent optional,
// opaque RGB colors with 8-bit per channel without an
// alpha channel. It ensures that the alpha channel is set
// to opaque for valid RGB colors and that invalid colors
// are always represented by the same, well defined QColor,
// namely QColor().
// Apart from assignment this type is immutable.
class RgbColor {
public:
RgbColor() {
DEBUG_ASSERT(!isValid());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the debug assertion? This is already covered by the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, we could discuss whether we want the "optionality" to be baked in RgbColor. From my point of view, it'd more flexible to have a type that represents a valid color. If you want an optional color, that's what std::optional is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must handle it, because QColor requires us to do so. Otherwise this class could not be agnostic and would require use case specific quirks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mapping to non-optional values has to be done on the next abstraction level, not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debug assertions might reveal bugs that we didn't anticipate in the tests. Yes, these are helpful and intended!

Copy link
Contributor

@ferranpujolcamins ferranpujolcamins Jan 30, 2020

Choose a reason for hiding this comment

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

What I mean is:

If I understood correctly, RgbColor is just RgbColorCode with a special value to indicate an invalid color (and some validation to make sure we don't accidentally set an invalid color).

Instead, I propose to use std::optional<RgbColorCode>. Here we don't treat any value of RgbColorCode as "invalid", so we can just ignore the upper bits.

With my suggested approach, we get !=, == and bool() for free from std::optional, while you needed to implement them for RgbColor.

Furthermore, the following methods are no longer needed:

  • optional()
  • optionalCodeToInternalCode (any RgbColorCode is valid, we just ignore the upper bits)

The only code you actually need to write is a bunch of free functions to convert back and forth QColor:

QColor toQColor(RgbColorCode color) {
    return QColor::fromRgb(m_internalCode & kRgbCodeMask);
}

QColor toQColor(std::optional<RgbColorCode> color) {
    if (color) {
        return toQColor(*color);
    } else {
        return QColor();
    }
}

std::optional<RgbColorCode> toRgb(QColor color) {
    if (anyColor.isValid()) {
        return std::make_optional<RgbColorCode>(anyColor.rgb())
    } else {
        return std::optional<RgbColorCode>()
    }
}

Another advantage of this approach is that you can express whether you accept invalid colors or not in a function, by asking for a std::optional<RgbColorCode> or a RgbColorCode. This is not possible with your proposed approach.


All this being said, there's no need for more discussion unless you have some questions about what I've written here.

If you feel you approach is best, I'm fine. Just wanted you to consider this other one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferranpujolcamins Hm, I need to think about this again. Your remarks about optionality are valid.

I don't have any stakes in here and just wanted to demonstrate that we need some kind of abstraction. But it quickly escalated in terms of work effort ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, sometimes just happens that your abstraction can just be a type alias, because what you need is already there ;)

}
/*non-explicit*/ RgbColor(RgbColorCode code)
: m_color(codeToColor(code)) {
DEBUG_ASSERT(m_color == normalizeAnyColor(m_color));
}
/*non-explicit*/ RgbColor(std::optional<RgbColorCode> optionalCode)
: m_color(optionalCodeToColor(optionalCode)) {
DEBUG_ASSERT(m_color == normalizeAnyColor(m_color));
}
// Implicit conversion from QColor without normalization.
// Only use this conversion constructor if the argument
// matches an RgbColor! Otherwise a debug assertion will
// be triggered.
/*non-explicit*/ RgbColor(QColor color)
: m_color(color) {
DEBUG_ASSERT(m_color == normalizeAnyColor(m_color));
}
// Explicit conversion from any QColor with implicit
// normalization that results in a well defined RgbColor.
// Use this static function instead of the conversion
// constructor to ensure that the resulting RgbColor is
// well defined when converting from some QColor.
static RgbColor fromAnyColor(QColor anyColor) {
return RgbColor(normalizeAnyColor(anyColor));
}

// Implicit conversion into the corresponding QColor.
operator QColor() const {
return m_color;
}

// Checks if the color is valid or represents "no color",
// i.e. is missing or undefined. If the corresponding color
// code is stored in a database then the colum value is NULL.
bool isValid() const {
return m_color.isValid();
}

// Returns the corresponding, optional RGB color code.
std::optional<RgbColorCode> optionalCode() const {
return colorToOptionalCode(m_color);
}

friend bool operator==(const RgbColor& lhs, const RgbColor& rhs) {
return lhs.m_color == rhs.m_color;
}

private:
static const RgbColorCode kRgbCodeMask = 0x00FFFFFF;
static const RgbColorCode kAlphaCodeMask = 0xFF000000;

static QColor codeToColor(RgbColorCode code) {
return QColor((code & kRgbCodeMask) | kAlphaCodeMask);
}
static QColor optionalCodeToColor(std::optional<RgbColorCode> optionalCode) {
if (optionalCode.has_value()) {
return codeToColor(optionalCode.value());
} else {
return QColor();
}
}

static RgbColorCode validColorToCode(QColor color) {
DEBUG_ASSERT(color.isValid());
return color.rgb() & kRgbCodeMask;
}
static std::optional<RgbColorCode> colorToOptionalCode(QColor color) {
if (color.isValid()) {
return std::make_optional(validColorToCode(color));
} else {
return std::nullopt;
}
}

static QColor normalizeAnyColor(QColor anyColor) {
if (anyColor.isValid()) {
return codeToColor(validColorToCode(anyColor));
} else {
return QColor();
}
}

QColor m_color;
Copy link
Member

Choose a reason for hiding this comment

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

I mean this:

RgbColorCode m_color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use RgbColorCode as storage variable

What does that mean, any links? Please elaborate.

See my inline comment. You may use 0xFFFFFFFF for invalid. That automatically defaults to transparent, which is what we want in most cases.

Nope. Alpha = 0x00 is transparent.

We agreed on using QColor internally and this is the consequent follow-up and reasonable software design for such an approach. RgbColor allows even less variations than QColor.

We could also store RgbColorCode internally with alpha = 0xFF for all valid opaque colors and 0x00000000 as a special value for invalid/transparent/no color. But this custom mapping must not be exposed publicly! The only conversions that are provided are to/from QColor and std::optional<RgbColorCode>. The public API wouldn't change.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that was the Idea to keep the whole thing performant.

What is the public API you reffering to?
We have the CO interface that uses the 24 bit color as double, the ES interface that uses tree values for colors, the file tag that uses also a 24 bit value.
So it looks like QColor is only required to feed the Qt painting interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

invalid/transparent/no color

Let's be clear with language. These are not identical concepts. There are contexts in which transparent is a valid value such as track color; calling it invalid would be a misnomer.

};

inline bool operator!=(const RgbColor& lhs, const RgbColor& rhs) {
return !(lhs == rhs);
}

} // namespace mixxx

Q_DECLARE_TYPEINFO(mixxx::RgbColor, Q_MOVABLE_TYPE);
Q_DECLARE_METATYPE(mixxx::RgbColor)