paulrosen / abcjs

javascript for rendering abc music notation
Other
1.94k stars 285 forks source link

getBpm issues: documentation; d.ts; and so result of `renderAbc(...)[0].getBpm()` #1040

Closed AGBrown closed 2 months ago

AGBrown commented 3 months ago

environment:

When doing a test as follow:

abc (click to expand):

``` %abc-2.1 X: 0 T: grand staff bpm test Q: 1/4=80 M: 3/4 L: 1/8 V:R clef=treble V:L clef=bass K: C [V:R] [K: octave=-1] \ z2 z e cB | A2 [Acea] e cB | \ A2 [Acea] z z2 | [V:L] \ z2 z E CB, | [K: octave=-2] A2 [Ge] [K: octave=-1] E CB, | \ [K: octave=-2] A2 [^Fe] z z2 | ```

const renderOut = renderAbc(elem, tune.abc, {
  add_classes: true,
  responsive: 'resize',
} as AbcVisualParams);
const tuneObj = renderOut[0];

const bpm = tuneObj.getBpm();

Expected result: bpm === 80 Actual result: bpm === 180

From what I can work out...

  1. From https://github.com/search?q=repo%3Apaulrosen%2Fabcjs+getBpm&type=code it looks like getBpm has an optional tempo parameter, and all calls to it should supply tempo from const tempo = tune.metaText ? tune.metaText.tempo : null; if they want to get the actual tempo for the tune (e.g. abc_timing_callbacks).
  2. But getBpm in the d.ts doesn't show this optional parameter, so when you use the getBpm method in typescript the parameter ends up being undefined. This means the result ends up as the default value of 180, as per abc_tune.
  3. This ties in to the documentation which says

    This is the starting beats per minute. Tempo changes could appear later in the tune, but this is the value that was set with the Q: statement, or if that statement doesn't exist, it is the default tempo of 180.

    which implies that we don't need to supply the tempo metaText to this function, when it looks like we do?

This all assumes I'm not incorrectly using renderAbc and getBpm from the elements of the renderAbc result.

paulrosen commented 2 months ago

Will be fixed in 6.4.3