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 34 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
2 changes: 2 additions & 0 deletions src/mixxxapplication.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "soundio/soundmanagerutil.h"
#include "track/track.h"
#include "track/trackref.h"
#include "util/color/rgbcolor.h"
#include "util/math.h"

// When linking Qt statically on Windows we have to Q_IMPORT_PLUGIN all the
Expand Down Expand Up @@ -67,6 +68,7 @@ void MixxxApplication::registerMetaTypes() {
qRegisterMetaType<mixxx::ReplayGain>("mixxx::ReplayGain");
qRegisterMetaType<mixxx::Bpm>("mixxx::Bpm");
qRegisterMetaType<mixxx::Duration>("mixxx::Duration");
qRegisterMetaType<std::optional<mixxx::RgbColor>>("std::optional<mixxx::RgbColor>");
qRegisterMetaType<SoundDeviceId>("SoundDeviceId");
QMetaType::registerComparators<SoundDeviceId>();
}
Expand Down
50 changes: 50 additions & 0 deletions src/test/rgbcolor_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#include "util/color/rgbcolor.h"

#include <gtest/gtest.h>

#include "test/mixxxtest.h"

namespace mixxx {

TEST(RgbColorTest, OptionalRgbColorFromInvalidQColor) {
EXPECT_FALSE(RgbColor::optional(QColor()));
EXPECT_EQ(RgbColor::nullopt(), RgbColor::optional(QColor()));
}

TEST(RgbColorTest, OptionalRgbColorFromQColorWithoutAlpha) {
EXPECT_TRUE(RgbColor::optional(QColor::fromRgb(0x000000)));
EXPECT_EQ(0x000000, *RgbColor::optional(QColor::fromRgb(0x000000)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgb(0xFF0000)));
EXPECT_EQ(RgbColor::optional(0xFF0000), RgbColor::optional(QColor::fromRgb(0xFF0000)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgb(0x00FF00)));
EXPECT_EQ(RgbColor::optional(0x00FF00), RgbColor::optional(QColor::fromRgb(0x00FF00)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgb(0x0000FF)));
EXPECT_EQ(RgbColor::optional(0x0000FF), RgbColor::optional(QColor::fromRgb(0x0000FF)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgb(0xFFFFFF)));
EXPECT_EQ(RgbColor::optional(0xFFFFFF), RgbColor::optional(QColor::fromRgb(0xFFFFFF)));
}

TEST(RgbColorTest, OptionalRgbColorFromQColorWithAlpha) {
EXPECT_TRUE(RgbColor::optional(QColor::fromRgba(0xAA000000)));
EXPECT_EQ(RgbColor::optional(0x000000), RgbColor::optional(QColor::fromRgba(0xAA000000)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgba(0xAAFF0000)));
EXPECT_EQ(RgbColor::optional(0xFF0000), RgbColor::optional(QColor::fromRgba(0xAAFF0000)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgba(0xAA00FF00)));
EXPECT_EQ(RgbColor::optional(0x00FF00), RgbColor::optional(QColor::fromRgba(0xAA00FF00)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgba(0xAA0000FF)));
EXPECT_EQ(RgbColor::optional(0x0000FF), RgbColor::optional(QColor::fromRgba(0xAA0000FF)));
EXPECT_TRUE(RgbColor::optional(QColor::fromRgba(0xAAFFFFFF)));
EXPECT_EQ(RgbColor::optional(0xFFFFFF), RgbColor::optional(QColor::fromRgba(0xAAFFFFFF)));
}

TEST(RgbColorTest, OptionalRgbColorToQColor) {
EXPECT_EQ(QColor(), toQColor(RgbColor::nullopt()));
EXPECT_EQ(QColor::fromRgba(0xAABBCCDD),
toQColor(RgbColor::nullopt(), QColor::fromRgba(0xAABBCCDD)));
EXPECT_EQ(QColor::fromRgb(0x123456),
toQColor(RgbColor::optional(QColor::fromRgba(0xAA123456))));
EXPECT_EQ(QColor::fromRgb(0x123456),
toQColor(RgbColor::optional(QColor::fromRgba(0xAA123456)), QColor::fromRgba(0xAABBCCDD)));
}

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

#include <QColor>
#include <QVariant>
#include <QtDebug>
#include <QtGlobal>

#include "util/assert.h"
#include "util/optional.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.


// Type-safe wrapper for 24-bit RGB color codes without an alpha
// channel. Code values are implicitly validated upon construction.
//
// Apart from the assignment operator this type is immutable.
class RgbColor {
public:
// A pure 24-bit 0xxRRGGBB color code with 8-bit per channel
// without an an alpha channel.
// We are using a separate typedef, because QRgb implicitly
// includes an alpha channel whereas this type does not!
typedef quint32 code_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the t stand 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.

Shorthand for "type", common suffix in C/C++ for types and typedefs. The lowercase type names also pickup the spelling of the corresponding C++ types, i.e. optional. Lowercase type names for primitive types or PODs are also not uncommon. This keeps the definitions consistent and intuitive.


static constexpr code_t validateCode(code_t code) {
return code & kRgbCodeMask;
}
static bool isValidCode(code_t code) {
return code == validateCode(code);
}

// The default constructor is not available, because there is
// no common default value that fits all possible use cases!
RgbColor() = delete;
// Explicit conversion from code_t with implicit validation.
explicit constexpr RgbColor(code_t code)
: m_code(validateCode(code)) {
}
// Explicit conversion from QColor.
RgbColor(QColor anyColor, code_t codeIfInvalid)
: m_code(anyColorToCode(anyColor, codeIfInvalid)) {
}

// Implicit conversion to a color code.
constexpr operator code_t() const {
return m_code;
}

friend bool operator==(RgbColor lhs, RgbColor rhs) {
return lhs.m_code == rhs.m_code;
}

typedef std::optional<RgbColor> optional_t;

static constexpr optional_t nullopt() {
return std::nullopt;
}

// Overloaded conversion functions for conveniently creating
// std::optional<RgbColor>.
static constexpr optional_t optional(code_t code) {
return optional(RgbColor(code));
}
static constexpr optional_t optional(RgbColor color) {
return std::make_optional(color);
}
static optional_t optional(QColor color) {
if (color.isValid()) {
return optional(validateCode(color.rgb()));
} else {
return nullopt();
}
}
static optional_t optional(const QVariant& varCode) {
if (varCode.isNull()) {
return nullopt();
} else {
DEBUG_ASSERT(varCode.canConvert(QMetaType::UInt));
bool ok = false;
const auto code = varCode.toUInt(&ok);
VERIFY_OR_DEBUG_ASSERT(ok) {
return nullopt();
}
return optional(static_cast<code_t>(code));
}
}

protected:
// Bitmask of valid codes = 0x00RRGGBB
static constexpr code_t kRgbCodeMask = 0x00FFFFFF;

static code_t anyColorToCode(QColor anyColor, code_t codeIfInvalid) {
if (anyColor.isValid()) {
// Strip alpha channel bits!
return validateCode(anyColor.rgb());
} else {
return codeIfInvalid;
}
}

code_t m_code;
};

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

// Explicit conversion of both non-optional and optional
// RgbColor values to QColor as overloaded free functions.
Copy link
Member

Choose a reason for hiding this comment

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

Why you decide her for a free function? I think it is more QT like to have a member function for this.

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 cannot add new functions to std::optional. The conversion functions for RgbColor and std::optional<RgbColor should use the same invocation pattern. In C++ this is only possible with a free function.

Copy link
Member

Choose a reason for hiding this comment

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

Why is std::optional relevant here?
Can't we just introduce
RgbColor::toQColor()

Copy link
Contributor Author

@uklotzde uklotzde Feb 4, 2020

Choose a reason for hiding this comment

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

No. Because std::nullopt -> QColor() (= invalid) is not covered by just RgbColor.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand the point here.
I think I have missed something.

We have here the toQColor() function taking a RgbColor. There is no std::nullopt involved.

RgbColor rgb
QColor c1 = toQColor(rgb);
QColor c2 = rgb.toQColor();

What is the issue with the later?

Copy link
Member

Choose a reason for hiding this comment

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

oh I have some ideas, hit me up on zulip

Copy link
Member

Choose a reason for hiding this comment

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

ok sounds like we have consensus on this PR!

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 decision to map std::nullopt to QColor() should be done here, only once and it should be tested. Instead of hoping that developers use this convention at various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRY

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 same arguments apply for the conversion to QVariant. Let's not repeat those bad practices in the current code base again and again.


inline QColor toQColor(RgbColor color) {
return QColor::fromRgb(color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Qt will ignore the alpha bits, so you can remove the bitmask. Is there any other reason to have it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QColor::fromRgb() implicitly adds alpha bits. But QColor::rgb() doesn't remove them. Try it without the bitmask and the tests will fail.

}

inline QColor toQColor(RgbColor::optional_t optional, QColor defaultColor = QColor()) {
if (optional) {
return toQColor(*optional);
} else {
return defaultColor;
}
}

// Explicit conversion of both non-optional and optional
// RgbColor values to QVariant as overloaded free functions.

inline QVariant toQVariant(RgbColor color) {
return QVariant(static_cast<RgbColor::code_t>(color));
}

inline QVariant toQVariant(RgbColor::optional_t optional) {
if (optional) {
return toQVariant(*optional);
} else {
return QVariant();
}
}

// Debug output stream operators

inline QDebug operator<<(QDebug dbg, RgbColor color) {
return dbg << toQColor(color);
}

inline QDebug operator<<(QDebug dbg, RgbColor::optional_t optional) {
return dbg << toQColor(optional);
}

} // namespace mixxx

// Assumption: A primitive type wrapped into std::optional is
// still a primitive type.
Q_DECLARE_TYPEINFO(std::optional<mixxx::RgbColor>, Q_PRIMITIVE_TYPE);
Q_DECLARE_METATYPE(std::optional<mixxx::RgbColor>)
24 changes: 24 additions & 0 deletions src/util/optional.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#pragma once

#if __has_include(<optional>) || \
(defined(__cpp_lib_optional) && __cpp_lib_optional >= 201606L)

#include <optional>

#else

#include <experimental/optional>

namespace std {

using std::experimental::make_optional;
using std::experimental::nullopt;
using std::experimental::optional;

// Workarounds for missing member functions:
// option::has_value() -> explicit operator bool()
// option::value() -> operator*()

} // namespace std

#endif