Allow key file tags with tuning information like "A#m +50" #10992
Conversation
|
This is open since three month without interest to test and review. I will remove it form the 2.3 milestone. |
|
So, this just strips the additional tuning info so Mixxx can recognise and import the base key? |
| // Mixxx does not allow this but Rapid Evolution | ||
| // EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian | ||
| // KeyUtils::guessKeyFromText(" g # - 50 cents ")); | ||
| // EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian | ||
| // KeyUtils::guessKeyFromText(" g # + 50 cents ")); |
There was a problem hiding this comment.
wouldn't it be better to add another regular expression that implements the promiscuous rapid evolution format instead of manually trimming the it away?
There was a problem hiding this comment.
Your comment is at the disabled test. Mixxx fails to pars that because of too many blanks. I do not consider the test data as realistic and failing is OK or even desired.
The tuning is trimmed away, because Mixxx does not make use of it right now.
The next iteration of making Mixxx tunig aware is to store the tuning information in the database. This is implemented here:
#11096 From this we can introduce the next iteration of making it tuning aware.
There was a problem hiding this comment.
Yes thats exactly what I mean, if rapid evolution allows / exports data with that many blanks, we should accept it. Whats your reasoning for concluding that its not realistic?
Stripping the actual tuning information is fine for now imo.
There was a problem hiding this comment.
This is artificial data for a full test coverage of the Rapid Evolution key parser. No one exports the data like that.
Since the Mixxxx parser works differently this test is IMHO pointless for us.
There was a problem hiding this comment.
I guess thats valid, lets just hope nobody does indeed export data like that just because its allowed in other software.
src/track/keyutils.cpp
Outdated
| // Remove Shift (Tuning) Information used by Rapid Evolution like: "A#m +50"; | ||
| int shiftStart = text.indexOf('+'); | ||
| if (shiftStart < 0) { | ||
| shiftStart = text.indexOf('-'); | ||
| } | ||
| if (shiftStart >= 0) { | ||
| text = text.left(shiftStart); | ||
| } | ||
| const QString trimmed = text.trimmed(); |
There was a problem hiding this comment.
I'd personally favor to change the regex's instead, but this is fine as stopgap solution if you insist.
There was a problem hiding this comment.
Yes, once we are aware of tuning this needs to be replaced.
While the tuning issue exists, luckily most tracks are at A = 440 Hz. So this is something that is only reasonable with detector support like in Rapid Evolution.
A prominent out of tune Track is "Coldplay - The Scientist" which is recorded with an out of tune piano at A = 446 Hz
https://www.reddit.com/r/Coldplay/comments/8tjhwy/nearly_every_song_on_arobth_is_out_of_tune_or_not/
src/track/keyutils.cpp
Outdated
| ChromaticKey KeyUtils::guessKeyFromText(const QString& text) { | ||
| QString trimmed = text.trimmed(); | ||
| ChromaticKey KeyUtils::guessKeyFromText(QString text) { |
There was a problem hiding this comment.
Why the change from const-ref to by value?
There was a problem hiding this comment.
Because text is no longer const and this function is used with l-values like
KeyUtils::guessKeyFromText("H")
There was a problem hiding this comment.
Sure, if you think reassigning in the body is necessary, its no longer const. It could probably made const again though once that code has been replaced.
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
|
Done |
|
Thank you. |
This is written by Rapid Evolution