Allow assigning more than one Hotcue to a Color in a Colorpalette #2902

Swiftb0y · 2020-06-25T14:05:58Z

The current commit is broken and I'm only pushing it like this because I need some help implementing it.
I had implemented an approach previously that just parses the comma separated list using a regex using
a simple pure function but that led to a bunch of duplicated code later. So I instead decided to subclass
QStandardItem and parse the data in there. However, this is where I kinda got stuck. First of all because
I was unable to find answers to some questions in the Qt docs and second of all because I'm not familiar
enough with C++ to know whether my solution could even work.

Swiftb0y

These were some of the questions that came up and where I'm now stuck. I'd really appreciate some guidance here.

Swiftb0y · 2020-06-25T14:09:08Z

src/preferences/colorpaletteeditormodel.h

@@ -46,3 +49,27 @@ class ColorPaletteEditorModel : public QStandardItemModel {
    bool m_bEmpty;
    bool m_bDirty;
 };
+
+class HotcueIndexListItem : public QStandardItem {


The entire purpose of creating this class is to shift the "(de-)serialization" logic out of the model into the item so the item is responsible for translating between the String input and the data actually supposed to be represented as the string.

Swiftb0y · 2020-06-25T14:13:15Z

src/preferences/colorpaletteeditormodel.h

+
+    HotcueIndexListItem(const QList<int>& hotcueList = {});
+
+    void setData(const QVariant& value, int role = Qt::UserRole + 1) override;


I still don't quite understand the purpose of roles. I get that setData and data are "generic" accessors to get data for different purposes (such as displaying the data itself, providing a tooltip description etc). Since I only intent to intercept the data for editing and displaying, I should be able to reuse those roles instead of making my own role, right? Or what exactly am I supposed to do with the custom role.

Swiftb0y · 2020-06-25T14:14:45Z

src/preferences/colorpaletteeditormodel.cpp

+HotcueIndexListItem::HotcueIndexListItem(const QList<int>& hotcueList = {})
+        : QStandardItem(), m_hotcueIndexList(hotcueList) {
+}
+QVariant HotcueIndexListItem::data(int role = Qt::UserRole + 1) const {


My understanding here is that this acts as a getter for certain roles, right? Can't I just reuse the displayrole here and call the parent for every other role?

mixxx/src/library/browse/browsethread.cpp

Line 95 in 472ec12

class YearItem: public QStandardItem {

I think you want the display role like here.

Swiftb0y · 2020-06-25T14:17:04Z

src/preferences/colorpaletteeditormodel.cpp

+    //         m_hotcueIndexList.append(hotcueIndex);
+    //     }
+    // }
+    const auto hotcueIndexStrings = value.toStringList();


I'm just using a stringlist here because I wanted to test whether it was possible to somehow use that. Though I simply can't know since I don't fully understand how QVariant is supposed to work with a stringlist in this context. The Qt docs don't mention that.

Swiftb0y

Cleaned up the code a bit. It still doesn't build because I'm unable to handle some stuff conceptually (see comments). I'm still not sure whether subclassing QStandardItem to parse and store a parsed version of the data for later access is a good idea. Could I get some feedback on that approach?

src/preferences/colorpaletteeditormodel.h

src/preferences/colorpaletteeditormodel.cpp

Swiftb0y · 2020-06-26T17:58:19Z

src/preferences/colorpaletteeditormodel.cpp

@@ -47,25 +56,39 @@ bool ColorPaletteEditorModel::dropMimeData(const QMimeData* data, Qt::DropAction

 bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVariant& value, int role) {


I'm still confused when setData on the model and when setData on the Item is called...

Swiftb0y · 2020-06-26T17:58:47Z

src/preferences/colorpaletteeditormodel.cpp

@@ -8,6 +15,8 @@ QIcon toQIcon(const QColor& color) {
    return QIcon(pixmap);
 }

+const QRegularExpression hotcueListMatchingRegex(QStringLiteral("([0-9]+)[ ,]+"));


I tried making this part of the class but that didn't work. Is it fine to just leave it here?

src/preferences/colorpaletteeditormodel.h

daschuer · 2020-06-29T12:14:01Z

src/preferences/colorpaletteeditormodel.h

@@ -46,3 +49,25 @@ class ColorPaletteEditorModel : public QStandardItemModel {
    bool m_bEmpty;
    bool m_bDirty;
 };
+
+class HotcueIndexListItem : public QStandardItem {
+    Q_OBJECT


Remove this macro and it will build.

It currently doesn't build because of a bunch of syntax and typeerrors. I don't see how removing it would suddenly fix the build. Can I still remove the Q_OBJECT though? It doesn't use any signals/slots or moc specific mechisms, though the parent QStandardItem does.

Q_OBJECT macro defines some things in objects Inherited from QObject. QStandardItem does not inherit from QObject.

daschuer · 2020-06-29T12:19:51Z

src/preferences/colorpaletteeditormodel.h

+        return QStandardItem::UserType;
+    };
+
+    QList<int> getHotcueIndexList() const {


Here you can return a const &

daschuer · 2020-06-29T12:22:34Z

src/preferences/colorpaletteeditormodel.cpp

    QList<mixxx::RgbColor> colors;
    QMap<int, int> hotcueColorIndices;
    for (int i = 0; i < rowCount(); i++) {
        QStandardItem* pColorItem = item(i, 0);
-        QStandardItem* pHotcueIndexItem = item(i, 1);
-        mixxx::RgbColor::optional_t color = mixxx::RgbColor::fromQString(pColorItem->text());
+        auto pHotcueIndexItem = static_cast<HotcueIndexListItem*>(item(i, 1));


This is the place where you may check (ASSERT) the type()

Swiftb0y · 2020-07-06T11:54:42Z

Thanks for your reviews so far @daschuer. Unfortunately, I'm still a bit split on whether subclassing QStandardItem is actually the right approach here: When data is entered it passes through ColorPaletteEditorModel::setData which is then calling QStandardItem::setData further down the "chain". Subclassing QStandardItem to implement the parsing step is too late since we already need to have the field parsed in ColorPaletteEditorModel::setData however, once that data was parsed and validated, we are saving it again as a string in the QStandardItem. That would mean that each time the data is changed, we would evaluate x regexp (x being the number of colours in the palette) just to discard that parsed data afterward again.
The only two alternative approach I can think of would be either getting rid of the subclassed QStandardItem completely (meaning we'd have to parse each field each time just to throw away the result once we finished validating it), or just keeping it for storing the parsed hotcuelist and making the parsing step a static method. However, the second method isn't pretty either because we pretty much abuse the class just for storing the parsed data grouping functions.
What do you think?

github-actions · 2020-10-19T00:21:59Z

This PR is marked as stale because it has been open 90 days with no activity.

Make it possible to assign the same color to different hotcues by writing them as a comma-separated list into the table cell

Swiftb0y · 2021-04-03T14:38:28Z

I'd like to propose to add this to 2.3 as it fixes a missing feature / bug highlighted by #3746

Swiftb0y · 2021-04-03T15:03:18Z

src/preferences/colorpaletteeditormodel.cpp

    // If we have a non consequitive list of hotcue indexes, indexes are shifted down
    // due to the sorting nature of QMap. This is intended, this way we have a color for every hotcue.
    return ColorPalette(name, colors, hotcueColorIndices.values());


I fear handling this silently results in confusion for the user. While we unfortunately don't have a "default color" we could insert into these empty positions, we should still test whether this shifting will occur and warn the user in that case.

Opinions? @Holzhaus

If you want to implement a warning icon/message that is displayed in the edit dialog, go ahead.

Holzhaus

How is this supposed to work? I typed2,3, 2, 3 and 2-3 into a lineedit field, and none of them works (field became empty after pressing enter or clicking elsewhere).

Holzhaus · 2021-04-04T22:29:30Z

How is this supposed to work? I typed 2,3, 2, 3 and 2-3 into a lineedit field, and none of them works (field became empty after pressing enter or clicking elsewhere).

Ah ok, I didn't merge this correctly. It works, but there are some issues:

After closing the edit window, the indices becomes zero-based (1 becomes 0, 2 becomes 1, etc.)
It would be nice to allow ranges 1-3 instead of comma-separated values, too
Editing is a bit clunky become it's not possible to reassign hotcues without unassigning first. E.g. if I have some color that is assigned to 1 (or some combination like 1, 3), then I can't assign 1 to another color. I'd expect that to work and automatically unassign 1 on the other color.

Swiftb0y · 2021-04-05T10:03:13Z

After closing the edit window, the indices becomes zero-based (1 becomes 0, 2 becomes 1, etc.)

That is indeed a bug I overlooked, thanks for finding it.

It would be nice to allow ranges 1-3 instead of comma-separated values, too

Indeed, though its a bit more difficult to implement. I'll investigate.

Editing is a bit clunky become it's not possible to reassign hotcues without unassigning first. E.g. if I have some color that is assigned to 1 (or some combination like 1, 3), then I can't assign 1 to another color. I'd expect that to work and automatically unassign 1 on the other color.

I agree, though that increases complexity quite a bit, I'll see if I can refactor the code and work this in afterwards.

The entire feature hacks around some of Qts Model/View idioms, so either its not suited for what I'm trying to do or I don't understand Qt well enough to implement it properly. If anyone has ideas how to implement this better, please share.

Allow hotcue index assignments to consist of a mix of regular indicies and ranges of indicies. For example "1 - 3, 5" to express the list `[1, 2, 3, 5]`.

Swiftb0y · 2021-04-08T17:24:05Z

@Holzhaus

I implemented both of your suggestions. What do you think?

Swiftb0y · 2021-04-14T22:21:04Z

I would love for this to go into 2.3, but as I said, I'd like to polish it more, but I don't know whether I'll find time for that. I guess I should just focus creating unit tests and any other polishing is optional.

Holzhaus · 2021-04-14T22:33:41Z

I suggest we merge now and polish later, if you find the time.

daschuer · 2021-04-14T22:48:49Z

The list af ideas to make it more intuitive, was just a brainstorming of alternative. Any improvement is welcome.

If a test looks ugly it is ok. Just a ugly test is better then no test.

I don't think it is worth the effort to refactor the functions out. That can be done if it is needed.

If you think you are ready here, just remove the draft state and we can merge.

Swiftb0y · 2021-04-15T16:50:50Z

Thanks, I'll create some unit tests and then we can merge. If I won't find time today, I'll try to next week.

daschuer · 2021-04-15T19:44:19Z

Thank you.

Swiftb0y · 2021-04-18T12:47:43Z

I moved the free functions and constants into their own file and wrote some unit tests. What do you think?

Holzhaus

Thank you, works well!

src/preferences/colorpaletteeditormodel.cpp

src/test/rangelist_test.cpp

src/util/rangelist.h

Holzhaus · 2021-04-18T15:29:09Z

src/preferences/colorpaletteeditormodel.h

+        return QStandardItem::UserType;
+    };
+
+    const QList<int>& getHotcueIndexList() const {


Suggested change

const QList<int>& getHotcueIndexList() const {

const QList<int>& getHotcueIndexList() const {

QList is implicitly shared, should we really return a const reference here?

Good question. My guess was that explicitly constructing a QList with that reference at the call site when needed should still be pretty fast because of implicit sharing.

@daschuer @uklotzde opinions? I don't think really care and it can be changed in a subsequent PR you think it's needed. So no need to delay this PR.

Yes, references should be always const, except in a locals scope like fool[i].bar() and similar.

src/preferences/colorpaletteeditormodel.cpp

Swiftb0y · 2021-04-18T19:23:53Z

@Holzhaus thanks for the review. I addressed all but one of your suggestions.

Holzhaus

Thank you, LGTM.

Holzhaus

Ah, I just noticed we lost the mixxx namespace for rangelist.

src/util/rangelist.h

src/util/rangelist.cpp

variables of pointers should be prefixed with `p`

enable detection of list containing duplicates

Swiftb0y · 2021-04-19T11:25:08Z

Thanks, I went back changed that commit and rebased the rest. Should be fine now.

Holzhaus

LGTM, thanks.

documents feature introduced in mixxxdj/mixxx#2902

Swiftb0y commented Jun 25, 2020

View reviewed changes

Swiftb0y force-pushed the enable-multple-cues-per-color-autoassign branch from c273c3f to 0861564 Compare June 26, 2020 17:50

Swiftb0y commented Jun 26, 2020

View reviewed changes

daschuer requested changes Jun 29, 2020

View reviewed changes

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2020

Be-ing changed the base branch from master to main October 23, 2020 23:16

Swiftb0y mentioned this pull request Mar 24, 2021

PredefinedColorPalettes: Add Rekordbox Hotcue Color Palettes #3746

Merged

ColorPaletteEditor: enable multi cue auto-assign

8ffb21e

Make it possible to assign the same color to different hotcues by writing them as a comma-separated list into the table cell

Swiftb0y force-pushed the enable-multple-cues-per-color-autoassign branch from 0861564 to 8ffb21e Compare April 3, 2021 14:34

github-actions bot added the ui label Apr 3, 2021

Swiftb0y added minor bug and removed stale Stale issues that haven't been updated for a long time. labels Apr 3, 2021

Swiftb0y marked this pull request as ready for review April 3, 2021 14:36

Swiftb0y requested a review from daschuer April 3, 2021 14:36

Swiftb0y changed the base branch from main to 2.3 April 3, 2021 14:36

ColorPaletteEditor: fix crash when autoassign column is empty

8d53dcd

Swiftb0y commented Apr 3, 2021

View reviewed changes

HotcueIndexListItem: fix missing override warning

68b2115

Holzhaus reviewed Apr 4, 2021

View reviewed changes

ColorPaletteEditorModel: fix loading index 0-based

02de7df

Swiftb0y marked this pull request as draft April 7, 2021 10:21

ColorPaletteEditorModel: add range index assign

20df43c

Allow hotcue index assignments to consist of a mix of regular indicies and ranges of indicies. For example "1 - 3, 5" to express the list `[1, 2, 3, 5]`.

Swiftb0y force-pushed the enable-multple-cues-per-color-autoassign branch from 8d865d4 to d7d7ef0 Compare April 8, 2021 17:34

Swiftb0y added 2 commits April 15, 2021 22:21

ColorPaletteEditorModel move rangelist to own file

c2f5668

rangelist: add some unit tests

bc50748

github-actions bot added build code quality labels Apr 15, 2021

Swiftb0y marked this pull request as ready for review April 15, 2021 20:22

ColorPaletteEditorModel remove unnecessary comment

08c842f

Holzhaus reviewed Apr 18, 2021

View reviewed changes

Holzhaus approved these changes Apr 18, 2021

View reviewed changes

Holzhaus reviewed Apr 18, 2021

View reviewed changes

src/util/rangelist.h Show resolved Hide resolved

src/util/rangelist.cpp Show resolved Hide resolved

Swiftb0y added 7 commits April 19, 2021 13:05

rangelist: separate declaration and definition

7c5b0de

ColorPaletteEditorModel: adhere to naming scheme

ffaa010

variables of pointers should be prefixed with `p`

ColorPaletteEditorModel: reduce nesting

14b7560

ColorPaletteEditorModel: fix if branch braces

d989d73

ColorPaletteEditorModel: improve const-correctness

f1aa399

rangelist: add debug assert testing invalid lists

b549298

enable detection of list containing duplicates

rangelist: debug assert on negative numbers

cd95da5

Swiftb0y force-pushed the enable-multple-cues-per-color-autoassign branch from a1cf8ad to cd95da5 Compare April 19, 2021 11:17

Holzhaus approved these changes Apr 19, 2021

View reviewed changes

Holzhaus merged commit ff4275e into mixxxdj:2.3 Apr 19, 2021

Swiftb0y added a commit to Swiftb0y/manual that referenced this pull request Apr 19, 2021

preferences: assign >1 hotcues to color in palette

44d0e87

documents feature introduced in mixxxdj/mixxx#2902

Swiftb0y mentioned this pull request Apr 19, 2021

preferences: assign >1 hotcues to color in palette mixxxdj/manual#372

Merged

Swiftb0y deleted the enable-multple-cues-per-color-autoassign branch April 19, 2021 17:59


		HotcueIndexListItem(const QList<int>& hotcueList = {});

		void setData(const QVariant& value, int role = Qt::UserRole + 1) override;

		@@ -47,25 +56,39 @@ bool ColorPaletteEditorModel::dropMimeData(const QMimeData* data, Qt::DropAction

		bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVariant& value, int role) {

	const QList<int>& getHotcueIndexList() const {
	const QList<int>& getHotcueIndexList() const {

Allow assigning more than one Hotcue to a Color in a Colorpalette #2902

Allow assigning more than one Hotcue to a Color in a Colorpalette #2902

Conversation

Swiftb0y commented Jun 25, 2020

Swiftb0y 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y 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

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 Jul 6, 2020

github-actions bot commented Oct 19, 2020

Swiftb0y commented Apr 3, 2021

Swiftb0y Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus commented Apr 4, 2021

Swiftb0y commented Apr 5, 2021

Swiftb0y commented Apr 8, 2021

Swiftb0y commented Apr 14, 2021

Holzhaus commented Apr 14, 2021

daschuer commented Apr 14, 2021

Swiftb0y commented Apr 15, 2021

daschuer commented Apr 15, 2021

Swiftb0y commented Apr 18, 2021

Holzhaus 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

Choose a reason for hiding this comment

Swiftb0y commented Apr 18, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

Swiftb0y commented Apr 19, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Swiftb0y Apr 3, 2021 •

edited

Loading