[WIP] Enable Setting hotcue color via controlobject #1830

Swiftb0y · 2018-10-03T13:17:45Z

This is my first attempt at exposing the the color feature of the hotcues to the UI. I want to base most of the Design choices on the work done by @ferranpujolcamins (https://drive.google.com/file/d/0Bw8dE5EyzDY6bk1WemUwWENlQlE/view) and the discussions in the zulip thread (https://mixxx.zulipchat.com/#narrow/stream/109122-general/topic/Colored.20Hotcues).

make color editable via the Cuepoints panel in the Track Properties.
let woverview reflect the color changes.
add a ControlObject for the color.
make contrastLineColor of Cuemarkers adapt to CueColor
invert color of waveformrendermark label textcolor based on contrast.
Convert QString of color to predefined QCombobox with pallete.
make CueColors overwritable by the skin
add Javascript utility methods for convient interface with the CO

I want to split any further tasks (like being able to configure the hotcueproperties directly within the skin) into seperate PRs.

Swiftb0y · 2018-10-03T20:23:25Z

Correction for the commit message. The DB and the Cuepointer do update, but the UI doesn't.

daschuer · 2018-10-04T07:40:37Z

Thank you. You can correct commit messages by:

git commit --amend
git push -f

ferranpujolcamins · 2018-10-04T16:44:47Z

Thank you for adopting this! :)

Swiftb0y · 2018-10-04T17:33:46Z

You can correct commit messages by:
git commit --amend
git push -f

I know but I don't like to amend commits that I have already pushed (since its somewhat rewriting the history which never works out).

…fixed codefactor issues

Be-ing · 2018-10-08T00:13:37Z

I know but I don't like to amend commits that I have already pushed (since its somewhat rewriting the history which never works out).

Generally, it's okay to force push for a pull request. If anyone else is working together with you on the same branch, force pushing can cause hassles. Also, when you force push GitHub can't associate old comments with the specific lines of code anymore.

Be-ing · 2018-10-08T00:20:00Z

Thanks for working on improving Mixxx's cue point capabilities! IMO this is the biggest remaining weak point of Mixxx compared to other DJ software. Let's take this big project step-by-step. I think a good goal for this first pull request will be integrating color selection into the track properties dialog and using that color in the waveform marks. Creating an easier to use UI for manipulating cue points could be done in another PR after this. Before you put in the work to do that, you may want to work on https://bugs.launchpad.net/mixxx/+bug/1277689 to allow easier access to the existing track properties window.

src/widget/woverview.cpp

src/library/dlgtrackinfo.cpp

Swiftb0y · 2018-10-18T15:08:24Z

This is ready to be merged IMHO...
But I don't know why Codefactor is complaining about the public: being on a blank line. Isn't that the standard?

daschuer · 2018-10-18T15:54:31Z

The CodeFactor issue is:
"public:" should be preceded by a blank line
I think they want blocks of public and private. Somhow a kind of false positive.
So you can either add a blank line above private to silence CodeFactor or ignore it.
It does IMHO not prevent this PR from merging.

src/engine/cuecontrol.cpp

daschuer · 2018-10-18T17:02:53Z

src/engine/cuecontrol.cpp

+ // the Color is stored as a QRgb (which is just a unsigned int
+ // representation of the color (AARRGGBB)
+ m_hotcueColor = new ControlObject(keyForControl(i, "color"));
+ connect(m_hotcueColor, SIGNAL(valueChanged(double)),


I think this is a randomly CO and should not be changed directly by skin or controller scrips, right?
In this case you need to connect the valueChangeRequest() and discard changes there. Programmatically you can use setAndConfirm to bypass the request.

I just mimicked how the already existing r/w COs work. I'm trying to plan ahead for integration into skin (where writing to the CO might be useful (maybe, I don't know how such a widget would even be implemented).
And what do you mean by "randomly CO"?

~~randomly~~ read only

daschuer · 2018-10-18T23:02:32Z

Works good.
There is only an issue that the cue number can not be read with light colors.
Can you invert the font color once a certain lightness value of the cue color is passed?

daschuer · 2018-10-18T23:06:30Z

The color field in the cue table can become a combo box with for example these colors:
https://sashat.me/2017/01/11/list-of-20-simple-distinct-colors/
This prevents us that beta users clutters their library with random colors.
It would also OK for me if you change the edited Hex value to the closest palette color.

Swiftb0y · 2018-10-19T22:44:14Z

There is only an issue that the cue number can not be read with light colors.
Can you invert the font color once a certain lightness value of the cue color is passed?

I have done some tests and determined that in many circumstances, the the contrast is to low in general. The compatible colors are really depended on the context/skin in which they are displayed atleast in the overview. My current solution would be color the markers in their set color with enough contrast to the signalcolors (achieved via some borders around the marker), while still having the number within the overview in the color that is chosen by the skin (instead of the marker color) for better visibility. I have changed that the waveformrendermark to switch to a white textcolor when the color gets to dark (like you have suggested).

ronso0 · 2019-01-27T15:38:15Z

While I'm playing with the colors and looking at the overview, I think we should reduce the palette and 'merge' similar colors:

Blue, Cyan, ~~Navy~~, ~~Teal~~
~~Pink~~, Lavender, ~~Apricot~~, Beige
Purple, Magenta
Green, ~~Olive~~, Teal
Yellow & Lime also very close, but both nice colors

ferranpujolcamins · 2019-01-27T15:39:21Z

Agree, I've chosen
Red,
Green,
Yellow,
Blue,
Cyan,
Magenta,
Pink,
Teal,
Grey,

Would this work for you?

daschuer · 2019-01-27T15:40:35Z

So we have "no color" for all existing cues set by the skin and override able by the user. I like that idea.

This will also help to adjust all existing cues to the schema in the users mind. We can't predict the users meaning of default red.

Swiftb0y · 2019-01-27T15:40:38Z

How about white instead of grey?

ferranpujolcamins · 2019-01-27T15:44:30Z

This will also help to adjust all existing cues to the schema in the users mind. We can't predict the users meaning of default red.

I'm sold with this argument :)
I'll try to implement it this way.

How about white instead of grey?

I'd avoid white because it won't work very well with a white skin. Like this one we used to have.
https://lh6.googleusercontent.com/-YATHf2XGrD0/T1YyRukZARI/AAAAAAAAAB4/q3uV4zGcLS0/s640/Mixxx1.10-Phoney.png

ronso0 · 2019-01-27T15:44:39Z

How about white instead of grey?

well, I'm reluctant to drop neon green in LateNight.. IMO that's an essential indicator color there

daschuer · 2019-01-27T15:45:26Z

I would prefere to use rather more usable colours then less.

daschuer · 2019-01-27T15:45:50Z

I would prefere to use rather more usable colours then less.

daschuer · 2019-01-27T15:46:57Z

Just because we should be prepared to import competitors colours. The user can finally decide if a colour is useful.

daschuer · 2019-01-27T15:47:37Z

Sorry for the close cycle. I hate this button.

ronso0 · 2019-01-27T15:48:01Z

Just because we should be prepared to import competitors colours. The user can finally decide if a colour is useful.

good point (although I have no idea how competitors handles this)

ferranpujolcamins · 2019-01-27T15:48:34Z

The problem is that we must only offer colors that can be distinguishable. The current list has colors that are too similar, and also white and black which won't render nice in all skins.

Swiftb0y · 2019-01-27T15:48:37Z

I'd avoid white because it won't work very well with a white skin.

You're right. Then white could also be used as a reserved controller color (eg. to highlight a specific hotcue) which could be useful.

ronso0 · 2019-01-27T15:50:09Z

Sorry for the close cycle. I hat this button.

and afaik there's no way to report that issue to github itself, except the contact form.
I use github-dark theme in ff, and there it's easy to just remove that button.

ferranpujolcamins · 2019-01-29T20:11:23Z

There's one issue if we change the default cue colors in the future:

User sets blue color to a hotcue. #4363D8 is stored in database (the default blue hex code)
In a future release we change the default blue color to #0000FF
User loads the track in the new version. Mixxx tries to find which default color has code #4363D8.
Since no default color has such code, the hotcue gets assigned the Invalid color.

This will happen for all blue hotcues.

Ways to fix this:

If we change a default color, we need to add a new DB scheme version and change the color code for all affected cues.
Store the default color name in DB instead of the Hex code. We don't allow custom colors anyway, so why store the HEX code?

@daschuer Thoughts?

Swiftb0y · 2019-01-30T18:02:33Z

Would a relational database solve the problem?
I mean if you had a table for the cuecolors within the DB (not the C++ source) you could also allow the user to edit that table (to add more colors/change colors for example). The tracks themselves would then just store a reference to the color in the dedicated color table. Skins could then just define/populate their own colortable variants and mixxx would access the right color table depending on the current skin.
This is just how I would do it when I were to develop something in PHP so this might not be the right approach in our case.

ferranpujolcamins · 2019-01-30T18:33:08Z

@Swiftb0y Thanks for the suggestion. Yes that's pretty much my proposal, but without storing to the DB, just reference color objects in code.

Swiftb0y · 2019-01-30T20:38:05Z

Well I thought that the colors in the database would allow for more flexibility in the long run. I am not that set on preventing users completely from adding custom cue colors. Would it be possible to integrate that similar to how skins are able to override colors? Just with the exception that the user colors are stored in a separate table instead of an/(the skins) XML file?

Store the default color name in DB instead of the Hex code. We don't allow custom colors anyway, so why store the HEX code?

I don't necessarily want to restart the discussion again whether we should allow custom colors but IMO it would be feasible than before if we stored the colors as IDs instead of their absolute value.

ferranpujolcamins · 2019-01-30T20:44:29Z

Yes, in a future we might allow custom colors. But right now we are just implementing predefined colors. One step at a time :)

Store the default color name in DB instead of the Hex code. We don't allow custom colors anyway, so why store the HEX code?

I ended up implementing this solution, but storing a color id instead of name. Thus we will be able to change both the HEX code and the name of a color safely.

From the controller scripts point of view, the experience is pretty much the same. Before we needed to call a function to decode the CO value into an usable RGB color. Now you will be able to call a function to get an RGB color from the id. Same thing.

Commits coming soon

Swiftb0y · 2019-01-30T20:48:26Z

but storing a color id instead of name.

That was my Idea as well as it seems more flexible.

Commits coming soon

I'm very hyped. Thank you continuing what I started (even though I suspect that there won't be much of my code left once you're done ;))

ferranpujolcamins · 2019-01-30T20:54:02Z

That was my Idea as well as it seems more flexible.

Thank you for the hint ;)

I'm very hyped. Thank you continuing what I started (even though I suspect that there won't be much of my code left once you're done ;))

I've heavily refactored color.h. It was a mess (my fault, I added too much things there).
The brightness function is very useful and it's still there ;). I've changed some of the accompanying functions. You prepared everything so we didn't need to calculate the brightness of a color all the time but just once. However, we were actually calculating it all the time. I fixed that :)

ferranpujolcamins · 2019-01-30T21:07:59Z

I've opened #2016.
Shall we close this PR?

Swiftb0y · 2019-01-30T21:10:43Z

definitely.

added color column to properties dlg but UI and DB dont update

93c0451

Swiftb0y added 3 commits October 4, 2018 19:53

fixed functionality broken by previous commit.

e0d73a0

woverview waveformsmarks will now reflect their hotcue color

29df355

markproperties.m_textColor is now the same color as the mark itself; …

68efc42

…fixed codefactor issues

uklotzde reviewed Oct 8, 2018

View reviewed changes

src/widget/woverview.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Oct 8, 2018

View reviewed changes

src/widget/woverview.cpp Outdated Show resolved Hide resolved

Swiftb0y changed the title ~~[WIP] integrate setting of hotcue color and label into UI/Skins~~ [WIP] Enable Setting hotcue color via controlobject Oct 11, 2018

daschuer reviewed Oct 12, 2018

View reviewed changes

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

Swiftb0y added 2 commits October 13, 2018 15:54

followed daschauers request to VERIFY_OR_DEBUG_ASSERT in a case

92d381d

followed uklotzde change requests in widget/woverview

d8057bd

daschuer reviewed Oct 13, 2018

View reviewed changes

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

Swiftb0y added 4 commits October 14, 2018 15:50

fixed redundant blank line (codefactor complained)

d4e0735

fixed inverted VERIFY_OR_DEBUG Statement

2c253ff

Added R/W ControlObject for the HotcueColor

f57e682

contrastLineColor of Markers now adapt to MarkerColor Brightness

b0270ba

daschuer reviewed Oct 18, 2018

View reviewed changes

Removed Comment in cuecontrol.cpp

9833f67

Swiftb0y added 2 commits October 20, 2018 00:50

the marker number color is now set by the skin again, not the m_color

d52f107

moved brightness function to color.h; rendermark textColor adapts now

c8c4150

daschuer closed this Jan 27, 2019

daschuer reopened this Jan 27, 2019

ferranpujolcamins mentioned this pull request Jan 30, 2019

Enable setting hotcue color via controlobject #2016

Merged

7 tasks

Swiftb0y closed this Jan 30, 2019

Be-ing mentioned this pull request Oct 14, 2019

Rekordbox removable device library feature #2119

Merged

mixxxbot mentioned this pull request Aug 22, 2022

Per cue custom color #7575

Closed

[WIP] Enable Setting hotcue color via controlobject #1830

[WIP] Enable Setting hotcue color via controlobject #1830

Conversation

Swiftb0y commented Oct 3, 2018 • edited Loading

Swiftb0y commented Oct 3, 2018

daschuer commented Oct 4, 2018 • edited Loading

ferranpujolcamins commented Oct 4, 2018

Swiftb0y commented Oct 4, 2018 • edited Loading

Be-ing commented Oct 8, 2018 • edited Loading

Be-ing commented Oct 8, 2018

Swiftb0y commented Oct 18, 2018

daschuer commented Oct 18, 2018

daschuer Oct 18, 2018

Choose a reason for hiding this comment

Swiftb0y Oct 19, 2018

Choose a reason for hiding this comment

daschuer Dec 28, 2018

Choose a reason for hiding this comment

daschuer commented Oct 18, 2018

daschuer commented Oct 18, 2018

Swiftb0y commented Oct 19, 2018

ronso0 commented Jan 27, 2019

ferranpujolcamins commented Jan 27, 2019

daschuer commented Jan 27, 2019

Swiftb0y commented Jan 27, 2019

ferranpujolcamins commented Jan 27, 2019 • edited Loading

ronso0 commented Jan 27, 2019

daschuer commented Jan 27, 2019

daschuer commented Jan 27, 2019

daschuer commented Jan 27, 2019

daschuer commented Jan 27, 2019 • edited Loading

ronso0 commented Jan 27, 2019

ferranpujolcamins commented Jan 27, 2019

Swiftb0y commented Jan 27, 2019

ronso0 commented Jan 27, 2019

ferranpujolcamins commented Jan 29, 2019

Swiftb0y commented Jan 30, 2019

ferranpujolcamins commented Jan 30, 2019

Swiftb0y commented Jan 30, 2019

ferranpujolcamins commented Jan 30, 2019

Swiftb0y commented Jan 30, 2019

ferranpujolcamins commented Jan 30, 2019

ferranpujolcamins commented Jan 30, 2019

Swiftb0y commented Jan 30, 2019

Swiftb0y commented Oct 3, 2018 •

edited

Loading

daschuer commented Oct 4, 2018 •

edited

Loading

Swiftb0y commented Oct 4, 2018 •

edited

Loading

Be-ing commented Oct 8, 2018 •

edited

Loading

ferranpujolcamins commented Jan 27, 2019 •

edited

Loading

daschuer commented Jan 27, 2019 •

edited

Loading