Add ControlObjects for track table sorting

[Holzhaus]

This is supposed to resolve Launchpad Bug #1828882 ("Add mapping controls for library
sort").

It adds two controls to the [Library] group:

Key/Control	Range	What it does	On-screen feedback
sort_column	0-??	Indicates the sorting column of the model for the track table.	Sorting indicator in the column headers of the track table.
sort_order	binary	0 means that the order is ascending, 1 that it is descending.	Sorting indicator in the column headers of the track table.

I do not have any experience with the Mixxx codebase or Qt, so I'm relatively sure that my code absolutely abysmal. Feedback would be appreciated.

[upCASE]

Splendid! The 202 will benefit from this as well.
What would be real nice is, if the column labels would somehow be accessible in scripts, so that it would always sort the desired one even if they were rearranged.

[Holzhaus]

@upCASE

What would be real nice is, if the column labels would somehow be accessible in scripts, so that it would always sort the desired one even if they were rearranged.

That'd be nice, but since the column numbers are taken from the underlying track model, not derived from the column order in the table widget, the column indices should be static unless the track model source code changes.

[uklotzde]

What happens if the columns are rearranged? I suppose then the column index in the control doesn't match the primary sort column and the internal state becomes inconsistent. How do we handle this case?

[Holzhaus]

@uklotzde Rearraging columns via drag&drop on the column headers does not influence the column indices, since the indices refer to the columns in the underlying track model, NOT the columns in the WTrackTableView widget. AFAIK it isn't possible to rearrange the former without patching and recompiling Mixxx. Hence, there shouldn't be a problem.

[uklotzde]

The reduction in both code size and runtime dependencies by your latest commit is nice, even more than what I expected :)

[uklotzde]

LGTM

@daschuer / @Be-ing Did we miss anything?

[daschuer]

How will this Control be used? Is there an issue we sort twice when changing the column?
Currently the sort order is saved per column. This is probably broken now.

Can we read back the sort order after changing the sort column and change the "sort_order" CO accordingly?

[Holzhaus]

When changing the sort column by clicking on the column headers, the table should only be sorted once.

When the CO is used in a controller mapping, it depends. Here's an example how I'd use it on the DJ-505:

if (sortColumn === engine.getValue("[Library]", "sort_column")) {
    script.toggleControl("[Library]", "sort_order");
} else {
    engine.setValue("[Library]", "sort_column", sortColumn);
    engine.setValue("[Library]", "sort_order", 0);
}

The code would trigger two sorts if the sort_order is descending when the sort_column changes, because the JS code sets sort_order to 0 if the column changes.

I could avoid this by setting the sort_order to 0 on sort_column changes unconditionally in the C++ code. However, there might be legitimate usecases for keeping the order even when the column changes, so I'd rather not do that and keep the current behaviour.

[daschuer]

Ah OK, so you have a push button for each column exactly like the table header bar in the GUI.
Right?

Than I think the new controls should behave like this without a need of script magic.
How about this:

• Setting "sort_column" to a new value sorts the new coloumn with the stored value.
• the current sort order is reflected in "sort_order"
• Setting the same value to "sort_column" again toggles the sorting. The same happens if you click again on the header in the GUI.

Your script will then melt down to just:

    engine.setValue("[Library]", "sort_column", sortColumn);

[Holzhaus]

Ah OK, so you have a push button for each column exactly like the table header bar in the GUI.
Right?

Correct.

Than I think the new controls should behave like this without a need of script magic.
How about this:

• Setting "sort_column" to a new value sorts the new coloumn with the stored value.
• the current sort order is reflected in "sort_order"
• Setting the same value to "sort_column" again toggles the sorting. The same happens if you click again on the header in the GUI.

If you assign a new value to sort_column, should sort_order get reset to 0 or keep its current value?

[daschuer]

The idea was to model the exact behavior of the header bar and update the "sort_order" from the c++ domain after changing the "sort_column".
Originally I had the impression that the sort order is saved per column, but I have just tested again and it turns out that this is not true.

[Holzhaus]

I noticed something odd when using qDebug() statements. When I apply the following patch, it looks like some slots get called multiple times:

diff --git a/src/widget/wtracktableview.cpp b/src/widget/wtracktableview.cpp
index 4fd1acfc3..188d98efe 100644
--- a/src/widget/wtracktableview.cpp
+++ b/src/widget/wtracktableview.cpp
@@ -128,9 +128,9 @@ WTrackTableView::WTrackTableView(QWidget * parent,
     m_pKeyNotation->connectValueChanged(this, &WTrackTableView::keyNotationChanged);

     m_pSortColumn = new ControlProxy("[Library]", "sort_column", this);
-    m_pSortColumn->connectValueChanged(this, &WTrackTableView::applySorting);
+    m_pSortColumn->connectValueChanged(this, &WTrackTableView::slotSortColumnChanged);
     m_pSortOrder = new ControlProxy("[Library]", "sort_order", this);
-    m_pSortOrder->connectValueChanged(this, &WTrackTableView::applySorting);
+    m_pSortOrder->connectValueChanged(this, &WTrackTableView::slotSortOrderChanged);

     connect(this, SIGNAL(scrollValueChanged(int)),
             this, SLOT(slotScrollValueChanged(int)));
@@ -1781,6 +1781,7 @@ void WTrackTableView::doSortByColumn(int headerSection) {
 }

 void WTrackTableView::applySorting() {
+    qDebug() << "applySorting";
     int sortColumn = static_cast<int>(m_pSortColumn->get());
     Qt::SortOrder sortOrder = m_pSortOrder->get() ? Qt::DescendingOrder : Qt::AscendingOrder;

@@ -1992,15 +1993,17 @@ void WTrackTableView::slotReloadCoverArt() {
 }

 void WTrackTableView::slotSortingChanged(int headerSection, Qt::SortOrder order) {
-
+    qDebug() << "slotSortingChanged" << headerSection << order;
     double sortOrder = static_cast<double>(order);
     bool sortingChanged = false;

     if (headerSection != static_cast<int>(m_pSortColumn->get())) {
+        qDebug() << "setSortColumn" << headerSection;
         m_pSortColumn->set(headerSection);
         sortingChanged = true;
     }
     if (sortOrder != m_pSortOrder->get()) {
+        qDebug() << "setSortOrder" << order;
         m_pSortOrder->set(sortOrder);
         sortingChanged = true;
     }
@@ -2028,3 +2031,13 @@ void WTrackTableView::keyNotationChanged()
 {
     QWidget::update();
 }
+
+void WTrackTableView::slotSortColumnChanged(double v)
+{
+    qDebug() << "slotSortColumnChanged" << v;
+}
+
+void WTrackTableView::slotSortOrderChanged(double v)
+{
+    qDebug() << "slotSortOrderChanged" << v;
+}
diff --git a/src/widget/wtracktableview.h b/src/widget/wtracktableview.h
index 8eaef53c4..ffb993199 100644
--- a/src/widget/wtracktableview.h
+++ b/src/widget/wtracktableview.h
@@ -95,6 +95,8 @@ class WTrackTableView : public WLibraryTableView {
     void slotTrackInfoClosed();
     void slotTagFetcherClosed();
     void slotSortingChanged(int headerSection, Qt::SortOrder order);
+    void slotSortColumnChanged(double v);
+    void slotSortOrderChanged(double v);
     void keyNotationChanged();

   private:

Debug [Main]: slotSortingChanged 7 Qt::AscendingOrder <-- Artist column header was clicked
Debug [Main]: setSortColumn 7
Debug [Main]: slotSortColumnChanged 7
Debug [Main]: slotSortColumnChanged 7
Debug [Main]: slotSortColumnChanged 7
Debug [Main]: slotSortColumnChanged 7
Debug [Main]: slotSortColumnChanged 7
Debug [Main]: applySorting
Debug [Main]: LibraryTableModel(0x561ba672c2a0) select() took 1 ms 124
Debug [Main]: slotSortingChanged 8 Qt::AscendingOrder <-- Title column header was clicked
Debug [Main]: setSortColumn 8
Debug [Main]: slotSortColumnChanged 8
Debug [Main]: slotSortColumnChanged 8
Debug [Main]: slotSortColumnChanged 8
Debug [Main]: slotSortColumnChanged 8
Debug [Main]: slotSortColumnChanged 8
Debug [Main]: applySorting
Debug [Main]: LibraryTableModel(0x561ba672c2a0) select() took 1 ms 124
Debug [Main]: slotSortingChanged 8 Qt::DescendingOrder <-- Title column header was clicked again
Debug [Main]: setSortOrder 8
Debug [Main]: slotSortOrderChanged 1
Debug [Main]: slotSortOrderChanged 1
Debug [Main]: slotSortOrderChanged 1
Debug [Main]: slotSortOrderChanged 1
Debug [Main]: slotSortOrderChanged 1
Debug [Main]: applySorting
Debug [Main]: LibraryTableModel(0x561ba672c2a0) select() took 1 ms 124
Debug [Main]: slotSortingChanged 8 Qt::AscendingOrder <-- Title column header was clicked again
Debug [Main]: setSortOrder 8
Debug [Main]: slotSortOrderChanged 0
Debug [Main]: slotSortOrderChanged 0
Debug [Main]: slotSortOrderChanged 0
Debug [Main]: slotSortOrderChanged 0
Debug [Main]: slotSortOrderChanged 0
Debug [Main]: applySorting
Debug [Main]: LibraryTableModel(0x561ba672c2a0) select() took 1 ms 124

I don't get why this happens. Any ideas?

[Holzhaus]

Looks like slots being fired is caused by m_pSortOrder->set(sortOrder); and m_pSortColumn->set(headerSection);. Shouldn't this be prevented by https://github.com/mixxxdj/mixxx/blob/master/src/control/controlproxy.h#L171 ?

[daschuer]

Normally yes.
I would use GDB and place a breakepoint in the slot and look at the call stack.
This should tell you what happens.

On which OS and IDE are you?

[Holzhaus]

@daschuer I'm using Linux x86_64 with X11. My IDE is vim. I'll try to debug this tomorrow, when I have some spare time.

[Holzhaus]

@daschuer Looks like there are multiple instances WTrackTableView, each with a seperate ControlProxy instance. These can be shown/hidden by selecting the corresponding sidebar entry ("Tracks", "Computer", ...). I think the simplest way to fix this is by checking if the WTrackTableView instance is visible in the applySorting() method.

[Holzhaus]

There's another problem with this implementation: Apparently there are different track models for the different WTrackTableView instances. In the "Tracks" table, sort_column is 7 when I sort by "Artist". In the file system table ("Computer"), sorting by "Artist" sets sort_column to 2. Hence, the current implementation is not really useful.

Is there a way to have string controls so that we can use column names? Or should I implement an additional abstraction layer that maps column indices to some model-independent numbering scheme?

[daschuer]

Or should I implement an additional abstraction layer that maps column indices to some model-independent numbering scheme?

This sounds reasonable anyway, even without the issue. It would allow us to change implementation details without breaking mappings.

This bit might be interesting:

mixxx/src/library/basesqltablemodel.cpp

Line 67 in ae392c9

setHeaderProperties(ColumnCache::COLUMN_LIBRARYTABLE_ARTIST,

[Holzhaus]

Ok, I added a translation layer to the individual TrackModel subclasses. Here's an overview what values can be assigned to the sort_column control object.

sort_column	Column	Library	Playlist	Crate	Browse
0	Artist	X	X	X	X
1	Title	X	X	X	X
2	Album	X	X	X	X
3	Albumartist	X	X	X	X
4	Year	X	X	X	X
5	Genre	X	X	X	X
6	Composer	X	X	X	X
7	Grouping	X	X	X	X
8	Tracknumber	X	X	X	X
9	Filetype	X	X	X	X
10	Native Location	X	X	X	X
11	Comment	X	X	X	X
12	Duration	X	X	X	X
13	Bitrate	X	X	X	X
14	BPM	X	X	X	X
15	ReplayGain	X	X	X	X
16	Datetime Added	X	X	X	X
17	Times Played	X	X	X	X
18	Rating	X	X	X	X
19	Key	X	X	X	X
20	Preview	X	X	X	X
21	Coverart	X	X	X
22	Position		X
23	Playlist ID		X
24	Location		X
25	Filename				X
26	File Modified Time				X
27	File Creation Time				X

Feedback is appreciated ;-)

[daschuer]

Looks good, need to test is though.
Do you have worrys about this?

[Holzhaus]

Looks good, need to test is though.
Do you have worrys about this?

No, but I didn't test all tables (e.g. Recordings), so I'd like someone else to test this.

[uklotzde]

We have a weird mixture between snake_case and PascalCase control objects. The [Library] section mainly uses PascalCase. Should we adopt this convention here?

[Holzhaus]

We also have the mixture inside sections, e.g. [Library] we have GoToItem and show_coverart. This is also the case with other sections, e.g. [Master] has booth_enabled and headEnabled. IMHO it makes sense to unify this globally.

When looking at the wiki, it looks like the majority uses snake_case. I'd suggest that from now on, new control object should always use snake case and that existing control objects are converted from PascalCase at some later point (v2.4.0? v3.0.0?).

[daschuer]

snake_case for NEW COs works for me.
Can you add a hint to the wiki for that.

We should not touch old COs though. There is only a minor benefit compared to the hassle to maintain two versions for backwards compatibility.

[uklotzde]

I also agree that snake_case is easier to read when used in the code.

[Holzhaus]

snake_case for NEW COs works for me.
Can you add a hint to the wiki for that.

Done.

We should not touch old COs though. There is only a minor benefit compared to the hassle to maintain two versions for backwards compatibility.

True. That why I'd propose a backwards-incompatible change that can be added in case some future major release breaks backwards-compatibility anyway.

[daschuer]

I have did some tests and left some comments.

[daschuer]

can you fix the CodeFactor complains as well?

[daschuer]

I have just tested this branch and it works perfect.
Will you actually use the sort_column or the sort_column_toggle control from scripts?
If you use the first we will probably still suffer the double sql lookup issue.
A way around would be to use negative values for descending order.

[Holzhaus]

@daschuer The CodeFactor complaints have been fixed.

Will you actually use the sort_column or the sort_column_toggle control from scripts?

I'll use the sort_column_toggle control for the Roland DJ-505 mapping (#2125).

If you use the first we will probably still suffer the double sql lookup issue.
A way around would be to use negative values for descending order.

The double SQL select occurs only when both sort_column and sort_order are set in succession because setting each control will trigger the applySorting function and thus an SQL lookup. The sort_column_toggle works around this by setting both sort_column and sort_order but calling applySorting only once.
If some controller has dedicated button for column and sort order so that you can switch the column while keeping the order unchanged (thus only set one of the controls per button press), these controls won't from a double sql lookup.

I'm not sure how negative values for the descending order would change this behaviour.

[daschuer]

Ok. Thank you for you patience and making this really good.
LGTM