Add case-insensitive parsing for Lancelot key notation

[cr7pt0gr4ph7]

When a user types in key:5a into the search field, the intent is most likely to search for key:5A instead of doing a text-only search.

The only case where case-insensitive parsing is ambigous is for the d/m (OpenKey) and D/M (Lancelot) suffixes, where lowercase will imply OpenKey, and uppercase will imply Lancelot (due to the order in which guessKeyFromText checks for matches with the different notations).

[cr7pt0gr4ph7]

I know that case-insensitivity was intentionally removed when adding the advanced Lancelot notations "ILMDPC" (though I haven't yet found a document describing which scales these are associated with), but this has caused a sub-par user experience:

Typing key:2a into the search field will fail to parse the provided key notation, and fall back to a case-insensitive text search in the key column instead, which will then return results that are nearly correct:

• ✔️ Searching for e.g. key:5a will only turn up results with key == 5A.
• ❌ Searching for key:2a will turn up results with key == 2A || key == 12A
• ❌ Fuzzy notation ~key:5a will not work, while ~key:5A does.

Footnote 1: We can avoid the ambiguity issue for d/m (OpenKey) vs. D/M (Lancelot) by depending on the lowercase/uppercase distinction there, while still providing case-insensitivity for the more common A/B (/ a/b) (Lancelot) suffix.

Footnote 2: Every other searchable field is case insensitive, so the case sensitivity for the special case of key: (as long as the case does not carry actual information) is even more suprising.

Footnote 3: While the more flexible parsing is definitely desirable for search queries, I am not 100% sure about e.g. metadata parsing - though I think that even here, a bit of informed guessing is preferable over returning no data at all.

[acolombier]

Thanks for this nice improvement! Some minor change request but otherwise good to go

[acolombier]

It might be easier to read to us toLower() on line 462. toLower will effectively make a single subtraction, while doubling the size might significantly decrease the find performance. Likely invisible on our modern CPU, but we need to remain mindful of the slimer specs such as old RPi 😄

[cr7pt0gr4ph7]

@acolombier

It might be easier to read to us toLower() on line 462. toLower will effectively make a single subtraction, while doubling the size might significantly decrease the find performance. Likely invisible on our modern CPU, but we need to remain mindful of the slimer specs such as old RPi 😄

We could also use a binary tree hash for more efficient matching, or even a bloom filter. Okay, I'm just joking now 😄 Converted the code to use toUpper() instead. Thanks! 👍

Thanks for this nice improvement! Some minor change request but otherwise good to go.

I originally wanted to implement ~key:5a matching - just to find out after one year of using Mixxx that it's actually already present, and it just wasn't parsing 5a as expected...

[cr7pt0gr4ph7]

On a more serious note, after thinking about parsing of the d/D and m/M prefix: As a user typing in a search query, I would probably still expect 5d to be parsed using Lancelot notation when I have selected KeyUtils::KeyNotation::Lancelot or KeyUtils::KeyNotationLancelotAndTraditional as the default key notation in the settings.

(And, vice versa, 5D to be parsed as 5d when KeyUtils::KeyNotation::OpenKey is selected).

What do you think?

[daschuer]

The additionals modes have been introduced by Rapid Ecolution. See:
https://www.myqnap.org/product/rapid-evolution-3/
https://github.com/djqualia/RapidEvolution3/blob/6ef912c1843bf5201068973434237bb378c5e02f/src/com/mixshare/rapid_evolution/music/key/Key.java#L56

[acolombier]

I originally wanted to implement ~key:5a matching - just to find out after one year of using Mixxx that it's actually already present, and it just wasn't parsing 5a as expected...

I was looking at this some time ago and realise the search feature was actually quite complete! I think our UI doesn't give it good usability, but this is something as part of the new Mixxx design :)

I would probably still expect 5d to be parsed using Lancelot notation when I have selected KeyUtils::KeyNotation::Lancelot or KeyUtils::KeyNotationLancelotAndTraditional as the default key notation in the settings.

I agree - I feel like search should allow you to use any key annotation. independently of the display preference. Searching for 4d wile having Lancelot display selected, should still work, even if the results display 11b in the Key column. Ideally, the column should provide context about the search input and display both the preferred annotation + the input one (e.g 11b (4d)), but I know working with our library model is quite challenging, so I'm not sure this small improvement is worth the effort.

Alternatively, we could display a warning message and CTA to update the selected key notation in one click.

Appreciate this may become a fairly large change, so happy to see this coming as part of a follow up PR if you would prefer!

[daschuer]

And, vice versa, 5D to be parsed as 5d when KeyUtils::KeyNotation::OpenKey is selected

I think we can very much assume that the usere searches for the notation he also sees in the library. We just to need to be carefull to not mess with metadata read form the file tags. Here we can't use this assumption.

[cr7pt0gr4ph7]

@daschuer

The additionals modes have been introduced by Rapid Ecolution. See: https://www.myqnap.org/product/rapid-evolution-3/ https://github.com/djqualia/RapidEvolution3/blob/6ef912c1843bf5201068973434237bb378c5e02f/src/com/mixshare/rapid_evolution/music/key/Key.java#L56

Thanks! Googling for lancelot key notation did not turn up any relevant results at all.

For future reference, this page contains a reference on what the "advanced" Lancelot modes actually mean:

Basic key notation

• A - minor
• B - major

Advanced key notation
– A – Aeolian (natural minor)
– I – Ionian (normal major, normally B)
– D – Dorian
– P – Phrygian
– L – Lydian
– M – Mixolydian
– C – Locrian

[cr7pt0gr4ph7]

@acolombier

Appreciate this may become a fairly large change, so happy to see this coming as part of a follow up PR if you would prefer!

Yes, I think that this PR is good to go as-is (provided you also agree) as it's a 99.9% solution (because, although there is some infrastructure in place for parsing different scale modes like Dorian, Phrygian etc., this information is not currently stored in the database, and simply discarded).

@daschuer

I think we can very much assume that the user searches for the notation he also sees in the library.

The other table columns all use case-insensitive text matching of what the user can visually see, so its very reasonable for key to adhere to that as close as possible.

We just to need to be carefull to not mess with metadata read form the file tags. Here we can't use this assumption.

I agree. My thought was that there should probably be two parsing modes:

• A strict one were the parsing mode is always deterministic and non-configurable[^1] (i.e. the current guessKeyFromText).
• A fuzzy one where an expected key notation can be passed in, to be used in places were we coerce user input into our internal representation (but not for parsing track metadata).
  Custom key notations might require some more consideration, as these should IMO probably be parsed in a case-insensitive as well, except for case is required to distinguish two different keys.

The necessary infrastructure already seems to be in place for this, as e.g. TrackRecord::updateGlobalKeyNormalizeText already receives a source parameter that can have the values mixxx::track::io::key::Source::ANALYZER, FILE_METADATA and USER, and we could just pass that on to guessKeyFromText.

[Eve00000]

@cr7pt0gr4ph7
while you're taking a look at the searchLineEdit, can you also take a look at src/library/trackset/smarties/smartiesfeaturehelper.cpp in #14182 ,
in this file the conversion from searchLineEdit to smarties (smartcrates) is done: create the conditions to save the search.
So I would realy appreciate it if you could enter the updated search for keys there to.

[acolombier]

Code LGTM, thanks you!

[cr7pt0gr4ph7]

@Eve00000

@cr7pt0gr4ph7 while you're taking a look at the searchLineEdit, can you also take a look at src/library/trackset/smarties/smartiesfeaturehelper.cpp in #14182 , in this file the conversion from searchLineEdit to smarties (smartcrates) is done: create the conditions to save the search. So I would realy appreciate it if you could enter the updated search for keys there to.

I had a look at #14182, and it seems to me like SmartiesFeatureHelper::createEmptySmartiesFromSearch basically duplicates SearchQueryParser::parseQuery (but not completely, some features like e.g. ~key:5A which should match all compatible keys, i.e. 4A, 5A, 5B and 6A, are missing).

I can't think of a good reason for why the search syntax should differ between Smarties/SmartCrates vs. the searchLineEdit (and for why that would not be really confusing for users). Given that assumption, I don't see why it shouldn't be possible possible to use SearchQueryParser::parseQuery to parse the queries to a QueryNode AST, and then turn that AST into the format required for the smart crates. If you go that route, you automatically get this as well as all future improvements to search query parsing.