-
-
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
Hotcue RGB colors #2520
Hotcue RGB colors #2520
Changes from 1 commit
75e6e2a
393ac12
42a22b0
86bda12
48b2b02
fcb4a1d
c22b737
f017447
f68f28c
66d3ffa
30b82cf
1a3b8aa
7a5d13f
23e114f
0ef7da4
bbc4503
2259321
e1f92ec
413c17c
74b098c
75fdbed
7702c6c
738db3a
123172c
f3153b7
39928d6
f259558
5732911
fafe58b
37133d9
ac28f21
a477c87
d803ea8
78c5101
080663c
3864c80
16a4778
b3b9c0d
d4a0508
e1cc9be
f97a15b
f8ccbcd
8fe1bd2
f313cc7
1dad294
09f6982
5da79b0
7e90a64
6c8496f
8a4e87d
df91a83
dac8810
0e27925
327702e
e964345
a2d789c
982f7b7
e8ce23e
4938898
ba3b84d
11935e2
74e884a
d54fd95
709cb3f
e6a4735
5b846c5
52062f3
6181401
dc9e476
fcf2401
b8ee8d6
26f8c3b
a0379dc
dd353e6
328edfb
bb2f30b
129e0e6
7022535
35e2e79
8060f91
d7557ad
9d1cbf4
b648a5d
c8ad295
6841da6
8d36723
fa6dba8
4180310
1ef5084
43cbedb
287855a
0621d0a
3f94fd6
5b4c695
4bc091e
dcec320
80eb2ba
9b68234
df02dec
c4be500
448d1b7
f1ac5de
8b222ff
c52f8b3
9e0c651
abea669
67af302
ef8e535
fbcea0b
5cda96d
e719af5
74e6cf7
078e595
43fc59d
ee5e5e4
7fe6f81
0a423a0
ba01c5f
9883318
3c0a3fe
b4e9de9
90b1109
20ce76f
8ca8003
6eb2124
be415ba
eb2fd28
6046abf
a538a09
702050f
e859c7d
42f82a3
9e9a56f
2a0608b
a48afde
05012ec
f3a954e
2f95d2d
f50ccbe
44b8237
abd663f
e7a3553
ec11d6b
714f446
8dca9a6
9130b80
4a79af3
7cc0444
92df887
be48247
b4855e0
9712d26
4e943f8
0e18f19
dac09b2
6499025
5380904
233e99f
c0fd0b4
96ca1d2
827f6a0
e18f8d5
243fb32
4944594
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,22 @@ static const double CUE_MODE_CUP = 5.0; | |
|
||
constexpr double kNoColorControlValue = -1; | ||
|
||
namespace { | ||
|
||
// Helper function to convert doubles into RgbColor instances (or nullopt) | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
inline mixxx::RgbColor::optional_t doubleToRgbColor(double value) { | ||
ywwg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (value < 0) { | ||
return std::nullopt; | ||
} | ||
mixxx::RgbColor::code_t colorCode = static_cast<mixxx::RgbColor::code_t>(value); | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (value != mixxx::RgbColor::validateCode(colorCode)) { | ||
return std::nullopt; | ||
} | ||
return mixxx::RgbColor::optional(colorCode); | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
} // namespace | ||
|
||
CueControl::CueControl(QString group, | ||
UserSettingsPointer pConfig) | ||
: EngineControl(group, pConfig), | ||
|
@@ -1865,13 +1881,13 @@ void HotcueControl::slotHotcuePositionChanged(double newPosition) { | |
} | ||
|
||
void HotcueControl::slotHotcueColorChanged(double newColor) { | ||
if (newColor < 0) { | ||
mixxx::RgbColor::optional_t color = doubleToRgbColor(newColor); | ||
if (!color) { | ||
qWarning() << "slotHotcueColorChanged got invalid value:" << newColor; | ||
return; | ||
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. Due to this return, the co and the cue object are out of sync. 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. Ok 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. Done. 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 actually undid that change after some local tests. This does not work properly, e.g. the color is not unset if the color is deleted. Also, it's a bit unsettling to not check if And no other CO in that file checks the value before setting it, hotcue color handling would the be only CO that does this. We can introduce more value change requests to reduce the possible values in the slots, but lets please do that separately, not as part of this PR. |
||
} | ||
|
||
DEBUG_ASSERT(newColor == mixxx::RgbColor::validateCode(static_cast<mixxx::RgbColor::code_t>(newColor))); | ||
m_pCue->setColor(mixxx::RgbColor(static_cast<mixxx::RgbColor::code_t>(newColor))); | ||
m_pCue->setColor(*color); | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
emit(hotcueColorChanged(this, newColor)); | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
|
@@ -1887,21 +1903,15 @@ void HotcueControl::setCue(CuePointer pCue) { | |
m_pCue = pCue; | ||
} | ||
mixxx::RgbColor::optional_t HotcueControl::getColor() const { | ||
double value = m_hotcueColor->get(); | ||
if (value < 0) { | ||
return std::nullopt; | ||
} | ||
|
||
DEBUG_ASSERT(value == mixxx::RgbColor::validateCode(static_cast<mixxx::RgbColor::code_t>(value))); | ||
return mixxx::RgbColor(static_cast<mixxx::RgbColor::code_t>(value)); | ||
return doubleToRgbColor(m_hotcueColor->get()); | ||
} | ||
|
||
void HotcueControl::setColor(const mixxx::RgbColor::optional_t newColor) { | ||
if (newColor) { | ||
m_hotcueColor->set(*newColor); | ||
if (!newColor) { | ||
Holzhaus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
m_hotcueColor->set(kNoColorControlValue); | ||
return; | ||
} | ||
m_hotcueColor->set(kNoColorControlValue); | ||
m_hotcueColor->set(*newColor); | ||
} | ||
void HotcueControl::resetCue() { | ||
// clear pCue first because we have a null check for valid data else where | ||
|
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.
I think we wanted to get rid of this? This triggers not well designed code paths everywhere.
Can we use the default color instead?
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.
No, this is used for
HotcueControl
(notCue
). We need this because aHotcueControl
is not always associated with aCue
- this is the case if a hotcue is not set or no track is loaded. I don't want my performance pads to light up in the default color in these cases.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.
Oh, I think we have a conceptional issue here. The color stored in hotcue_nn_color control is not responsible to set the lit state. See notes https://github.com/mixxxdj/mixxx/pull/2520/files#diff-9bedde251ff7d14d89b40d05168275e4R313
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.
Yes, but in any case the control value should be
-1
. We do the same for hotcue positions, etc.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.
We can consider it, but there should not be a need at all. Setting the cue position to -1 means it is not set, this is a programmatic requirement and should be the only one.
Currently parts of the code are looking to the position others to the color. That is the conceptual issue that should be fixed.
Once this is the case, the color can remain where it was. This has the benefit that we can assert that the hotcue_nn_color CO alwasy holds a valid color. So there is no need for extended handling.
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.
I can't assert that
hotcue_nn_color
always holds a valid color, because COs could be set to any value. What if I start mixxx with--developer
and set the CO via the "Developer Tools" to-12.74
? We need the value checks anyway.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.
I don't mind if we trigger a debug assert in this case. If you are, we can register the change request hook and reject all invalid values.
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.
Ok, I will issue that in my follow up PR.