-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 6 commits
5055657
a831e39
2144b51
e3b5353
cfd14f1
c657a9f
af1fdb1
7b6acf0
ef4ce69
cc2e262
a991d23
c298ab3
1b52a67
cac064b
3326c92
ec9a027
30f203d
b6f37c1
3f4ca14
d565a89
439b985
6184663
666422c
bcf18b9
5e41678
b31edfd
bc2fed7
a864454
18b6ffe
fc6a36a
3df8889
dc597b0
14a29f8
f6b781c
06d393e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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::fromCode(0xF0000000).isValid()); | ||
EXPECT_EQ(0x000000, RgbColor::fromCode(0xF0000000).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF0FF0000).isValid()); | ||
EXPECT_EQ(0xFF0000, RgbColor::fromCode(0xF0FF0000).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF000FF00).isValid()); | ||
EXPECT_EQ(0x00FF00, RgbColor::fromCode(0xF000FF00).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF00000FF).isValid()); | ||
EXPECT_EQ(0x0000FF, RgbColor::fromCode(0xF00000FF).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF0FFFFFF).isValid()); | ||
EXPECT_EQ(0xFFFFFF, RgbColor::fromCode(0xF0FFFFFF).optionalCode()); | ||
} | ||
|
||
TEST(RgbColorTest, FromQColorWithAlpha) { | ||
EXPECT_TRUE(RgbColor::fromCode(0xF0000000).isValid()); | ||
EXPECT_EQ(0x000000, RgbColor::fromCode(0xF0000000).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF0FF0000).isValid()); | ||
EXPECT_EQ(0xFF0000, RgbColor::fromCode(0xF0FF0000).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF000FF00).isValid()); | ||
EXPECT_EQ(0x00FF00, RgbColor::fromCode(0xF000FF00).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF00000FF).isValid()); | ||
EXPECT_EQ(0x0000FF, RgbColor::fromCode(0xF00000FF).optionalCode()); | ||
EXPECT_TRUE(RgbColor::fromCode(0xF0FFFFFF).isValid()); | ||
EXPECT_EQ(0xFFFFFF, RgbColor::fromCode(0xF0FFFFFF).optionalCode()); | ||
} | ||
|
||
} // namespace mixxx |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
#pragma once | ||
|
||
#include <QColor> | ||
|
||
#include <optional> | ||
|
||
#include "util/assert.h" | ||
|
||
namespace mixxx { | ||
|
||
// 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, | ||
uklotzde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is: If I understood correctly, Instead, I propose to use With my suggested approach, we get Furthermore, the following methods are no longer needed:
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 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ;) |
||
} | ||
// Implicit conversion constructors without normalization. | ||
// Only use these conversion constructors if the argument | ||
// matches an RgbColor! Otherwise a debug assertion will | ||
// be triggered. | ||
/*non-explicit*/ RgbColor(RgbColorCode code) | ||
: m_color(codeToColor(code)) { | ||
DEBUG_ASSERT(m_color == normalizeColor(m_color)); | ||
} | ||
/*non-explicit*/ RgbColor(std::optional<RgbColorCode> optionalCode) | ||
: m_color(optionalCodeToColor(optionalCode)) { | ||
DEBUG_ASSERT(m_color == normalizeColor(m_color)); | ||
} | ||
/*non-explicit*/ RgbColor(QColor color) | ||
: m_color(color) { | ||
DEBUG_ASSERT(m_color == normalizeColor(m_color)); | ||
} | ||
|
||
// Check that the provided color code is valid. | ||
static bool isValidCode(RgbColorCode code) { | ||
return code == (code & kRgbCodeMask); | ||
} | ||
uklotzde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Explicit conversions with implicit normalization. | ||
// Use these static functions instead of the conversion | ||
// constructors to ensure that the resulting RgbColor is | ||
// well defined. | ||
static RgbColor fromCode(RgbColorCode code) { | ||
return RgbColor(code & kRgbCodeMask); | ||
} | ||
static RgbColor fromOptionalCode(std::optional<RgbColorCode> code) { | ||
if (code.has_value()) { | ||
return fromCode(code.value()); | ||
} else { | ||
return RgbColor(); | ||
} | ||
} | ||
static RgbColor fromColor(QColor color) { | ||
return RgbColor(normalizeColor(color)); | ||
} | ||
|
||
// 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 { | ||
uklotzde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) { | ||
DEBUG_ASSERT(isValidCode(code)); | ||
return QColor(code | 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 normalizeColor(QColor color) { | ||
if (color.isValid()) { | ||
return codeToColor(validColorToCode(color)); | ||
} else { | ||
return QColor(); | ||
} | ||
} | ||
|
||
QColor m_color; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean this: RgbColorCode m_color There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope. Alpha = 0x00 is transparent. We agreed on using We could also store There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.`
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 withinmixxx
for just a bunch of classes, all with color in their class name?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.