Color preferences improvements #2589

daschuer · 2020-03-22T18:08:34Z

This improved the color preferences a bit. See:

daschuer · 2020-03-22T19:40:05Z

I would like to change this even more, to improve the workflow editing the palette.

Normally, the Palette Editor should be hidden. When pressing edit, the palette editor is shown. With the Palette selected for Tracks or Cues. The name is appended with "(Edited)"
There will be a "Save" and a "Remove" button.
When pressing save, the palette is stored with "Name" and automatically selected for the for cues or tracks where Edit... was pressed

Saving a palette with a name of a build in palette is not possible. We may pop up a dialog or create a unique automatically.

What do you think?

Holzhaus

Thanks for working on UI polish ;-)

This mostly looks good. The button placement is probably a matter of preference but I'm okay with that. I'm unsure about the hotcue index changes (see my inline comment).

Also, maybe we need another button in place of the Discard button. In your screenshot it looks like "Discard" is translated to "Schließen ohne zu Speichern" ("Close without Saving") for German users. This is unfortunate, but I guess we can't change it to "Verwerfen" ("Discard"), right? Because we don't actually close without saving - instead we delete the currently selected palette.

Holzhaus · 2020-03-22T19:41:16Z

src/preferences/dialog/dlgprefcolorsdlg.ui

@@ -62,7 +76,7 @@
         <string>Automatically assigns a predefined color to a newly created hotcue point, based on its index.</string>
        </property>
        <property name="text">
-         <string>Assign predefined colors to newly created hotcue points</string>
+         <string>Select consecutive palette colors for new hotcues</string>


The palette colors aren't necessarily consecutive as they can be in any order depending on the "Assign to Hotcue Index" column. However, I'd suggest we remove this option completely - see PR #2578,

I think it is very usable to have this option separate from the palette editor.
Many users probably fine with the Mixxx colors and only like to select the default color or the color by number option.
So I actually want the combobox back where we can select one or all colors from the current palette.
I think you are used to the color by number feature, right?
I like to have one default color. Currently the later is hidden and therefore unpredictable.
We need to fix that as well.
Would it OK for you to bring that box back?

Holzhaus · 2020-03-22T19:46:12Z

src/preferences/colorpaletteeditormodel.cpp

+        }
+    } else {
+        for (int i = 0; i < palette.size(); i++) {
+            hotcueColorIndicesMap.insert(i, i);


Now it's a bit cumbersome so set a single default color for a large palette because you'd have to remove all the other numbers in the index column by hand. It's also a bit confusing because we now have numbers for the built-in track color palettes, too. The previous situation was probably also confusing, so I'm not sure which one is better. If we keep this change I think we need a context menu item to unset all indices in the palette editor.

The idea was to have always the coloumn populated with the cue numbers and enable the use via the box.
So you only need to edit the column if you are not fine with this. This should effect virtually no one, isn't it?

Let's say I have a palette with 10 colors. One of the colors is red. If I want all my new cues red, I have to set "assign to hotcue number" to 1 for red and an empty value for all others. Before, I could just double click next to the red color and type 1. Done.

Now I have to the the same and then delete the numbers from all other rows.

I think this should be move into the paletteditor(model) classes. If there are no assigned hotcue indices, we could display grey placeholder values in the "assign to hot cue numbers" table (the placeholder values are not saved) . As soon as you set one value eyplicitly, the placeholder values should disappear.

Before, I could just double click next to the red color and type 1. Done.

Interesting. That was not self explaining to me. I have actually tried to add all cue numbers to red like 1, 2, 3 ... but it fails.

A combobox with all palette colors + color by number for default cue color will be IMHO quite easy to understand compared to that.

I like to avoid to bothering the user with the palette editor if he just want to select the default color.

Does this work for you as well?

I agree that setting the default color via the palette editor causes too much friction, however let's keep this out of this PR as I have an idea for that that I'd like to try in a separate PR after this one has been merged.

I think this should be move into the paletteditor(model) classes. If there are no assigned hotcue indices, we could display grey placeholder values in the "assign to hot cue numbers" table (the placeholder values are not saved) . As soon as you set one value explicitly, the placeholder values should disappear.

Can you implement this? The current change also prevent reordering the palette with no indices set. Also, palette could be big so you should check if i is less than the number of hotcues.

ronso0 · 2020-03-22T19:54:03Z

Normally, the Palette Editor should be hidden.

Why? It shows a preview of the palette.

daschuer · 2020-03-22T20:05:07Z

Normally, the Palette Editor should be hidden.

Why? It shows a preview of the palette.

Because it's editable style is overwhelming.
Editing the palette is a very rare edge use case, that should not be that prominent.

Instead it would be better to show palette as it is shown in the color selector from the skin.
That is finally what the most users want to do here.

Instead of a combo box for selecting the palette, we can pup up a menu with all palettes shown as colors. That's way the user can compare the palettes and pick the best suitable.

ronso0 · 2020-03-22T20:13:20Z

Instead it would be better to show palette as it is shown in the color selector from the skin.
That is finally what the most users want to do here.

Instead of a combo box for selecting the palette, we can pup up a menu with all palettes shown as colors. That's way the user can compare the palettes and pick the best suitable.

👍

Be-ing · 2020-03-23T18:54:04Z

Maybe we should make the default color palettes immutable and create a new custom palette when the user saves any edit?

Holzhaus · 2020-03-23T19:09:00Z

Maybe we should make the default color palettes immutable and create a new custom palette when the user saves any edit?

The default color palette is immutable. If you change anything you need to enter a new name in the combo box before you can click save.

Be-ing · 2020-03-24T02:05:36Z

Instead it would be better to show palette as it is shown in the color selector from the skin.
That is finally what the most users want to do here.

I agree. I find the implementation of the palette editor as a table overcomplicated and I think it takes much more screen space than necessary. It would be more straightforward to show a grid of colors like the cue menu. I don't understand why there is a column for assigning colors to hotcues. I would expect that the hotcue number a color is assigned to is the same as its position in the palette list. What is the need for this extra layer of indirection?

daschuer · 2020-03-24T14:57:52Z

Now it looks like this:

daschuer · 2020-03-24T15:02:37Z

I think it is a good step forward to merge it as it is.
We may consider the palette editor usability and the remove of "[Controls]", "auto_hotcue_colors" in a follow up PR.
However this is not critical for a release IMHO. The mass changing of orange to something else should however be included.
Maybe we can focus on this?

Repopulate the default color selector when changing the palette.

Holzhaus · 2020-03-24T15:46:39Z

I think it is a good step forward to merge it as it is.

Yup, thank you. The palette previews are very nice. Can you also set a miniature version as icon for the combobox items? Not necessary for merge, but would look nice and users wouldn't need to select each palette individually to see the colors.

Also, can you please implement the placeholder-style hotcue indices or just revert the auto-setting of hotcue indices before merge?

The mass changing of orange to something else should however be included.
Maybe we can focus on this?

We can, but as I said on #2547 I need help with the DB caching issues. If nobody figures out what causes them I cannot continue working on this. I already spent hours looking at the code but with my lack of the DB caching internals I can't spot the issue.

Holzhaus · 2020-03-24T15:55:17Z

src/preferences/dialog/dlgprefcolors.cpp

+    comboBoxHotcueDefaultColor->setItemIcon(0, QIcon(pixmap));
+
+    for (int i = 0; i < hotcuePalette.size(); ++i) {
+        comboBoxHotcueDefaultColor->addItem(tr("Palette") +


"Palette N" is not the right term, why no use something like "Color N: #FF8000"?

src/preferences/dialog/dlgprefcolors.cpp

Be-ing · 2020-03-24T17:44:11Z

src/preferences/dialog/dlgprefcolors.cpp

+    }
+
+    bool autoHotcueColors =
+            m_pConfig->getValue(ConfigKey("[Controls]", "auto_hotcue_colors"), false);


How about we change the name of this ConfigKey to hotcue_colors_by_number? The current name is unclear. I don't think we should worry about breaking configurations considering we haven't released a beta yet and it's easy to change this.

In this case, I think we should get entirely rid of it.

Then how do you propose to keep track of whether to color hotcues by number or not? I suppose we could use a special value like -1 for HotcueDefaultColorIndex, but I think it's more appropriate to keep a separate boolean ConfigKey.

This should be done after we have considered @Holzhaus plans in a follow up PR.

I don't understand what you mean. What would be done in that follow up PR? Are you talking about #2578? I think that PR is made obsolete by this one and we should close #2578 without merging.

I'd prefer to switch the name of a ConfigKey now than during beta.

@daschuer what do you think?

The current name is not that bad IMHO
I like to keep it either as it is to not break existing configurations or dispose this parameter entirely and reflect the selected combobox value in one parameter.

I do think auto_hotcue_colors is not a good name. On its own I have no idea what it means. hotcue_colors_by_number would make the code easier to understand. However, I'm not going to insist on blocking this PR over this.

daschuer · 2020-03-30T21:24:59Z

Done.

Holzhaus · 2020-03-30T23:20:13Z

Nice. I think you forgot to commit or push your button reordering though. This is how it looks for me on 4b3f9b4:

Holzhaus · 2020-03-30T23:23:31Z

Another very minor nitpick: Can we use the list-add and list-remove icons instead of a regular "+" and "-"? If the icon is not available can can fall back to them.
https://specifications.freedesktop.org/icon-naming-spec/latest/ar01s04.html

daschuer · 2020-03-30T23:30:19Z

I think we see now the behavior of different OSs wen using the QButtonBar.
Ubuntu for instsnce does not show the icons on the buttons.
I also wonder why you see discard, with a paper bin. This does not make sense at all in our context. Or does it?

Can you please Provide the suggested changes as a PR. Since I am not able to test it.

Be-ing · 2020-03-30T23:35:22Z

Huh... it looks different from both of you for me using GNOME:

src/preferences/colorpaletteeditor.cpp

ywwg · 2020-03-31T14:43:29Z

This is looking really good! Can you reverse the + and -? That's what the usual UX convention is

Be-ing

Add documentation for recently added classes

src/preferences/colorpaletteeditor.h

src/preferences/colorpaletteeditormodel.cpp

src/preferences/colorpalettesettings.h

src/util/color/colorpalette.h

daschuer · 2020-03-31T21:00:23Z

Done

Holzhaus

Looks good. I think this is pretty nice now.

I have some additional small changes in mind, but these can also go in a separate PR:

Use icons for +/- buttons if available
Add a Delete Icon to the "Remove Palette" button if available (currently it looks strange if the other buttons all have an icon, but this button does not)
Possibly replace the QDialogButtonBox with a HBoxLayout to get a consistent button order across all OSes (if desired)

Holzhaus · 2020-04-02T13:14:57Z

src/preferences/dialog/dlgprefcolors.cpp

+    if (paletteName == palette.getName()) {
+        for (int i = 0; i < palette.size() && i < 4; ++i) {
+            painter.setPen(mixxx::RgbColor::toQColor(palette.at(i)));
+            painter.setBrush(mixxx::RgbColor::toQColor(palette.at(i)));


Can you use palette.colorForHotcueIndex() instead? Right now, the icon may show colors that are not actually assigned to hotcues.

Holzhaus · 2020-04-02T13:15:24Z

src/preferences/dialog/dlgprefcolors.cpp

+    return QPixmap();
+}
+
+QIcon DlgPrefColors::drawPaletteIcon(const QString& paletteName) {


Suggested change

QIcon DlgPrefColors::drawPaletteIcon(const QString& paletteName) {

QIcon DlgPrefColors::drawHotcueColorByPaletteIcon(const QString& paletteName) {

src/preferences/dialog/dlgprefcolors.cpp

ColorPaletteEditor: hide hotcue number column for track palette

daschuer · 2020-04-02T15:08:09Z

Done.

I have some objections to the proposed changes:

Use icons for +/- buttons if available

Sound good, in the first place however this may create a bunch of follow up issues:
We just do make sure that the icon is actually drawn. Most of the Desktop environments have button icons disabled. Currently the button resizes with the selected font, will this still work.
Does any desktop environment have + and - icons. In case we do our own icons, do the icons allways have enough contrast with the background used in the Desktop environment. Do they match to the overall desktop style?

Given that only some user uses this dialog only a few times. I suggest to save you from all this work.

Add a Delete Icon to the "Remove Palette" button if available (currently it looks strange if the other buttons all have an icon, but this button does not)

Do you use KDE? I think this is one of the last environments using button Icons together with text.
It even features a setting to hide this button. So if you like to spend the time to add an edit icon on the edit button, please make sure it follows this setting.

Possibly replace the QDialogButtonBox with a HBoxLayout to get a consistent button order across all OSes (if desired)

I think this is a bad idea. All dialogs and even the common bar on the bottom of the preferences are using the QDialogButtonBox() this should feature a consistent user interface compared to other installed applications. It is part of the KDE style to have the Discard button right of OK.
Chaining this would be a step back.

daschuer · 2020-04-02T15:09:06Z

Merge?

ronso0 · 2020-04-02T15:14:24Z

Use icons for +/- buttons if available

Add a Delete Icon to the "Remove Palette" button if available (currently it looks strange if the other buttons all have an icon, but this button does not)

Thought about that as well, but setting icons is not as easy as in the skins or the Hotcue popup: we need to choose contrasting Icons and it's not predictable what background color OS themes use. Didn't look into that: can we use OS icons? At least Delete/Trash bin icon should be available

Holzhaus · 2020-04-02T15:36:34Z

Done.

I have some objections to the proposed changes:

Use icons for +/- buttons if available

Sound good, in the first place however this may create a bunch of follow up issues:
We just do make sure that the icon is actually drawn. Most of the Desktop environments have button icons disabled. Currently the button resizes with the selected font, will this still work.
Does any desktop environment have + and - icons. In case we do our own icons, do the icons allways have enough contrast with the background used in the Desktop environment. Do they match to the overall desktop style?

Most DEs that don't have button icons for buttons that also have a text label. There are icons on buttons when there is no label, which is the case here.
I didn't look into it but I think QIcon offers a way to get the icon by name from the system's icon theme (list-add/list-remove according to the FreeDesktop spec). If there are no icons, we should fall back to textual +/-.

Add a Delete Icon to the "Remove Palette" button if available (currently it looks strange if the other buttons all have an icon, but this button does not)

Do you use KDE? I think this is one of the last environments using button Icons together with text.
It even features a setting to hide this button. So if you like to spend the time to add an edit icon on the edit button, please make sure it follows this setting.

No, I use i3wm. I don't really care about the icon, but the inconsistency looks really bad (all other buttons do have an icon for me):

Possibly replace the QDialogButtonBox with a HBoxLayout to get a consistent button order across all OSes (if desired)

I think this is a bad idea. All dialogs and even the common bar on the bottom of the preferences are using the QDialogButtonBox() this should feature a consistent user interface compared to other installed applications. It is part of the KDE style to have the Discard button right of OK.
Chaining this would be a step back.

Yeah, I'm not sure about this either. I just thought that it's a bit confusing to have a different button order on different OSes (e.g. when we put screenshots into the manual). But I guess users will be able to live with that.

Be-ing · 2020-04-02T18:10:19Z

I agree that using standard add/remove icons instead of "+" and "-" characters would look better.

I guess users will be able to live with that.

I don't think anyone will even notice.

Be-ing · 2020-04-05T04:06:57Z

Icons changed: daschuer#55

daschuer · 2020-04-05T09:11:41Z

This is the way it looks at my machine now, somehow chunky :-/. Compared to @Be-ing s screenshot there seems to be a scaling issue.

If I change to list-add-symbolic and list-remove-symbolic it looks like that:

This is better, but there is not enough contrast.

There is also he issue that this is a Linux only thing see:
https://doc.qt.io/qt-5/qicon.html

Note: By default, only X11 will support themed icons. In order to use themed icons on Mac and Windows, you will have to bundle a compliant theme in one of your themeSearchPaths() and set the appropriate themeName().

So seeing all, I think lets go with + - as text. This works at any OS without additional testing and also with screen readers. Giving that this dialog is not a main Mixxx use case, it is probably not worth to investigate this issue even further ,

Be-ing · 2020-04-05T11:48:29Z

We ship Qt with its default built in theme on macOS and Windows. Does that not provide these icons? Anyway, I propose we merge this now and we continue with the icon discussion in a new PR that won't block the beta release.

Be-ing · 2020-04-05T14:20:41Z

I'm going to go ahead and merge this now then open a new PR targeted at master for the +/- buttons.

Be-ing · 2020-04-05T14:21:17Z

Good work everyone. We came up with a good solution together.

daschuer added 4 commits March 22, 2020 16:55

Improve strings in the color preferences pane

a653006

Allow to translate "Name" in the ColorPaletteEditor

29158a5

Move button row below the palette and optimize column widths

7e1be9e

Fill table with assigned cue numbers if no cue numbers are set

30aaea8

Holzhaus requested changes Mar 22, 2020

View reviewed changes

daschuer mentioned this pull request Mar 22, 2020

Remove auto hotcue colors option #2578

Closed

Be-ing added this to the 2.3.0 milestone Mar 24, 2020

daschuer added 2 commits March 24, 2020 12:20

Introduce Compbobox for selecting the hotcue default color

3b66047

Respect new hotcue default settings

d990c5e

daschuer added 2 commits March 24, 2020 16:06

Added a preview stripe for the selected palette.

c53f7c1

Repopulate the default color selector when changing the palette.

Fix default default hotcue selection

b4d3fa5

daschuer force-pushed the color_preferences branch from 49e6f2b to b4d3fa5 Compare March 24, 2020 15:07

Holzhaus reviewed Mar 24, 2020

View reviewed changes

Holzhaus mentioned this pull request Mar 24, 2020

Add Hotcue Color Replace Dialog #2547

Merged

Be-ing reviewed Mar 24, 2020

View reviewed changes

src/preferences/dialog/dlgprefcolors.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Mar 24, 2020

View reviewed changes

src/preferences/dialog/dlgprefcolors.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Mar 24, 2020

View reviewed changes

ywwg added the blocker label Mar 24, 2020

Added a warning dialog when removing the palette.

4b3f9b4

Be-ing reviewed Mar 30, 2020

View reviewed changes

src/preferences/colorpaletteeditor.cpp Outdated Show resolved Hide resolved

Remove commented code

a82bf80

Be-ing suggested changes Mar 31, 2020

View reviewed changes

src/preferences/colorpaletteeditor.h Show resolved Hide resolved

src/preferences/colorpaletteeditormodel.cpp Show resolved Hide resolved

src/preferences/colorpalettesettings.h Show resolved Hide resolved

src/util/color/colorpalette.h Show resolved Hide resolved

daschuer added 2 commits March 31, 2020 21:30

Swap "+" and "-" button

cfce951

Added some comments

f29e3b8

Holzhaus requested changes Apr 2, 2020

View reviewed changes

daschuer added 2 commits April 2, 2020 15:38

Merge pull request #52 from Be-ing/color_preferences_hide_hotcue_number

3126268

ColorPaletteEditor: hide hotcue number column for track palette

Use the real hotcue colors when coloring the icon

9db6ad2

Be-ing merged commit 3e3b477 into mixxxdj:master Apr 5, 2020

Be-ing mentioned this pull request Apr 5, 2020

ColorPaletteEditor: use theme's list-add and list-remove icons #2630

Merged

daschuer deleted the color_preferences branch September 26, 2021 17:37

	QIcon DlgPrefColors::drawPaletteIcon(const QString& paletteName) {
	QIcon DlgPrefColors::drawHotcueColorByPaletteIcon(const QString& paletteName) {

Color preferences improvements #2589

Color preferences improvements #2589

Conversation

daschuer commented Mar 22, 2020

daschuer commented Mar 22, 2020

Holzhaus left a comment • 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

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

ronso0 commented Mar 22, 2020

daschuer commented Mar 22, 2020

ronso0 commented Mar 22, 2020

Be-ing commented Mar 23, 2020

Holzhaus commented Mar 23, 2020

Be-ing commented Mar 24, 2020 • edited Loading

daschuer commented Mar 24, 2020

daschuer commented Mar 24, 2020

Holzhaus commented Mar 24, 2020

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

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

daschuer commented Mar 30, 2020

Holzhaus commented Mar 30, 2020

Holzhaus commented Mar 30, 2020 • edited Loading

daschuer commented Mar 30, 2020

Be-ing commented Mar 30, 2020 • edited Loading

ywwg commented Mar 31, 2020

Be-ing left a comment

Choose a reason for hiding this comment

daschuer commented Mar 31, 2020

Holzhaus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Apr 2, 2020

daschuer commented Apr 2, 2020

ronso0 commented Apr 2, 2020 • edited Loading

Holzhaus commented Apr 2, 2020

Be-ing commented Apr 2, 2020 • edited Loading

Be-ing commented Apr 5, 2020

daschuer commented Apr 5, 2020

Be-ing commented Apr 5, 2020 • edited Loading

Be-ing commented Apr 5, 2020

Be-ing commented Apr 5, 2020

Holzhaus left a comment •

edited

Loading

Be-ing commented Mar 24, 2020 •

edited

Loading

Holzhaus commented Mar 30, 2020 •

edited

Loading

Be-ing commented Mar 30, 2020 •

edited

Loading

Holzhaus left a comment •

edited

Loading

ronso0 commented Apr 2, 2020 •

edited

Loading

Be-ing commented Apr 2, 2020 •

edited

Loading

Be-ing commented Apr 5, 2020 •

edited

Loading