Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow key file tags with tuning information like "A#m +50" #10992

Merged
merged 7 commits into from
Feb 8, 2023
Merged

Allow key file tags with tuning information like "A#m +50" #10992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
58e8978
Allow file tags with tuning information written by Rapid Evolution
daschuer Oct 23, 2022
acbc16b
Improve readability
daschuer Oct 23, 2022
157af7d
Allow Lancelot notation with a leading zero
daschuer Oct 23, 2022
87157a3
Add Tests from RapidEvolution
daschuer Oct 23, 2022
cf381d3
Allow any numbers of leading zeros in Lancelot format.
daschuer Feb 8, 2023
3bd8cd9
Explain RapidEvolution Key Code format
daschuer Feb 8, 2023
4832307
Use more const in guessKeyFromText()
daschuer Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/test/keyutilstest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ TEST_F(KeyUtilsTest, LancelotNotation) {

TEST_F(KeyUtilsTest, KeyNameNotation) {
// Invalid letter
// (actually valid in traditional german notation where B is H and Bb is B -
// (actually valid in traditional German notation where B is H and Bb is B -
// everyone confused?)
EXPECT_EQ(mixxx::track::io::key::INVALID,
KeyUtils::guessKeyFromText("H"));
Expand Down Expand Up @@ -142,6 +142,47 @@ TEST_F(KeyUtilsTest, KeyNameNotation) {
KeyUtils::guessKeyFromText("cb"));
EXPECT_EQ(mixxx::track::io::key::C_MINOR,
KeyUtils::guessKeyFromText("b#"));

// Rapid Evolution test cases
EXPECT_EQ(mixxx::track::io::key::A_MINOR,
KeyUtils::guessKeyFromText("Am"));
EXPECT_EQ(mixxx::track::io::key::A_MINOR,
KeyUtils::guessKeyFromText("08A"));
EXPECT_EQ(mixxx::track::io::key::B_FLAT_MINOR,
KeyUtils::guessKeyFromText("A#m"));
EXPECT_EQ(mixxx::track::io::key::B_FLAT_MINOR,
KeyUtils::guessKeyFromText("Bbm"));
EXPECT_EQ(mixxx::track::io::key::A_MAJOR,
KeyUtils::guessKeyFromText("11B"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("G#+50"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("Ab +50"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("Ab +50cents"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("04B +50cents"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("G#-50"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("Ab -50"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("Ab -50cents"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText("04B -50cents"));
EXPECT_EQ(mixxx::track::io::key::A_FLAT_MAJOR, // ionian
KeyUtils::guessKeyFromText(" 4b -50 cents "));
// 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 "));
Comment on lines +175 to +179
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be better to add another regular expression that implements the promiscuous rapid evolution format instead of manually trimming the it away?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess thats valid, lets just hope nobody does indeed export data like that just because its allowed in other software.

EXPECT_EQ(mixxx::track::io::key::INVALID, // ionian
KeyUtils::guessKeyFromText(" "));
EXPECT_EQ(mixxx::track::io::key::INVALID, // ionian
KeyUtils::guessKeyFromText(""));
EXPECT_EQ(mixxx::track::io::key::INVALID, // ionian
KeyUtils::guessKeyFromText("xyz"));
}

mixxx::track::io::key::ChromaticKey incrementKey(
Expand Down
12 changes: 10 additions & 2 deletions src/track/keyutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ using mixxx::track::io::key::ChromaticKey_IsValid;
static const QString s_openKeyPattern("^\\s*(1[0-2]|[1-9])([dm])\\s*$");

// Lancelot notation, the numbers 1-12 followed by a (minor) or b (major).
static const QString s_lancelotKeyPattern("^\\s*(1[0-2]|[1-9])([ab])\\s*$");
// This is also used to detect RapidEvolution Key Code format using a padding "0"
static const QString s_lancelotKeyPattern("^\\s*0*(1[0-2]|[1-9])([ab])\\s*$");

// a-g followed by any number of sharps or flats, optionally followed by
// a scale spec (m = minor, min, maj)
Expand Down Expand Up @@ -248,7 +249,14 @@ QString KeyUtils::getGlobalKeyText(const Keys& keys, KeyNotation notation) {

// static
ChromaticKey KeyUtils::guessKeyFromText(const QString& text) {
QString trimmed = text.trimmed();
// Remove Shift (Tuning) Information used by Rapid Evolution like: "A#m +50";
int shiftStart = text.indexOf('+');
if (shiftStart < 0) {
shiftStart = text.indexOf('-');
}
const QString trimmed =
shiftStart >= 0 ? text.left(shiftStart).trimmed() : text.trimmed();

if (trimmed.isEmpty()) {
return mixxx::track::io::key::INVALID;
}
Expand Down
4 changes: 2 additions & 2 deletions src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,9 @@ void Track::importMetadata(
// overwriting with an inconsistent value. The bpm must always be
// set together with the beat grid and the key text must be parsed
// and validated.
const auto importedBpm = importedMetadata.getTrackInfo().getBpm();
const mixxx::Bpm importedBpm = importedMetadata.getTrackInfo().getBpm();
importedMetadata.refTrackInfo().setBpm(getBpmWhileLocked());
const auto importedKeyText = importedMetadata.getTrackInfo().getKey();
const QString importedKeyText = importedMetadata.getTrackInfo().getKey();
importedMetadata.refTrackInfo().setKey(m_record.getMetadata().getTrackInfo().getKey());

bool modified = false;
Expand Down