Bug: remember library sorting #947

jmigual · 2016-05-11T16:47:53Z

Resolves https://bugs.launchpad.net/mixxx/+bug/1474098

Now the BaseSqlTableModel remembers the hierarchical sorting across restarts

rryan · 2016-05-11T18:44:26Z

src/library/basesqltablemodel.cpp

@@ -21,7 +21,7 @@
 #include "util/assert.h"
 #include "util/performancetimer.h"

-static const bool sDebug = false;
+static const bool sDebug = true;


Please keep this one off since it produces a ton of verbose logging.

daschuer · 2016-05-11T19:31:32Z

Travis fails unrelated to this PR:

The job exceeded the maxmimum time limit for jobs, and has been terminated.

rryan · 2016-05-11T19:31:32Z

src/library/basesqltablemodel.h

@@ -139,6 +139,23 @@ class BaseSqlTableModel : public QAbstractTableModel, public TrackModel {
        }
        int m_column;
        Qt::SortOrder m_order;
+
+        friend QTextStream& operator >> (QTextStream &in, SortColumn &sc) {


Storing the columns as integers means we can't make changes to the column orderings in the future since they are stored in user databases. Could you store them as column names instead?

Ok I'll change that

I am afraid we hit a conceptual issue. :-/
The current solution is a simple serialize without ans sanity checks. The same we have with the header state.
Since this is not that critical data, that "could" work as it is, including all issues we already heave.
If not, we should should probably ditch the SettingsDAO entirely and move to mixxx.cfg, which allows us to parse and check the values before using. .. and is human readable.
@jmigual and @rryan: What do you think about it?

But if we save the column name as a string this should solve the problem. It is true that there are no sanity checks but this can only be a problem if we change the name or remove a column that the user has previously sorted but once she tries to sort again and restarts mixxx the problem solves (when I changed from integers to strings I had this problem and I forced sorting and restarted and then it worked fine again)

I am afraid we hit a conceptual issue. :-/
The current solution is a simple serialize without ans sanity checks. The same we have with the header state.

Since this is not that critical data, that "could" work as it is, including all issues we already heave.

I would prefer to treat user data as "sacred" -- if we decide to save something we should save it in a future-proof manner or not save it at all :).

If not, we should should probably ditch the SettingsDAO entirely and move to mixxx.cfg, which allows us to parse and check the values before using. .. and is human readable.

SettingsDAO and mixxx.cfg don't seem any different in this regard, are they? You can store any QString / QByteArray in either of them. It's up to the code that reads/stores to it to parse/check.

We could use a protocol buffer to serialize column sort state -- we could store that in either mixxx.cfg or SettingsDAO and we would get parsing, serializing and the ability to migrate users to a future format easily. Human readability can be achieved with protobuf text formatting but I'm not sure if it needs to be human readable?

Before this PR SettingsDAO has serialized data which where likely misinterpreted after an update and mixxx.cfg has human readable and editable data. I like the idea to treat the data as "sacred", but we don't do this with the header state.

Right -- header data is opaque since we can't really tell what Qt is storing in there (well, we could but we don't). So we rely on Qt to correctly save/restore it.

Do you mean that the header data already contains sorting state so there's the possibility for this PR to store data conflicting with the header state?

What will be the rule of thump, which storage is used when?

I agree conceptually it's awkward to have two different places to store key/value configuration data. I think the rule of thumb is that the library settings table is for storing library-specific configuration data. It's nice that a user can copy their mixxxdb.sqlite to a different profile and still have a working library. So things like the current schema version absolutely have to go in SettingsDAO, but things like the preferred column sort order is less clear-cut. I'd be OK with the rule of thumb being "all library-specific preferences go in the library settings table".

Ok. that sounds reasonable.

Do you mean that the header data already contains sorting state so there's the possibility for this PR to store data conflicting with the header state?

No, because we need here the sorting hierarchy.

@jmigual: Would you mind to put this rule of thump as comment into

mixxx/src/library/dao/settingsdao.h

Thank you.

we can't really tell what Qt is storing in there

I thought I rewrote the header-saving code and got rid of the old QT-binary blob? So that upgrading is easier.

rryan · 2016-05-11T19:34:30Z

Travis fails unrelated to this PR:

The job exceeded the maxmimum time limit for jobs, and has been terminated.

Yea I think I caused this when I enabled asan on Travis builds for OS X. I think we should make the tests run faster -- they take almost a minute on my machine.

rryan · 2016-05-11T19:38:00Z

Travis fails unrelated to this PR:

The job exceeded the maxmimum time limit for jobs, and has been terminated.
Yea I think I caused this when I enabled asan on Travis builds for OS X. I think we should make the tests run faster -- they take almost a minute on my machine.

I reverted my asan commit.

daschuer · 2016-05-11T20:03:48Z

src/library/basesqltablemodel.h

+        }
+
+        friend QTextStream & operator << (QTextStream &out, SortColumn &sc) {
+            out << sc.m_column << " ";


I wonder why we have here a space and not in the reading code above.

This is because QTextStream skips spaces when reading but you need to set spaces when writing (like it happens normally in cin and cout)

Set sDebug to false

daschuer · 2016-05-12T19:48:06Z

src/library/basesqltablemodel.cpp

+            QString name;
+
+            in >> name >> ordI;
+            int col = this->fieldIndex(name) + m_tableColumns.size() - 1;


this-> is redundant

daschuer · 2016-05-12T20:56:28Z

With this patch it happens, that the table is emptied.
When I click on the preview coloumn thhan cover than artist,
the track table is empty
When I click on album and artist, it comes back.
Could this be related to the -1 column issue?

jmigual · 2016-05-12T21:02:16Z

Yes it is related to the -1 column issue, I'll fix it now. By the way how can you view this with mixxx?

VALUES ("mixxx.db.model.playlist.ColumnsSorting"," 1 -1 -1 ","0","0");

daschuer · 2016-05-12T21:03:41Z

I use the Firefox SqLite Manager Add-on

daschuer · 2016-05-17T11:20:33Z

src/library/basesqltablemodel.cpp

+
+        QString name;
+        if (sc.m_column > 0 && sc.m_column < m_tableColumns.size()) {
+            name = m_tableColumns[column];


Typo?
name = m_tableColumns[sc.m_column];

Yes it's a typo, I'll change it now

daschuer · 2016-05-20T15:06:15Z

src/library/basesqltablemodel.cpp

+    // Write new sortColumns order to user settings
+    QString val;
+    QTextStream out(&val);
+    qDebug() << m_tableColumns;


This seams to be an intermediate debug. Remove?

daschuer · 2016-05-20T15:09:23Z

Thank you. There is only a single debug leftover and than LGTM.
(Note: Since GitHub does not notify us for new commits, please leave a comment every time you wish a review)

jmigual · 2016-05-20T15:14:59Z

Ok, fixed. I didn't know GitHub does not notify users so I'll take into account.

daschuer · 2016-05-21T19:28:04Z

Thank you :-)

jmigual added 6 commits May 6, 2016 22:02

First steps

d643d7f

Add c++11 style code

aa077cc

Add load option for sorting

ca9b478

Merged branch master into bug/rememberLibrarySorting

8613b17

Add string generation to save

f9e09f5

Finish library sorting remember

2eec030

rryan reviewed May 11, 2016
View reviewed changes

daschuer reviewed May 11, 2016
View reviewed changes

jmigual added 3 commits May 11, 2016 22:15

Changed sorting to names instead of integers

96f8cc8

Set sDebug to false

Add empty method for better readability

103d417

Added rule of thump

49987c8

daschuer reviewed May 12, 2016
View reviewed changes

jmigual added 2 commits May 13, 2016 00:11

Respect coding guidelines

aa16dcf

Fix getting wrong name when selecting a tableColumn

5be2dc3

daschuer reviewed May 17, 2016
View reviewed changes

Fix typo, error in loading saved columns and add comment

638a52e

daschuer reviewed May 20, 2016
View reviewed changes

Remove intermediate debug

ddd2396

daschuer merged commit 4008e11 into mixxxdj:master May 21, 2016

jmigual deleted the bug/rememberLibrarySorting branch May 23, 2016 19:27

mixxxbot mentioned this pull request Aug 22, 2022

remember library sorting across restarts #8151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: remember library sorting #947

Bug: remember library sorting #947

jmigual commented May 11, 2016

rryan May 11, 2016

daschuer commented May 11, 2016

rryan May 11, 2016 •

edited

jmigual May 11, 2016

daschuer May 11, 2016

jmigual May 11, 2016

rryan May 11, 2016

rryan May 11, 2016

daschuer May 11, 2016

daschuer May 11, 2016

jmigual May 12, 2016

ywwg May 12, 2016

rryan commented May 11, 2016

rryan commented May 11, 2016

daschuer May 11, 2016

jmigual May 11, 2016

daschuer May 12, 2016

daschuer commented May 12, 2016

jmigual commented May 12, 2016

daschuer commented May 12, 2016

daschuer May 17, 2016

jmigual May 17, 2016

daschuer May 20, 2016

daschuer commented May 20, 2016

jmigual commented May 20, 2016

daschuer commented May 21, 2016

Bug: remember library sorting #947

Bug: remember library sorting #947

Conversation

jmigual commented May 11, 2016

Choose a reason for hiding this comment

daschuer commented May 11, 2016

rryan May 11, 2016 • edited

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

rryan commented May 11, 2016

rryan commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 12, 2016

jmigual commented May 12, 2016

daschuer commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 20, 2016

jmigual commented May 20, 2016

daschuer commented May 21, 2016

rryan May 11, 2016 •

edited