use QColor for cue colors #2345

ferranpujolcamins · 2019-11-02T20:02:18Z

This PR follows from the conversation on #2119.

Holzhaus · 2019-11-02T22:53:52Z

res/schema.xml

+    </description>
+    <sql>
+      <!-- No Color becomes black-->
+      UPDATE cues SET color=4278190080 WHERE color=0;


How do you differentiate between actual black and no color? IMHO you should make the color column nullable and map no color to NULL.

In the new implementation No Color is gone. Hotcues always have a color.
Thus we need to assign a default color to the hotcues that had No Color assigned.

I think we need to have a closer look at it because of the migration path. With a default colour black, all exiting cue points will turn to black. I think that is not a good color for default.

What is the issue you are trying to solve by removing the No Color?

We can think of another color.

In my opinion, the only value that No Color brings is that it allows users to use colors as "categories" of hotcues. I.e. blue is where the vocals start, red is where the drop starts, etc.. Thus No Color would mean that the cue has not been assigned to any category yet.
No Color needs to be displayed somehow, so we mapped it to a default color.

However, with the new design, the user can just add another color to the palette and think about it like "the hotcue has not been assigned to any category yet". Since the default color assigned to a hotcue will be configurable, this works well.

In my opinion, the former is simpler from the point of view of the user, but also in code.

all exiting cue points will turn to black

Only the ones with No Color. If a hotcue had a color assigned we will respect that. The default mixxx palette is the one we designed as our PredefinedColorsSet

Currently the legacy cues are pure red in the skins. Can we pick a different color or actually black? New set cues are palette red, I had expected to have them black as well.

This fits somehow, because users don't have to care about cue colors in the past and when they may sitll don't want to care they may continue to produce legacy cues like before.

We may consider to give a default color (including black) for newly created cues in Decks preferences.

If I'm not mistaken, hotcue colors have not been part of any Mixxx release yet, so the number of users with manually-colored cue points should be neglible. Hence I'd say let's set uncolored hotcues either to the first color in the palette (red), or assign a palette color based on the number (like the AutoHotcueColors option).

IMHO this is a much better solution than mapping all uncolored hotcue to black. It would either need a lot of manual hacking around the issue to display a replacement color, or - if we handle the black cue like any other cue and pass it to the nearestColor implementation - the performance pads on controllers would more or less display a random color (since most controllers don't have "black" LEDs I assume).

Hmmm, I thought the cue colors were included in the latest release. Then we can just remove this DB migration

We can't remove the DB migration because we have users who have been using master with the old schema.

DB migrations on master are immutable and must not be reverted! They can only be compensated by a subsequent migration.

src/util/color/hotcuecolorpalette.cpp

src/library/dlgtrackinfo.cpp

daschuer · 2019-11-03T09:14:40Z

res/schema.xml

+    </description>
+    <sql>
+      <!-- No Color becomes black-->
+      UPDATE cues SET color=4278190080 WHERE color=0;


We need to keep in mind, that a user comming from a earlyer version of Mixxx may has thousands of with cues all turing to black. This will look ugly and forces him to think about colours even if he does not want to use them. Using a usable default color for it will work, but this creates the situation that this default colour may already has a meaning for the user. A explicited No Colour solved that.

How about using fully transparent for default?

src/engine/controls/cuecontrol.cpp

ferranpujolcamins · 2019-11-03T09:27:02Z

We need to keep in mind, that a user comming from a earlyer version of Mixxx may has thousands of with cues all turing to black. This will look ugly and forces him to think about colours even if he does not want to use them. Using a usable default color for it will work, but this creates the situation that this default colour may already has a meaning for the user. A explicited No Colour solved that.

How about using fully transparent for default?

Then all these hotcues won't be displayed on the waveform, which will be confusing for the user. Furthermore the user will need to manually set each hotcue color to a non-transparent one if they want to fix this situation. Right now we don't have mass-edit capabilities for hotcue colors.

If we keep No Color, we still need to choose a default color when we render the hotcue on screen (this is what was happening before this PR), so it can still happen that the color we choose already has a meaning for the user.

Considering this, I don't really see a regression here. We just need to choose a color that is not in the default palette.

Do you agree?

Holzhaus · 2019-11-03T10:35:24Z

If we keep No Color, we still need to choose a default color when we render the hotcue on screen (this is what was happening before this PR), so it can still happen that the color we choose already has a meaning for the user.

On the other hand, it could be possible that the color chosen for "No color" is unsupported on the controller used. I liked that controllers could decide which is the best way to display "No color".

However, if this makes the code significantly easier, I could also live with dropping "no color". Maybe it makes sense to replace "no color" with a color depending on the hotcue number (like the auto_hotcue_colors setting does). If we're removing uncolored hotcue, the "auto_hotcue_colors" setting should be removed as well (and make the behavior mandatory).

daschuer · 2019-11-03T12:16:09Z

The idea with transparent is that the skin color shines through. This keeps the old appearance without to much special casing.

It is fine for me to select skin independent default colour as long the can distinguish that the colour is not explicit set.

daschuer · 2019-11-03T12:17:43Z

The old idea was to just not change the colors of skin and controller comming from Mixxx 2.2. IMHO this still works.

Holzhaus · 2019-11-03T14:50:35Z

@daschuer

It is fine for me to select skin independent default colour as long the can distinguish that the colour is not explicit set.

If the user can add arbitrary colors to the palette, it would be necessary to use Null for that. Otherwise you can't distinguish if the color was set by the schema migration or if the user added the color to the palette and set it explicitly.

ferranpujolcamins · 2019-11-03T18:58:16Z

If the user can add arbitrary colors to the palette, it would be necessary to use Null for that. Otherwise you can't distinguish if the color was set by the schema migration or if the user added the color to the palette and set it explicitly.

In the new color model, it's up to the user to keep a color that means "No Color". We will provide such a color when users migrate from the old color model to the new one. It's up to the user to keep this color or not. After the migration, users will that a cue has No Color because it will have the only color that is not on the default palette.

I really don't see an alternative here. If we keep No Color, then we need to provide a default value for it. What if users then add a color to the palette that is exactly the same color we use by default?

daschuer · 2019-11-03T19:18:05Z

The idea was that all transparent colors are useless anyway. So we can use fully transparent as "No Color" that has actually "No color". Even if a user actually uses it for some reason it has still no color.

ferranpujolcamins · 2019-11-03T19:20:04Z

The idea was that all transparent colors are useless anyway. So we can use fully transparent as "No Color" that has actually "No color". Even if a user actually uses it for some reason it has still no color.

I agree. But shall we map the old No Color to transparent? Before the migration cues with No Color are visible on the waveform. After the migration they won't.

daschuer · 2019-11-03T19:23:56Z

After the migration they won't.

Not really, the skin default cue color shines through. Where we are back at the question what is default.
Did we had a problem with the old skin dependent default? If not we can stick with it.

ferranpujolcamins · 2019-11-03T19:28:36Z

Did we had a problem with the old skin dependent default?

I like the simplicity of "all cues have a color, thus there's no need to a default color".

Not really, the skin default cue color shines through.

And what about semi-transparent colors (e.g. alpha of 50%)? We merge the cue color with the skin default? Do we even want to support transparent cues at all?

My plan for the alpha value was to support it internally, but not expose it on the color picker by now, so all colors will have 100% alpha by now.

daschuer · 2019-11-03T19:39:43Z

Yes, we should reject them because there is IMHO no valid use case for them, except "No Color".

ferranpujolcamins · 2019-11-03T20:05:08Z

Thanks for your explanation. I still don't see the value though :(

From the point of view of identifying cues for which the user hasn't set a color manually, the new solution is as good as the No Color solution: they are the cues with the only color that is outside the palette.

From the point of view of user experience, I like the new approach where the cues are always rendered with the same color (independently of which skin is selected). I think it's a complication to have some cues that change color when I switch skins while other cues keep the same color. Suddenly cues that had different colors now can look very similar.
This also makes implementation logic simpler.

In the previous design our mistake was over-complication. I'm afraid what you propose may add complication, without any benefit.

Maybe @Be-ing can help us decide?

daschuer · 2019-11-03T20:44:16Z

The changing color cue is only a side effect of the proposed solution. I am happy to fix that by indicate the "old cues" alternatively.

src/test/colorconfig_test.cpp

src/library/dlgtrackinfo.h

Be-ing · 2019-11-03T22:06:00Z

res/controllers/Roland_DJ-505-scripts.js

-                if (this.colorIdKey !== undefined && outval !== this.off) {
-                    this.outputColor(engine.getValue(this.group, this.colorIdKey));
+                if (this.colorKey !== undefined && outval !== this.off) {
+                    this.outputColor(engine.getValue(this.group, this.colorKey));


I think it would be more convenient to use color.colorFromHexCode on the ControlObject value before passing it to this.outputColor.

Maybe @Holzhaus can help us here.
Can you have a look and check if I didn't break something?

This does not work. hotcue_X_color contains an actual ARGB value, not the index of the color in the color palette.

The HotcueButtons stopped working as well. Let me check what causes this.

Ok, the problem is that the colors property of HotcueButton now needs to be an object with the different colors as keys. Here's a diff:

diff --git a/res/controllers/Roland_DJ-505-scripts.js b/res/controllers/Roland_DJ-505-scripts.js index 2e2777c0e8..94dde87533 100644 --- a/res/controllers/Roland_DJ-505-scripts.js +++ b/res/controllers/Roland_DJ-505-scripts.js @@ -941,17 +941,17 @@ DJ505.PadColor = { DIM_MODIFIER: 0x10, }; -DJ505.PadColorMap = [ - DJ505.PadColor.OFF, - DJ505.PadColor.RED, - DJ505.PadColor.GREEN, - DJ505.PadColor.BLUE, - DJ505.PadColor.YELLOW, - DJ505.PadColor.CELESTE, - DJ505.PadColor.PURPLE, - DJ505.PadColor.APRICOT, - DJ505.PadColor.WHITE, -]; +DJ505.PadColorMap = { + 0: DJ505.PadColor.OFF, + 0xffc50a08: DJ505.PadColor.RED, + 0xff32be44: DJ505.PadColor.GREEN, + 0xff0044ff: DJ505.PadColor.BLUE, + 0xfff8d200: DJ505.PadColor.YELLOW, + 0xff42d4f4: DJ505.PadColor.CELESTE, + 0xffaf00cc: DJ505.PadColor.PURPLE, + 0xfffca6d7: DJ505.PadColor.APRICOT, + 0xfff2f2fe: DJ505.PadColor.WHITE, +} DJ505.PadSection = function (deck, offset) { // TODO: Add support for missing modes (flip, slicer, slicerloop) @@ -1190,7 +1190,7 @@ DJ505.HotcueMode = function (deck, offset) { this.ledControl = DJ505.PadMode.HOTCUE; this.color = DJ505.PadColor.WHITE; - var hotcueColors = [this.color].concat(DJ505.PadColorMap.slice(1)); + var hotcueColors = DJ505.PadColorMap; this.pads = new components.ComponentContainer(); for (var i = 0; i <= 7; i++) { this.pads[i] = new components.HotcueButton({ @@ -1226,7 +1226,7 @@ DJ505.CueLoopMode = function (deck, offset) { this.ledControl = DJ505.PadMode.HOTCUE; this.color = DJ505.PadColor.BLUE; - var cueloopColors = [this.color].concat(DJ505.PadColorMap.slice(1)); + var cueloopColors = DJ505.PadColorMap; this.PerformancePad = function(n) { this.midi = [0x94 + offset, 0x14 + n]; this.number = n + 1; @@ -1492,7 +1492,7 @@ DJ505.PitchPlayMode = function (deck, offset) { this.color = DJ505.PadColor.GREEN; this.cuepoint = 1; this.range = PitchPlayRange.MID; - var pitchplayColors = [this.color].concat(DJ505.PadColorMap.slice(1)); + var pitchplayColors = DJ505.PadColorMap; this.PerformancePad = function(n) { this.midi = [0x94 + offset, 0x14 + n];

src/controllers/colorjsproxy.cpp

Be-ing · 2019-11-03T22:13:16Z

src/controllers/colorjsproxy.cpp

-QScriptValue ColorJSProxy::makeHotcueColorPalette(
-        QScriptEngine* pScriptEngine, UserSettingsPointer pConfig) {
+QScriptValue ColorJSProxy::makeHotcueColorPalette(QScriptEngine* pScriptEngine,
+        HotcueColorPaletteSettings colorPaletteSettings) {


Make HotcueColorPaletteSettings::setHotcueColorPalette emit a signal, make this function a slot, and rename this function to slotColorPaletteUpdated.

Be-ing · 2019-11-03T22:45:05Z

Good work on the code @ferranpujolcamins and thank you for making small commits that are easy to review. I think we should remove the word "Hotcue" from all color related class and method names. We may want to use the color palette for other purposes in the future, for example letting the user assign a color to each track.

Could you add a way for ColorJSProxy to get the nearest color supported by a controller? I outlined a proposal for how this could work.

Changing the cue color based on the skin was reasonable before Mixxx supported colors for each individual cue. Now that Mixxx is supporting colors for each cue, I agree with @ferranpujolcamins that retaining this would overcomplicate the code and be a UX problem.

However, if this makes the code significantly easier, I could also live with dropping "no color". Maybe it makes sense to replace "no color" with a color depending on the hotcue number (like the auto_hotcue_colors setting does). If we're removing uncolored hotcue, the "auto_hotcue_colors" setting should be removed as well (and make the behavior mandatory).

Yes, I proposed this in a prior PR (I forget which one). I don't think there's a use case for this option. Instead it would be better to let the user pick a default color for each hotcue number. If a user really wants every hotcue to be one color, it would be easy to configure with this approach.

daschuer · 2019-11-03T23:02:02Z

Changing the cue color based on the skin was reasonable before Mixxx supported colors for each individual cue. Now that Mixxx is supporting colors for each cue, I agree with @ferranpujolcamins that retaining this would overcomplicate the code and be a UX problem.

This is only a detail of a possible solution for me: Keeping the default cue color from Mixxx 2.2 because we have no better one.

The crucial question is actually around the "auto_hotcue_colors" setting.

Users with this enabled want to probably just have a different number/color relationship. Here it would be helpful to just color the "no colour" cues by its number.

If we decide to make this default and remove the parameter the issue is gone.

However as far as I understand there is a second demand of users which color the cues by type. Here the colors are carrying real info. Facing hundreds of cued tracks with "no colour" cues, it should be easy to distinguish if a cue has a type, or the type is unknown. It is also IMHO not helpful to auto colour the cues by number, because it will get a random type which makes the color feature unreliable in the heat of the night. Using here "No Color" = "Type not set" fits to the use case.

I do not rally care how we show this in the GUI, using the 2.2 default color is IMHO the easiest solution, which not bothers the user at all, because by coloring a cue he moves away from the known 2.2 appearance.

Alternatively we may consider a Icon overlay for these cues or ask the user for a default color on the first start.

Ideas?

daschuer · 2019-11-03T12:20:30Z

src/library/dlgtrackinfo.cpp

+                        colorPaletteSettings.getHotcueColorPalette()
+                                .m_colorList;
+                QColor color =
+                        hotcueColorList.at(colorComboBox->currentIndex());


I think it is better to add the colour code as data (currentData()) here.

src/preferences/hotcuecolorpalettesettings.cpp

src/library/dlgtrackinfo.cpp

Be-ing · 2019-11-03T23:11:55Z

I propose removing the "Cuepoints" tab in the track properties window in this PR. It is difficult to discover, cumbersome to use, and now redundant with right clicking on cues on the overview waveform. At this point I think it is only a maintenance burden.

Be-ing · 2019-11-04T01:48:14Z

However as far as I understand there is a second demand of users which color the cues by type. Here the colors are carrying real info. Facing hundreds of cued tracks with "no colour" cues, it should be easy to distinguish if a cue has a type, or the type is unknown. It is also IMHO not helpful to auto colour the cues by number, because it will get a random type which makes the color feature unreliable in the heat of the night. Using here "No Color" = "Type not set" fits to the use case.

I think you are conflating type with color in all cases. I agree that it would be nice to have an option to color code cues by type. I think this should be independent of the cue color stored in the database. The cue type is already stored separately; there is no need to assign a null color to cues for this. When an option to color code cues by type is enabled, WaveformMark could disregard the color assigned to the cue and set the color only based on the type. This way there would be no need to change cues' stored colors and there would be no possibility for confusion mixing up cues that show an assigned color and cues that are color coded based on their type.

daschuer · 2019-11-04T07:11:09Z

I did not mean the cue type stored in the database. I mean the the type of track region the cue point is on. Something like

Blue for bridge start
Green for bridge end
Red for skip explicite lyrics
Celeste for end of build up.

If we decide to color all legacy cues red, the user cannot distinguish if is is actually an explicite lyrics marker, or another Region Importe from 2.2 or just an undefined on the fly cue point set during a set.

I would leave the coloring of hot cues entirely in the user domain and not associate it with a feature in Mixxx.

For me it is more like a lable addition.
Because you cannot see the labels on the buttons. Colors are handy.

In this sense, it would also be weird to add default labels.

... and use QColor instead.

# Conflicts: # src/util/color/predefinedcolorsset.h

and change cue color CO name # Conflicts: # res/controllers/Roland_DJ-505-scripts.js # res/controllers/midi-components-0.0.js

# Conflicts: # src/library/dlgtrackinfo.cpp

Shade still does not work

Be-ing · 2020-02-14T01:39:06Z

Thanks for working on this again.

daschuer · 2020-02-15T08:43:50Z

Due to my faulty merge, a master merge removes all changes.
Please have a look at #2398 where this is solved.

Be-ing · 2020-02-15T14:06:53Z

@daschuer I think that was fixed with a rebase. If not, I fixed that in #2399.

ywwg · 2020-02-25T18:53:00Z

Is anyone able to take over this work? @ferranpujolcamins says he doesn't have the time to finish it, and we'd really like to get this in for 2.3

Holzhaus · 2020-02-25T19:04:04Z

I'll see what I can do. Can he push his current progress? Tue Feb 25 19:53:02 GMT+01:00 2020 Owen Williams <notifications@github.com>:

…

Is anyone able to take over this work? @ferranpujolcamins [https://github.com/ferranpujolcamins] says he doesn't have the time to finish it, and we'd really like to get this in for 2.3 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [#2345?email_source=notifications&email_token=AAN74FFUZM763W2XDSOZJ6LREVSI3A5CNFSM4JIHZWXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM5BP3A#issuecomment-591009772] , or unsubscribe [https://github.com/notifications/unsubscribe-auth/AAN74FGVCTRHNNNM4F3STCDREVSI3ANCNFSM4JIHZWXA] . [https://github.com/notifications/beacon/AAN74FCO5VHICDZX55IQNB3REVSI3A5CNFSM4JIHZWXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM5BP3A.gif]

ferranpujolcamins · 2020-02-25T22:18:28Z

I don't really have any progress

Holzhaus · 2020-02-27T13:03:07Z

Superseeded by #2520.

Holzhaus requested changes Nov 2, 2019

View reviewed changes

Holzhaus mentioned this pull request Nov 2, 2019

Roland DJ-505: Add description for pad section colors #2346

Merged

ferranpujolcamins force-pushed the new_colors_impl branch from 3196155 to 223911e Compare November 3, 2019 09:18

ferranpujolcamins changed the title ~~[WIP] New colors impl~~ New colors impl Nov 3, 2019

daschuer requested changes Nov 3, 2019

View reviewed changes

Be-ing reviewed Nov 3, 2019

View reviewed changes

src/test/colorconfig_test.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Nov 3, 2019

View reviewed changes

src/library/dlgtrackinfo.h Outdated Show resolved Hide resolved

Be-ing changed the title ~~New colors impl~~ use QColor for cue colors Nov 3, 2019

Be-ing reviewed Nov 3, 2019

View reviewed changes

src/controllers/colorjsproxy.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Nov 3, 2019

View reviewed changes

daschuer reviewed Nov 3, 2019

View reviewed changes

ferranpujolcamins added 19 commits February 13, 2020 21:24

Make UserSettings available in ControllerEngine

b60f292

Remove PredefinedColors from controller scripts

37541e9

... and use QColor instead.

Remove PredefinedColor

37d5da1

# Conflicts: # src/util/color/predefinedcolorsset.h

Update midi-components library

aff6995

and change cue color CO name # Conflicts: # res/controllers/Roland_DJ-505-scripts.js # res/controllers/midi-components-0.0.js

Avoid constructing HotcueColorPaletteSettings every time its used

c756053

Change test names

40825c4

Reorder constructor parameters

b2c7c16

Rename member variable

f72a679

Rename some color related classes

1cadfea

Remove cues tab on TrackInfo

d07b672

# Conflicts: # src/library/dlgtrackinfo.cpp

Set hotcue color to the first predefined color

438b1c9

Set hotcue color CO to -1 when no hotcue is loaded

f6da0dd

Make hotcue skin buttons use the cue color

606091c

Shade still does not work

Allow skins to configure text color depending on background

9047343

Highlight hotcue buttons when hovered

ced08e2

Fix Shade hotcue buttons color

cd3555e

Make hotcue button hover highlighting configurable

525cd85

Fix auto_hotcue_colors not assigning first color

8e574d9

Merge branch 'master' into new_colors_impl

df545bf

ferranpujolcamins force-pushed the new_colors_impl branch from f557dd1 to df545bf Compare February 13, 2020 21:57

Holzhaus mentioned this pull request Feb 26, 2020

Hotcue RGB colors #2520

Merged

Holzhaus closed this Feb 27, 2020

use QColor for cue colors #2345

use QColor for cue colors #2345

Conversation

ferranpujolcamins commented Nov 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferranpujolcamins commented Nov 3, 2019

Holzhaus commented Nov 3, 2019

daschuer commented Nov 3, 2019

daschuer commented Nov 3, 2019

Holzhaus commented Nov 3, 2019

ferranpujolcamins commented Nov 3, 2019 • edited Loading

daschuer commented Nov 3, 2019

ferranpujolcamins commented Nov 3, 2019

daschuer commented Nov 3, 2019

ferranpujolcamins commented Nov 3, 2019

daschuer commented Nov 3, 2019

ferranpujolcamins commented Nov 3, 2019

daschuer commented Nov 3, 2019

Choose a reason for hiding this comment

ferranpujolcamins Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Nov 3, 2019 • edited Loading

daschuer commented Nov 3, 2019

Choose a reason for hiding this comment

Be-ing commented Nov 3, 2019

Be-ing commented Nov 4, 2019 • edited Loading

daschuer commented Nov 4, 2019

Be-ing commented Feb 14, 2020

daschuer commented Feb 15, 2020

Be-ing commented Feb 15, 2020

ywwg commented Feb 25, 2020

Holzhaus commented Feb 25, 2020 via email

ferranpujolcamins commented Feb 25, 2020

Holzhaus commented Feb 27, 2020

Holzhaus Nov 14, 2019 •

edited

Loading

ferranpujolcamins commented Nov 3, 2019 •

edited

Loading

ferranpujolcamins Nov 4, 2019 •

edited

Loading

Be-ing commented Nov 3, 2019 •

edited

Loading

Be-ing commented Nov 4, 2019 •

edited

Loading