sort keys by circle of fifths in library display

[Be-ing]

The order starts with C at the top, then its relative minor (a), then adds 1 sharp (G), then G's relative minor (e), and so adding sharps/removing flats around the circle of fifths.

Debugging output shows:
Warning [Main]: QString::arg: Argument missing: CASE WHEN key_id=1 THEN 1 WHEN key_id=22 THEN 2 WHEN key_id=8 THEN 3 WHEN key_id=17 THEN 4 WHEN key_id=3 THEN 5 WHEN key_id=24 THEN 6 WHEN key_id=10 THEN 7 WHEN key_id=19 THEN 8 WHEN key_id=5 THEN 9 WHEN key_id=14 THEN 10 WHEN key_id=12 THEN 11 WHEN key_id=21 THEN 12 WHEN key_id=7 THEN 13 WHEN key_id=16 THEN 14 WHEN key_id=2 THEN 15 WHEN key_id=23 THEN 16 WHEN key_id=9 THEN 17 WHEN key_id=18 THEN 18 WHEN key_id=4 THEN 19 WHEN key_id=13 THEN 20 WHEN key_id=11 THEN 21 WHEN key_id=20 THEN 22 WHEN key_id=6 THEN 23 WHEN key_id=15 THEN 24 END , key
Debug [Main]: LibraryTableModel(0x3732840) select() took 60 ms 4177

Is this warning okay? Would it be better to store the key_id to order mappings in a hash table and build the SQL string from that?

[daschuer]

Uh, this one is hard to maintain and I am also afraid this is not the best solution performance wise. (We have to verify this) Did you consider to register a custom sorting function?

We have already done this for umlauts here:
https://github.com/mixxxdj/mixxx/blob/master/src/library/trackcollection.cpp#L169

And have used this here:

mixxx/src/library/basesqltablemodel.cpp

Line 437 in 3f854c1

m_tableOrderBy.append(" COLLATE localeAwareCompare");

[Be-ing]

I had not considered that. I'll give it a try and compare the speed with gprof. Should I use a QMap for the callback function? If not, do you have a better suggestion?

[daschuer]

You have just register a compare function for two keys, the rest does sqlite for you.

The prototyp is similar to this:
int comp_func( void* udp, int sizeA, const void* textA,
int sizeB, const void* textB );

See slight docks for "sqlite3_create_collation()"

[Be-ing]

With n=20 sortings using the method in the first commit, LibraryTableModel select() execution time mean was 69.95ms with a standard deviation of 11.03ms.

I'm trying to write a custom collation function but I can't figure out how to debug it. I changed the QString sortCircleOfFifths in columcache.cpp to "(key_id COLLATE keysByCircleOfFifths)" and registered the custom callback function in trackcollection.cpp. However, sending anything to qDebug() (or std::cout) does not print anything in Mixxx's debug output.

The function is:

int TrackCollection::sqliteKeysByCircleOfFifths(void* pArg,
                                                int len1, const void* data1,
                                                int len2, const void* data2) {
    Q_UNUSED(pArg);
    static mixxx::track::io::key::ChromaticKey key1 = KeyUtils::keyFromNumericValue(
        QString::fromRawData(reinterpret_cast<const QChar*>(data1),
                             len1 / sizeof(QChar)).toDouble());
    static mixxx::track::io::key::ChromaticKey key2 = KeyUtils::keyFromNumericValue(
        QString::fromRawData(reinterpret_cast<const QChar*>(data2),
                             len2 / sizeof(QChar)).toDouble());

    static int OpenKeyComparison = KeyUtils::keyToOpenKeyNumber(key1) - KeyUtils::keyToOpenKeyNumber(key2);
    if (OpenKeyComparison != 0) {
        return OpenKeyComparison;
    }

    if (KeyUtils::keyIsMajor(key1) && !KeyUtils::keyIsMajor(key2)) {
        return 1;
    }
    return -1;
}

This doesn't seem to do anything; the keys are sorted chromatically as if the SQL query just included "SORT BY key_id" without the collation function.

[daschuer]

How does it compare to the original sorting?

IMHO qDebug() should work inside your function. Does it work in the sqliteLocaleAwareCompare compare function?

Does the new sorting function alread work?
If not, maybe there is a typo in "keysByCircleOfFifths" did you use the same name for registering sortCircleOfFifths?

[Be-ing]

qDebug does work inside sqliteLocaleAwareCompare, so I guess the SQL query isn't calling sqliteKeysByCircleOfFifths. Removing the parentheses around "(key_id COLLATE keysByCircleOfFifths)" makes no difference.

The whole SQL query being executed is:

SELECT id FROM library_cache_view
WHERE (id in ([giant list of every id in my library]))
ORDER BY key_id COLLATE keysByCircleOfFifths COLLATE localeAwareCompare ASC

[Be-ing]

Ah, the problem was that COLLATE only works on SQL strings. I had to use CAST(key_id AS TEXT) rather than just key_id.

Perhaps a custom SQL function (https://www.sqlite.org/c3ref/create_function.html ) would be more appropriate than a custom COLLATE function.

[Be-ing]

I got a working custom COLLATE function, but it is significantly slower than the CASE...WHEN...THEN sorting. Statistics with ~4000 tracks in my library, 2.3 GHz Core i5-2410M, 4GB RAM, running Fedora 22:
CASE (N=20):
M=69.95ms, SD=11.03ms
COLLATE (N=20):
M=94.0ms, SD=8.65ms

t=7.48, p<.001

The COLLATE function is:

int TrackCollection::sqliteKeysByCircleOfFifths(void* pArg,
                                                int len1, const void* data1,
                                                int len2, const void* data2) {
    Q_UNUSED(pArg);
    mixxx::track::io::key::ChromaticKey key1 = KeyUtils::keyFromNumericValue(
        QString::fromRawData(reinterpret_cast<const QChar*>(data1),
                             len1 / sizeof(QChar)).toDouble());
    mixxx::track::io::key::ChromaticKey key2 = KeyUtils::keyFromNumericValue(
        QString::fromRawData(reinterpret_cast<const QChar*>(data2),
                             len2 / sizeof(QChar)).toDouble());


    int OpenKeyComparison = KeyUtils::keyToOpenKeyNumber(key1) - KeyUtils::keyToOpenKeyNumber(key2);
    if (OpenKeyComparison != 0) {
        return OpenKeyComparison;
    }

    if (!KeyUtils::keyIsMajor(key1) && KeyUtils::keyIsMajor(key2)) {
        return 1;
    }
    return -1;
}

Is the improved readability and maintainability worth 24ms?

[Be-ing]

Apparently SQLite only supports using one COLLATE function per query because the custom COLLATE function did not work after I uncommented the lines in basesqltablemodel.cpp that append "COLLATE localeAwareCompare" to the query string. But now that I found the ChromaticKey type in the course of writing that custom COLLATE function, I converted the old list of numbers into an array of ChromaticKeys, which is much more readable (and just as fast as before).

[daschuer]

Thank you for the work and sorry for the wrong hints. Please add a comment about your findings to the code.

IMHO we can use this as default sorting. It there a single use case for the old sorting?
If not, we can just commit this and ditch the additional preference option.

Any thought or objections?

[Be-ing]

Thank you for the work and sorry for the wrong hints. Please add a comment about your findings to the code.

It was a good learning experience. :)

IMHO we can use this as default sorting. It there a single use case for the old sorting?
If not, we can just commit this and ditch the additional preference option.

Any thought or objections?

I don't see any use case for the old sorting (which was alphabetical). I understand the purpose of displaying keys in the library to be to help users select tracks with compatible keys, so the sorting method should put compatible keys next to each other. If a user wants to find tracks with a particular key, they can use the search box.

The only potential issue I see with this sorting is that the Lancelot notation system sorts with 7 at the top because it is centered at the opposite side of the circle of fifths (G#m) compared to every diagram of the circle of fifths that I have seen. Should I add an exception for that and sort using its number system? How can I read the user's configured notation system?

[Be-ing]

Whoops, accidentally closed the PR.

[daschuer]

you may use = m_pConfig->getValueString()
like here: https://github.com/mixxxdj/mixxx/blob/master/src/dlgprefkey.cpp#L120

[Be-ing]

How can I pass m_pConfig to ColumnCache's scope?

[daschuer]

You can pass it at the constructor by BaseTrackCache.
before you can retrieve it by pTrackCollection->getConfig.

All this m_pConfig passing is somehow annoying.

If you like to do some extra work, you can make m_pConfig a singleton or a member of MixxxApplication. And allow later just call ((MixxxApplication)qApp)->getConfig() from everywhere.

[daschuer]

Wait, With the outline solution we are Not able to catch change events. it is probably better to turn the property into A perasistant CO

[Be-ing]

Is catching change events necessary? What would the purpose of that be? Automatically resorting the library when the key notation is changed? That could be confusing.

I managed to make a persistent ControlObject for storing the preferred notation. However, I am at a loss for how to get the value to ColumnCache. I tried making the CO a private static member of DlgPrefKey, providing access to it with a public static member function of DlgPrefKey, and adding
ControlObject* DlgPrefKey::m_pCONotation = new ControlObject(ConfigKey(KEY_CONFIG_KEY, KEY_NOTATION), true, false, true);
to the top of dlgprefkey.cpp. However, regardless of the stored configuration value, the value of the CO is initialized to 0. If I don't make anything static, it works within DlgPrefKey, but that doesn't help get the value in ColumnCache.

[daschuer]

Is catching change events necessary? What would the purpose of that be? Automatically resorting the library when the key notation is changed? That could be confusing.

Will it be still confusing, if we do not the automatically resort, but use the new sort after the user clicks the header? This means, just prepare the new sorting clause.

Otherwise the user has to restart Mixxx to get 1 sorted on top, since setColumns() is called only one time.

Did you make a ControlObjectSlave to access the value form ColumnCache?
You have to be sure that the ControlObject is instantiated before the ControlObjectSlave.
If the instantiation in DlgPrefKey is to late you can use there a ControlObjectSlave too and move the
ControlObject to a pleas earlier.

[Be-ing]

Will it be still confusing, if we do not the automatically resort, but use the new sort after the user clicks the header? This means, just prepare the new sorting clause.

I'm not sure which way would be more intuitive.

Otherwise the user has to restart Mixxx to get 1 sorted on top, since setColumns() is called only one time.

That would be an issue.

Did you make a ControlObjectSlave to access the value form ColumnCache?
You have to be sure that the ControlObject is instantiated before the ControlObjectSlave.
If the instantiation in DlgPrefKey is to late you can use there a ControlObjectSlave too and move the ControlObject to a pleas earlier.

I'm not dealing with ColumnCache yet. The issue is that notation is reset on every startup. I confirmed that the value is being saved correctly by looking at my mixxx.cfg between closing Mixxx and restarting it. I'm guessing this means that the instantiation in DlgPrefKey is too late? I don't understand how this could be the case considering that it is only used in that one file so far.

[Be-ing]

I'm okay with merging this as-is for 1.12 and polishing the sorting with the Lancelot notation later.

[ywwg]

I'd prefer not to merge this change into 1.12. Let's do some more design work so we're sure how it should behave when the user changes the notation method with this column selected for sorting.

[Be-ing]

Given that the old sorting mechanism (alphabetical) doesn't really make sense for any use case, this is an improvement as-is. IMO the usefulness of adding key detection is really limited without this.

[ywwg]

I understand your position and I respectfully disagree, sorry! There is plenty of good functionality with key detection (key sync, library search, displaying the current key, etc) even without this sorting.

[daschuer]

I would like to have this a part of 1.12.

Here some references:
https://bugs.launchpad.net/mixxx/+bug/1435628
http://www.mixxx.org/forums/viewtopic.php?f=3&t=7066&p=24161

If you search the web for sorting by key you find only sorting examples by OpenKey or Lancelot.
IMHO there is no gain from the current cromatical sorting.
So we consider the current state as a bug.

If we switch to OpenKey sorting with this PR, we do not change the status = "Sorting only by one notation" but we make the sorting actually usable.
The scope of this PR is also very focussed, it is finish reviewed and tested.
Even if we need to revert this PR in favour of a new approach, it helps 1.12 users a lot in the current state. So lets merge it.

Any Thoughts?

[Be-ing]

To be clear, the present state does sort the Lancelot notation system in a meaningful way, but it is odd because it doesn't start with 1 on top. This sorting makes sense with both the traditional and OpenKey systems.

[daschuer]

Right, you sort by OpenKey which matches to Lancelot sorting with 8B on top wrapping at 7A.

[ywwg]

I made it clear when I offered to review patches for inclusion into 1.12 that I need to have the trust of other developers with respect to my decisions. I'm not interested in a long protracted argument about whether to include the feature or not. I have stated my position and I don't see any point in continuing the discussion.

[Be-ing]

I don't see any point in not merging this. Is there a risk of breaking anything? The current state of 1.12 without this is already broken. I don't really see it as a new feature, but rather partially fixing a broken existing feature (key sorting). On the other hand, whatever is changed in the future to support the Lancelot notation better would probably require more substantial changes that could potentially break something.

[Pegasus-RPG]

Please re-request this against master.

[rryan]

Hello! This branch is against the 1.12 branch, which is now closed to any changes except bugfixes. Please re-open your PR against master if this PR is still relevant.

[Be-ing]

I plan to return to this after the QSettings support is merged.