Tuning constructor defaults to 8 strings?

[ronyeh]

vexflow/src/tuning.ts

Lines 25 to 28 in a3ac41e

	constructor(tuningString = 'E/5,B/4,G/4,D/4,A/3,E/3,B/2,E/2') {
	// Default to standard tuning.
	this.setTuning(tuningString);
	}

The default tuningString is E/5,B/4,G/4,D/4,A/3,E/3,B/2,E/2 which looks like the shape of a standard tuning of a 6-string guitar but with two extra bass strings at B/2 and E/2. Is this a bug? The comment suggests it should default to standard tuning.

Additionally, the guitar tunings seem to be one octave higher than I expect.

vexflow/src/tuning.ts

Lines 11 to 19 in a3ac41e

	static get names(): Record<string, string> {
	return {
	standard: 'E/5,B/4,G/4,D/4,A/3,E/3',
	dagdad: 'D/5,A/4,G/4,D/4,A/3,D/3',
	dropd: 'E/5,B/4,G/4,D/4,A/3,D/3',
	eb: 'Eb/5,Bb/4,Gb/4,Db/4,Ab/3,Db/3',
	standardBanjo: 'D/5,B/4,G/4,D/4,G/5',
	};
	}

Shouldn't standard tuning go from E/2 to E/4?

C/4 is on string 2, fret 1 of a 6-string guitar with standard tuning.

I'm in the middle of cleaning up the migration/tests PR and I found what I thought was an incorrect test case comment:

vexflow/tests/tuning_tests.ts

Lines 23 to 27 in a3ac41e

	try {
	tuning.getValueForString(9);
	} catch (e) {
	equal(e.code, 'BadArguments', 'String 7');
	}

When I change the 9 to a 7 to match the comment, the test case fails.... since the standard tuning in VexFlow actually has 8 strings!

Additionally, the .getValueForString() method doesn't do what I initially expected:

vexflow/tests/tuning_tests.ts

Lines 29 to 34 in a3ac41e

	equal(tuning.getValueForString(6), 40, 'Low E string');
	equal(tuning.getValueForString(5), 45, 'A string');
	equal(tuning.getValueForString(4), 50, 'D string');
	equal(tuning.getValueForString(3), 55, 'G string');
	equal(tuning.getValueForString(2), 59, 'B string');
	equal(tuning.getValueForString(1), 64, 'High E string');

The values are set by:

vexflow/src/tuning.ts

Lines 30 to 33 in a3ac41e

	/** Return the note number associated to the note string. */
	noteToInteger(noteString: string): number {
	return Flow.keyProperties(noteString).int_value;
	}

It looks like C/5 is mapped to value 60, so my assumption is that this is using the MIDI convention of # 60 == Middle C.

It is also assuming that C/5 == Middle C, so that would make the octave numbers of the "standard tuning" make more sense.

However, in other parts of VexFlow, C/4 is middle C, which corresponds with scientific pitch notation.

In summary, Tuning probably has a small bug in the constructor, and it either needs extra comments to explain that middle C == C/5 == note value 60, or it needs to somehow be reworked such that middle C is C/4, like in StaveNote.