Make Hotcue RGB colors configurable #2530

Holzhaus · 2020-03-03T19:12:49Z

Follow up to #2520. This adds multiple predefined color palettes and a preferences page to select one.

src/util/color/predefinedcolorpalettes.cpp

Holzhaus · 2020-03-06T15:21:54Z

The Palette Editor was less work than I thought. Here's a little preview:

Holzhaus · 2020-03-06T15:42:58Z

This is lacking a nice icon for the the preference page, otherwise this is ready to review.

Holzhaus · 2020-03-06T15:44:25Z

@ronso0 Do you want to make an icon? Maybe a paintbrush or something like that?

daschuer · 2020-03-06T15:55:44Z

How about something similar to this: https://emojipedia.org/google/android-4.3/artist-palette/

daschuer · 2020-03-06T15:59:11Z

Thank you for the nice little preview.
Did you consider to add an edit mode to hide edit features?

The GUI suits for users who wants to tweak the palette. However this is a rare use case.

We should stream line the GUI for the common task to switch to Serato colors.

daschuer · 2020-03-06T16:04:42Z

The hotcue default color selection can be improved. Did you consider to use the combo box from my proposal?

Do we need a way to go back to default orange once another color has been selected?

ronso0 · 2020-03-06T16:17:54Z

@ronso0 Do you want to make an icon? Maybe a paintbrush or something like that?

Hehee, I was already looking for one.. :)
Reagrding the icon size in the Pref list I think a simple paint brush will do, or even just a brush tip.
A palette at that size is not clear and somehow looks like the GNome icon.

Holzhaus · 2020-03-06T19:33:32Z

Did you consider to add an edit mode to hide edit features?

The GUI suits for users who wants to tweak the palette. However this is a rare use case.

We could remove the palette editor from the preferences page and instead add an "edit palettes" button that opens the palette editor ins a separate window. Is that what you had in mind?

Holzhaus · 2020-03-06T19:40:05Z

The hotcue default color selection can be improved. Did you consider to use the combo box from my proposal?

If you want to use a single color, you can just edit the palette and but a "1" next to the color you want (and remove all other number from the "Assign to Hotcue" column). That will color all new cues in that color.

Do we need a way to go back to default orange once another color has been selected?

You could just re-add orange to the palette and then do as I described above.

I think this allows great flexibility when configuring palettes. IMHO we should even drop the dedicated auto hotcue colors option and make that the default. If you want to make all new cues a single color, you can just configure the palette to your liking and only auto-assign a single color.

ronso0 · 2020-03-09T10:29:22Z

I made an svg from the brush tip.
Does this look okay to everyone?

ic_preferences_colors.svg.png.zip

ywwg · 2020-03-09T13:38:51Z

Does this look okay to everyone?

that seems fine -- the source image you used for this was an open source SVG, right?

ronso0 · 2020-03-09T14:58:24Z

I created the svg by looking a some raster graphics.
Now it's opensource :)

ywwg · 2020-03-09T15:50:11Z

I created the svg by looking a some raster graphics.

IANAL but sgtm

ronso0 · 2020-03-09T18:07:40Z

I created the svg by looking a some raster graphics.

...and tweaking the design for the size we need in the Pref window.
This build errors, so I'll test the icon in master.
I'll be back with the final version then.

src/controllers/colormapperjsproxy.h

Holzhaus · 2020-03-10T10:19:52Z

I'll mark this as WIP until #2520 has been merged.

ywwg

first round of comments. Thanks again for doing all this

src/preferences/colorpaletteeditor.cpp

src/preferences/colorpaletteeditormodel.h

Holzhaus · 2020-03-19T11:27:06Z

Anything else or can we merge this?

uklotzde

Just some minor nitpicks

src/preferences/colorpaletteeditormodel.h

src/preferences/colorpalettesettings.cpp

src/preferences/dialog/dlgprefcolors.h

src/preferences/colorpaletteeditor.h

ywwg

lgtm

Holzhaus · 2020-03-19T13:25:47Z

@uklotzde Done.

uklotzde · 2020-03-19T18:53:15Z

Debug assertion on shutdown (some line numbers may vary, merged into my personal development branch):

Debug [Main]: 910 ms deleting DlgPreferences
DEBUG ASSERT: "!m_ptr || m_ptr->parent()" in function parented_ptr<ColorPaletteEditorModel>::~parented_ptr() [T = ColorPaletteEditorModel] at /tmp/mixxx/src/util/parented_ptr.h:31

Thread 1 "mixxx" received signal SIGABRT, Aborted.

#7  0x00000000004d0e4e in mixxx_debug_assert(char const*, char const*, int, char const*)
    (assertion=0x107e320 "!m_ptr || m_ptr->parent()", file=0x107e33a "/tmp/mixxx/src/util/parented_ptr.h", line=31, function=0x10c8cc4 "parented_ptr<ColorPaletteEditorModel>::~parented_ptr() [T = ColorPaletteEditorModel]") at /tmp/mixxx/src/util/assert.h:11
#8  0x0000000000797e20 in parented_ptr<ColorPaletteEditorModel>::~parented_ptr() (this=0x5673e18) at /tmp/mixxx/src/util/parented_ptr.h:31
#9  0x000000000077d5e3 in ColorPaletteEditor::~ColorPaletteEditor() (this=0x5673dc0) at /tmp/mixxx/src/preferences/colorpaletteeditor.h:14
#10 0x000000000077d64c in ColorPaletteEditor::~ColorPaletteEditor() (this=0x5673dc0) at /tmp/mixxx/src/preferences/colorpaletteeditor.h:14
#11 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#12 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#13 0x00007ffff6f2ba9d in QGroupBox::~QGroupBox() () at /lib64/libQt5Widgets.so.5
#14 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#15 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#16 0x0000000000559018 in DlgPreferencePage::~DlgPreferencePage() (this=0x5675030) at /tmp/mixxx/src/preferences/dlgpreferencepage.cpp:8
#17 0x0000000000b592d1 in DlgPrefColors::~DlgPrefColors() (this=0x5675030) at /tmp/mixxx/src/preferences/dialog/dlgprefcolors.cpp:33
#18 0x0000000000b5931c in DlgPrefColors::~DlgPrefColors() (this=0x5675030) at /tmp/mixxx/src/preferences/dialog/dlgprefcolors.cpp:32
#19 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#20 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#21 0x00007ffff6e308ad in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#22 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#23 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#24 0x00007ffff6f9289d in QScrollArea::~QScrollArea() () at /lib64/libQt5Widgets.so.5
#25 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#26 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#27 0x00007ffff6fa898d in QStackedWidget::~QStackedWidget() () at /lib64/libQt5Widgets.so.5
#28 0x00007ffff42f584c in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5
#29 0x00007ffff6e306b9 in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5
#30 0x00000000005094cd in DlgPreferences::~DlgPreferences() (this=0x432e130) at /tmp/mixxx/src/preferences/dialog/dlgpreferences.cpp:183
#31 0x000000000050954c in DlgPreferences::~DlgPreferences() (this=0x432e130) at /tmp/mixxx/src/preferences/dialog/dlgpreferences.cpp:171
#32 0x00000000004db381 in MixxxMainWindow::finalize() (this=0x7fffffffcfd0) at /tmp/mixxx/src/mixxx.cpp:745

uklotzde · 2020-03-19T19:09:40Z

I also see a warning during startup:

Debug [Main]: DbConnectionPooled - Found thread-local database connection "MIXXX-1" QSqlDatabase(driver="QSQLITE", database="file:///home/uk/.mixxx/mixxxdb.sqlite", host="", port=-1, user="mixxx", open=true)
Warning [Main]: Color Palette "Mixxx Hotcue Colors" is a built-in palette, not writing to config!
Warning [Main]: Color Palette "Mixxx Hotcue Colors" is a built-in palette, not writing to config!
Debug [Main]: Key plugin ID: "qm-keydetector:2"

This should not happen. Either the log level is wrong or we should handle this case gracefully.

…rite

Since the parented_ptr approach caused crashes on shutdown, I switched to a regular pointer and set the QTableView as the parent object of model. According to the Qt docs, this should transfer ownership to the table view when setModel() is called.

uklotzde

Please revert the last commit and keep using parented_ptr. Only the parent m_pTableView was missing:

m_pModel(make_parented<ColorPaletteEditorModel>(m_pTableView))

…del" This reverts commit de013aa.

uklotzde · 2020-03-20T12:40:18Z

Good job, Jan!

LGTM

JaviVilarroig · 2020-03-21T13:22:34Z

@Holzhaus It seems I was too slow. Still slow with git. I have just managed to compile this PR. Still interested in any testing from my side?

ywwg · 2020-03-21T17:16:48Z

it's ok to test still, and if you catch any problems we'll address them for sure!

Holzhaus · 2020-03-21T17:43:03Z

Yup, but it make sense to test this on master. That way you catch problems that were introduced by the merge itself as well.

JaviVilarroig · 2020-03-21T18:04:17Z

OK. Now that I know how to do it I will move to master :)
Anything special to test? O do I just "play" a little bit with the colors?

Holzhaus · 2020-03-21T18:14:40Z

No just play around and check if you can define custom cue palettes. I just found a little bug in master tho, so better test #2586.

Holzhaus added the ui label Mar 3, 2020

Holzhaus added this to the 2.3.0 milestone Mar 3, 2020

Holzhaus self-assigned this Mar 3, 2020

Holzhaus force-pushed the hotcue-rgb-color-picker branch 2 times, most recently from 2001075 to a0d1724 Compare March 4, 2020 11:23

daschuer reviewed Mar 4, 2020

View reviewed changes

src/util/color/predefinedcolorpalettes.cpp Show resolved Hide resolved

Holzhaus force-pushed the hotcue-rgb-color-picker branch 3 times, most recently from b41234d to 2dc411e Compare March 6, 2020 15:03

Holzhaus changed the title ~~[WIP] Make Hotcue RGB colors configurable~~ Make Hotcue RGB colors configurable Mar 6, 2020

Holzhaus force-pushed the hotcue-rgb-color-picker branch 2 times, most recently from 4280a0d to f3ca6f6 Compare March 6, 2020 22:17

uklotzde reviewed Mar 9, 2020

View reviewed changes

src/controllers/colormapperjsproxy.h Outdated Show resolved Hide resolved

Holzhaus changed the title ~~Make Hotcue RGB colors configurable~~ [WIP] Make Hotcue RGB colors configurable Mar 10, 2020

Holzhaus force-pushed the hotcue-rgb-color-picker branch from f3ca6f6 to 5115f1b Compare March 10, 2020 10:29

ywwg requested changes Mar 16, 2020

View reviewed changes

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

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

src/preferences/colorpaletteeditormodel.h Show resolved Hide resolved

Holzhaus added 3 commits March 17, 2020 15:16

preferences/colorpaletteeditor: Move lambda functions to private slots

fb16cd6

preferences/colorpaletteeditor: Replace foreach() with for (x : y)

9cb30ed

preferences/colorpaletteeditor(model): Add class comments to headers

b90de93

uklotzde reviewed Mar 19, 2020

View reviewed changes

Holzhaus added 3 commits March 19, 2020 14:12

preferences/colorpaletteeditormodel: Make isDirty/isEmpty const

da2d65e

preferences/colorpalettesettings: Use range-based for loops

6955377

preferences/dialog/dlgprefcolors: Fixed name and constness of config ptr

6c365bf

ywwg approved these changes Mar 19, 2020

View reviewed changes

preferences/colorpaletteeditor: Improve widget initialization

bdfde5e

Holzhaus added 2 commits March 19, 2020 21:31

preferences/colorpalettesettings: Print debug message when skipping w…

14b9e4d

…rite

Holzhaus requested a review from uklotzde March 20, 2020 10:25

uklotzde reviewed Mar 20, 2020

View reviewed changes

Holzhaus added 2 commits March 20, 2020 13:09

Revert "preferences/colorpaletteeditor: Don't use parented_ptr for mo…

ce88ef1

…del" This reverts commit de013aa.

preferences/colorpaletteeditor: Pass m_pTableView to model

a3b356a

Holzhaus requested a review from uklotzde March 20, 2020 12:15

uklotzde merged commit a189892 into mixxxdj:master Mar 20, 2020

Holzhaus mentioned this pull request Mar 23, 2020

Remove auto hotcue colors option #2578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Hotcue RGB colors configurable #2530

Make Hotcue RGB colors configurable #2530

Holzhaus commented Mar 3, 2020

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

daschuer commented Mar 6, 2020

daschuer commented Mar 6, 2020

daschuer commented Mar 6, 2020

ronso0 commented Mar 6, 2020 •

edited

Loading

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

ronso0 commented Mar 9, 2020

ywwg commented Mar 9, 2020

ronso0 commented Mar 9, 2020

ywwg commented Mar 9, 2020

ronso0 commented Mar 9, 2020

Holzhaus commented Mar 10, 2020

ywwg left a comment

Holzhaus commented Mar 19, 2020

uklotzde left a comment

ywwg left a comment

Holzhaus commented Mar 19, 2020

uklotzde commented Mar 19, 2020

uklotzde commented Mar 19, 2020

uklotzde left a comment

uklotzde commented Mar 20, 2020

JaviVilarroig commented Mar 21, 2020

ywwg commented Mar 21, 2020

Holzhaus commented Mar 21, 2020

JaviVilarroig commented Mar 21, 2020

Holzhaus commented Mar 21, 2020

Make Hotcue RGB colors configurable #2530

Make Hotcue RGB colors configurable #2530

Conversation

Holzhaus commented Mar 3, 2020

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

daschuer commented Mar 6, 2020

daschuer commented Mar 6, 2020

daschuer commented Mar 6, 2020

ronso0 commented Mar 6, 2020 • edited Loading

Holzhaus commented Mar 6, 2020

Holzhaus commented Mar 6, 2020

ronso0 commented Mar 9, 2020

ywwg commented Mar 9, 2020

ronso0 commented Mar 9, 2020

ywwg commented Mar 9, 2020

ronso0 commented Mar 9, 2020

Holzhaus commented Mar 10, 2020

ywwg left a comment

Choose a reason for hiding this comment

Holzhaus commented Mar 19, 2020

uklotzde left a comment

Choose a reason for hiding this comment

ywwg left a comment

Choose a reason for hiding this comment

Holzhaus commented Mar 19, 2020

uklotzde commented Mar 19, 2020

uklotzde commented Mar 19, 2020

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Mar 20, 2020

JaviVilarroig commented Mar 21, 2020

ywwg commented Mar 21, 2020

Holzhaus commented Mar 21, 2020

JaviVilarroig commented Mar 21, 2020

Holzhaus commented Mar 21, 2020

ronso0 commented Mar 6, 2020 •

edited

Loading