Use rgb presentation for cue colors with "no color" migration #2398
Conversation
.. instead of PredefinedColorPointer. COs and the database now hold the color hex int representation instead of a color id.
... and use QColor instead.
and change cue color CO name
Shade still does not work
daschuer
changed the title
daschuer
changed the title
|
Who has interests to try it out? |
daschuer
changed the title
|
OK, the TODOs are done. Has anyone interests to review? |
res/schema.xml
Outdated
| </revision> | ||
| <revision version="31" min_compatible="3"> | ||
| <description> | ||
| Convert the PredefinedColor id to the actual RGBA value. |
res/schema.xml
Outdated
| </description> | ||
| <sql> | ||
| <!-- No Color becomes black 0x000000 --> | ||
| UPDATE cues SET color=0 WHERE color=0; |
There was a problem hiding this comment.
SQLite understands hex numbers and we should use this notation here:
https://www.sqlite.org/syntax/numeric-literal.html
There was a problem hiding this comment.
Cool, I was not aware of this.
src/skin/legacyskinparser.cpp
Outdated
| @@ -388,6 +388,15 @@ QWidget* LegacySkinParser::parseSkin(const QString& skinPath, QWidget* pParent) | |||
| m_pParent = pParent; | |||
| QList<QWidget*> widgets = parseNode(skinDocument); | |||
|
|
|||
| const QString hotcueSkinDefaultColor = m_pContext->variable("HotcueSkinDefaultColor"); | |||
| if (!hotcueSkinDefaultColor.isNull()) { | |||
| QColor color; | |||
There was a problem hiding this comment.
Use the named color ctor to create an immutable value:
const QColor color(hotcueSkinDefaultColor);
| QRgb offPaletteDefaultColor() const; | ||
|
|
||
| private: | ||
| static const QString sGroup; |
There was a problem hiding this comment.
The static const member is only needed for the inlined function. I suggest to move it into an anonymous namespace in the .cpp file and un-inline the member function. This is a typical example of premature optimization.
src/track/cue.cpp
Outdated
| @@ -28,20 +28,29 @@ Cue::Cue(TrackId trackId) | |||
| m_sampleEndPosition(Cue::kNoPosition), | |||
| m_iHotCue(-1), | |||
| m_label(kDefaultLabel), | |||
| m_color(Color::kPredefinedColorsSet.noColor) { | |||
| m_color(0), | |||
There was a problem hiding this comment.
m_color(kDefaultDbColorValue)?
src/track/cue.cpp
Outdated
| @@ -52,6 +61,8 @@ Cue::Cue(int id, TrackId trackId, Cue::Type type, double position, double length | |||
| } else { | |||
| m_sampleEndPosition = Cue::kNoPosition; | |||
| } | |||
|
|
|||
| DEBUG_ASSERT(m_color); | |||
src/track/cue.cpp
Outdated
| m_bDirty = true; | ||
| m_colorIsDefault = false; | ||
| lock.unlock(); | ||
| emit(updated()); |
There was a problem hiding this comment.
Please change all emit statements according to the recent clazy fixes.
src/util/color/colorpalette.h
Outdated
| #pragma once | ||
|
|
||
| #include <QColor> | ||
| #include "QList" |
There was a problem hiding this comment.
Wrong #include directive. The whole class seems to be unfinished by violating many best practice rules.
|
This implements the "default color" idea but calls it a "legacy cue"? How would anyone know what a "legacy cue" is and why does this concept exist? Does anyone actually want the cue-color-per-skin option? Why was it added? Why are there options if Mixxx explicitly warns the user they are not recommended?? I don't understand this PR. |
|
I think this PR creates new problems without bringing us any closer to a release. I think it would be best to close this and let @ferranpujolcamins continue from where #2399 left off. |
daschuer
changed the title
I have removed the term legacy cue because it turns out to be misleading. We had the discussion that we want to discard the "no color" state as soon as the user has selected a palette color. But we don't want a pop up during migration from 2.2. So we need to save this state, which is done with black.
I have no colors set yet for all my track cues and I will probably not make use of this feature because my cue pads have no colors as well. Original I have selected orange to indicate that the cue color is not selected. But It looks misplaced in Shade. I am very confident with this, because nothing distracting will happen after migration to 2.3 if we use the old colors until the user starts to use the cue color feature.
Because it is recommended to use a palette color default, that replaces persistently the info that the cue has no color. This will happen implicit if one does a Serato round-trip. A user might be tempted to give a default color a meaning. But it is not possible to select it in the color picker. It serves only the use case for the time the user is unsure about coloring. The user can only add the original default color to the palette. This way it is stored in the database. This was IMHO part of our conclusion in #2345 right? |
No! There is no need to save this state, whatever you call it. This state will only exist if you invent it here. In 2.5 months no one has agreed it is a good idea to invent this state. |
|
I think @ferranpujolcamins is working on PR #2345. I'll close this for now. |
|
#2345 if far behind of this branch. |
|
Superseeded by #2520. |
|
Can we close this obsolete PR? |
|
Yes. |
This is a possible solution for #2345 without a persistent "no color" state.
Lagacy cues are stored with black color in the database, and are replaces by the selected default color on track load. By default the legacy skin default color is used in the CO interface and the skin. The default color is not stored (kept black) until the user manually decides for a different color per Hotcue or selects a default color from the palette as default. This replaces the black color entirely.
During development I have played with the different options and this turns out to be the smoothest transition from uncolored cues to colored. We get rid of the "no color" state as soon as the user decides for a default color. The user is not forced to do it immediately after upgrade, the GUI looks almost like Mixxx 2.2 until he has decided.
Instead of QColor it uses now the lightweight QRgb. The A value is omitted.
TODO: