feat: static color coding for key column #13390

danferns · 2024-06-19T10:27:43Z

Hi! I just got the core of this feature working and I'm seeking feedback.

A small rectangle is drawn next to the Key label in the Library. The color of the rectangle is based on the Key of the track.

I'm concerned about the method used for finding the track's key. Right now, I'm using KeyUtils::guessKeyFromText on the Key string, but I don't think that'll work if the user sets custom Key Notation.

I want to use the value in ColumnCache::COLUMN_LIBRARYTABLE_KEY_ID. But I haven't been able to figure out how to access this column from the QModelIndex& index that is passed into KeyDelegate::paintItem. I'm not sure if the column is even available.

Is there a place where I can see how the index is structured?

Once complete, this will resolve #7810.

Swiftb0y · 2024-06-19T10:53:49Z

Once complete, this will resolve #7810.

I don't think so. That issue requests to color the key depending on how compatible it is to the currently playing track.

src/library/tabledelegates/keydelegate.cpp

src/library/tabledelegates/keydelegate.h

src/library/tabledelegates/keydelegate.cpp

Swiftb0y · 2024-06-19T10:59:43Z

src/util/color/predefinedcolorpalettes.cpp

+constexpr mixxx::RgbColor kMixxxKeyColor1(0xFC4949);
+constexpr mixxx::RgbColor kMixxxKeyColor2(0xFE642D);
+constexpr mixxx::RgbColor kMixxxKeyColor3(0xF98C27);
+constexpr mixxx::RgbColor kMixxxKeyColor4(0xFED600);
+constexpr mixxx::RgbColor kMixxxKeyColor5(0x99FE00);
+constexpr mixxx::RgbColor kMixxxKeyColor6(0x42FE3E);
+constexpr mixxx::RgbColor kMixxxKeyColor7(0x0AD58F);
+constexpr mixxx::RgbColor kMixxxKeyColor8(0x0AE7E7);
+constexpr mixxx::RgbColor kMixxxKeyColor9(0x04C9FE);
+constexpr mixxx::RgbColor kMixxxKeyColor10(0x3D8AFD);
+constexpr mixxx::RgbColor kMixxxKeyColor11(0xAC64FE);
+constexpr mixxx::RgbColor kMixxxKeyColor12(0xFD3FEA);
+


as a follow up, it would be nice if the palette was customizable via the Color Preferences. similar to track colors.

Super nice to have, follow up²: set the colors in the notation table in Key preferences

Yeah, Though I guess we'll have to decide on whether its better to put in the "Colors" or the "Key Detection" settings. The former would be more consistent IMO.

Here's a little sketch:

I'd somehow try to put it in the same column. but I'm not a UI designer. Wdyt @ronso0?

I think it looks more compact:

maybe swap the order of the text and the check box so its all still right aligned? idk, IIANA Designer.

Swiftb0y · 2024-06-19T11:03:48Z

I'm concerned about the method used for finding the track's key. Right now, I'm using KeyUtils::guessKeyFromText on the Key string, but I don't think that'll work if the user sets custom Key Notation.

Probably not. You should store the underlying mixxx::track::io::key::ChromaticKey in the cell when its text gets set. You can use a custom item role for that (I think, I'm not much into Qt MV programming tbh).

danferns · 2024-06-19T13:32:42Z

Thank you so much @Swiftb0y for the feedback! Preferences for key color palettes is definitely in the plans. I will apply the requested changes and try the item role method.

src/library/tabledelegates/keydelegate.cpp

ronso0

Thank you, this looks nice!

src/library/tabledelegates/keydelegate.cpp

src/library/basetracktablemodel.cpp

src/library/tabledelegates/keydelegate.cpp

src/track/keyutils.cpp

.

src/library/tabledelegates/keydelegate.cpp

daschuer · 2024-07-01T06:40:25Z

Can you update the screenshot in the initial description of this PR? This way it is more significant if one is later following links from the Changelog.

daschuer · 2024-07-01T06:41:05Z

Is the draft state still correct or is it ready for final review and tests?

daschuer

Just a minor comment. Rest LGTM, thank you.

src/library/basetracktablemodel.cpp

daschuer · 2024-07-01T06:52:57Z

src/util/color/predefinedcolorpalettes.cpp

@@ -144,6 +144,21 @@ constexpr mixxx::RgbColor kVirtualDJTrackColorBlue(0x0000FF);
 constexpr mixxx::RgbColor kVirtualDJTrackColorFuchsia(0xFF00FF);
 constexpr mixxx::RgbColor kVirtualDJTrackColorWhite(0xFFFFFF);

+// Default Mixxx Key Color Palette


Please give also a hint as source code comment, how these colors have been generated

I think it is a comon practice to follow the Hue circle to easily distinguish neighbors.
However there might be issues with all kind of eye issues.

https://www-datylon-com.cdn.ampproject.org/v/s/www.datylon.com/blog/data-visualization-for-colorblind-readers?amp_gsa=1&amp_js_v=a9&hs_amp=true&usqp=mq331AQIUAKwASCAAgM%3D#amp_tf=Von%20%251%24s&aoh=17198172132733&csi=1&referrer=https%3A%2F%2Fwww.google.com&ampshare=https%3A%2F%2Fwww.datylon.com%2Fblog%2Fdata-visualization-for-colorblind-readers

Interesting are the hue curves of different color blind people. So I guess in a follow up PR, we may offer the full hue curve and reduced versions for color blind people that is only using half of the hue range.

We could provide preset palettes optimized for the different types of colorblindness. I think gradients for those already exist online. This would be fairly straightforward to implement once the requested custom keypalettes are in place.

danferns · 2024-07-01T07:56:27Z

Is the draft state still correct or is it ready for final review and tests?

Yes, I am not done yet. I still have to read the Key Colors setting and only render the rectangle if it is enabled.

Also I'd like to squash a couple of commits before review, at least the one which was failing to build.

danferns · 2024-07-01T09:49:49Z

I want to read the setting using ConfigKey("[Config]", "KeyColorsEnabled") inside KeyUtils::keyToColor (if key colors are disabled, return an invalid color). But to do that, I would need access to the pConfig variable inside keyutils.cpp.

I see that the pConfig is passed around in class constructors. Is there a way to access it directly?

Should I go about this in some other way entirely?

Swiftb0y · 2024-07-01T10:39:11Z

I don't think accessing the config in the KeyUtils is a good idea. Can you share a little more insight on what you're trying to accomplish? Then we can brainstorm together.

danferns · 2024-07-01T11:48:51Z

Can you share a little more insight on what you're trying to accomplish?

I have added a preference setting to enable/disable Key Colors. It is called KeyColorsEnabled and I put it under the [Config] group. It is defined inside ColorPaletteSettings.

Now, I want to display the key colors only when this setting is on.

Initially, I was trying to do this preference check inside KeyDelegate. But then I noticed that the KeyUtils::keyToString function is already aware of the user's chosen notation (it doesn't need to be passed in explicitly). So I thought maybe KeyUtils::keyToColor should also do the same, and access the user settings on it's own.

Should the check instead be done inside KeyDelegate?

Swiftb0y

LGTM. Thank you. Who wants to press the button? Did you want to rebase first?

danferns · 2024-07-12T05:34:11Z

Thank you so much, this is very helpful. I'll discuss with Daniel for the next steps, and I'll keep these points in mind for future PRs. I am growing a lot thanks to this project. :)

I do want to rebase!

daschuer · 2024-07-12T06:31:56Z

Thank you for the review. Yes, there is room for improvements. On the other hand we have tight plan for the remaining project. From this point of view I would rather see a beautiful Heat column instead of beautiful code.

We have already the green check marks so I will merge once @danferns gives a go.

If you feeling lucky to address some of @Swiftb0y points, go ahead and propose it in a separate PR.

also check bounds for Open Key number inside it. Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

This will allow for narrow column setups.

Co-authored-by: ronso0 <ronso0@mixxx.org>

Add a `KeyColorsEnabled` setting under the `[Config]` group. And render a checkbox for it in the Color preferences.

Co-authored-by: ronso0 <ronso0@mixxx.org>

also add checkbox to <tabstops> Co-authored-by: ronso0 <ronso0@mixxx.org>

danferns · 2024-07-12T08:08:50Z

I'm done with the rebase! I squashed only a build error, and two commits involving indentation / comments. If anything else is needed then let me know, but otherwise I am ready for this PR to be merged!

Swiftb0y · 2024-07-12T09:53:46Z

On the other hand we have tight plan for the remaining project. From this point of view I would rather see a beautiful Heat column instead of beautiful code.

I agree in this case.

daschuer · 2024-07-13T06:16:36Z

Thank you very much! This looks really nice and works good.

acolombier · 2024-07-14T10:29:20Z

After synching my dev branch, I just seen that eye candy appeared. It looks very good! Thanks for that feature!

Eve00000 · 2024-07-14T12:28:50Z

I tested this today, some remarks:

I don't have a key palette under 'Enable Key Colors' in Preferences -> Colors
-Pushing 'Apply' after changing 'Enable Key Colors' does not work, changes only happen after push on 'OK'
Nice color bar in 'Key'-column but Key text is cut off on the left side (normally text is cut of on the right side) while my most important part is on the left side.

ronso0 · 2024-07-14T13:42:23Z

I don't have a key palette under 'Enable Key Colors' in Preferences -> Colors

There is only one palette atm.

Repaint with/without colors only happens when the table is repainted after the preferences are closed.

danferns · 2024-07-15T13:00:01Z

but Key text is cut off on the left side (normally text is cut of on the right side)

Thank you for reporting this. Just fixed it, a PR will come soon!

danferns added 4 commits June 18, 2024 16:21

add KeyDelegate

50a9767

add Default Key Color Palette

ddff4a9

use Key Color Palette to change Key Column background

65bb7bd

implement the design for Key Column colors

680c86f

github-actions bot added library build labels Jun 19, 2024

style: increase right padding for key color rectangle

dc9a32f

Swiftb0y reviewed Jun 19, 2024

View reviewed changes

ronso0 reviewed Jun 19, 2024

View reviewed changes

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

ronso0 reviewed Jun 19, 2024

View reviewed changes

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

ronso0 previously requested changes Jun 19, 2024

View reviewed changes

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

daschuer requested changes Jun 20, 2024

View reviewed changes

src/library/basetracktablemodel.cpp Show resolved Hide resolved

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

danferns added 2 commits June 26, 2024 18:59

refactor: cleanup #includes and move KeyDelegate constructor into header

43947ec

fix: use selected text color in KeyDelegate

1f33dd4

daschuer reviewed Jun 26, 2024

View reviewed changes

src/track/keyutils.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jun 26, 2024

View reviewed changes

src/track/keyutils.cpp Outdated Show resolved Hide resolved

ronso0 reviewed Jun 27, 2024

View reviewed changes

src/library/tabledelegates/keydelegate.cpp Outdated Show resolved Hide resolved

github-actions bot added ui preferences labels Jun 29, 2024

daschuer approved these changes Jul 1, 2024

View reviewed changes

Swiftb0y approved these changes Jul 11, 2024

View reviewed changes

danferns and others added 16 commits July 12, 2024 12:59

move keyToColor translation into KeyUtils

1baeefc

also check bounds for Open Key number inside it. Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>

style: remove left padding of key color bars

89b0744

This will allow for narrow column setups.

refactor: pass KEY_ID to KeyDelegate, instead of Key String

db2ed11

Co-authored-by: ronso0 <ronso0@mixxx.org>

add checkbox to toggle key colors

e0fbe90

Add a `KeyColorsEnabled` setting under the `[Config]` group. And render a checkbox for it in the Color preferences.

feat: toggle key colors from preferences

8d039c4

fix: checkbox ui not in sync with key colors setting

4535b5a

refactor: remove redundant canConvert assertion

3c10659

mention source of Mixxx Key Color Palette colors

a79c571

fix indentation and set colspan in Color Preferences UI

9a903a7

remove unnecessary slotUpdate() call

82c9c7b

refactor: set keyColor, keyText as const

e1c07e8

Co-authored-by: ronso0 <ronso0@mixxx.org>

refactor: remove KeyUtils scope used in keyToColor

ca6d9c1

Co-authored-by: ronso0 <ronso0@mixxx.org>

refactor: update key colors checkbox name

af87ba6

also add checkbox to <tabstops> Co-authored-by: ronso0 <ronso0@mixxx.org>

clear keyText when key is invalid

69f3a92

refactor: draw key text/color only if key is valid

7c9e602

remove #define KEY_COLORS_ENABLED

f75ec58

danferns force-pushed the static_key_colors branch from 468c9be to f75ec58 Compare July 12, 2024 08:03

daschuer merged commit 1b9ebc1 into mixxxdj:main Jul 13, 2024
13 checks passed

danferns mentioned this pull request Jul 15, 2024

fix: Key text is elided from left, should be right #13475

Merged

danferns mentioned this pull request Jul 22, 2024

Add Key Color Palettes #13497

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: static color coding for key column #13390

feat: static color coding for key column #13390

danferns commented Jun 19, 2024 •

edited

Loading

Swiftb0y commented Jun 19, 2024

Swiftb0y Jun 19, 2024

ronso0 Jun 19, 2024

Swiftb0y Jun 26, 2024 •

edited

Loading

danferns Jun 29, 2024

Swiftb0y Jun 29, 2024

danferns Jun 29, 2024

Swiftb0y Jun 29, 2024

Swiftb0y commented Jun 19, 2024

danferns commented Jun 19, 2024

ronso0 left a comment

daschuer commented Jul 1, 2024

daschuer commented Jul 1, 2024

daschuer left a comment

daschuer Jul 1, 2024

daschuer Jul 1, 2024

daschuer Jul 1, 2024

Swiftb0y Jul 1, 2024 •

edited

Loading

danferns commented Jul 1, 2024

danferns commented Jul 1, 2024

Swiftb0y commented Jul 1, 2024

danferns commented Jul 1, 2024

Swiftb0y left a comment

danferns commented Jul 12, 2024

daschuer commented Jul 12, 2024

danferns commented Jul 12, 2024

Swiftb0y commented Jul 12, 2024

daschuer commented Jul 13, 2024

acolombier commented Jul 14, 2024

Eve00000 commented Jul 14, 2024 •

edited

Loading

ronso0 commented Jul 14, 2024

danferns commented Jul 15, 2024

feat: static color coding for key column #13390

feat: static color coding for key column #13390

Conversation

danferns commented Jun 19, 2024 • edited Loading

Swiftb0y commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Jun 26, 2024 • 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

Swiftb0y commented Jun 19, 2024

danferns commented Jun 19, 2024

ronso0 left a comment

Choose a reason for hiding this comment

daschuer commented Jul 1, 2024

daschuer commented Jul 1, 2024

daschuer left a 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

Swiftb0y Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

danferns commented Jul 1, 2024

danferns commented Jul 1, 2024

Swiftb0y commented Jul 1, 2024

danferns commented Jul 1, 2024

Swiftb0y left a comment

Choose a reason for hiding this comment

danferns commented Jul 12, 2024

daschuer commented Jul 12, 2024

danferns commented Jul 12, 2024

Swiftb0y commented Jul 12, 2024

daschuer commented Jul 13, 2024

acolombier commented Jul 14, 2024

Eve00000 commented Jul 14, 2024 • edited Loading

ronso0 commented Jul 14, 2024

danferns commented Jul 15, 2024

danferns commented Jun 19, 2024 •

edited

Loading

Swiftb0y Jun 26, 2024 •

edited

Loading

Swiftb0y Jul 1, 2024 •

edited

Loading

Eve00000 commented Jul 14, 2024 •

edited

Loading