Skip to content

Chore/implement pitch helpers#5869

Open
vanshika2720 wants to merge 2 commits intosugarlabs:masterfrom
vanshika2720:chore/implement-pitch-helpers
Open

Chore/implement pitch helpers#5869
vanshika2720 wants to merge 2 commits intosugarlabs:masterfrom
vanshika2720:chore/implement-pitch-helpers

Conversation

@vanshika2720
Copy link
Contributor

Overview

The getPitchInfo function in MusicBlocks has been refactored and enhanced to correctly handle a wide range of pitch strings, including multi-digit octaves, negative octaves, and double accidentals, including Unicode symbols.

A new helper _parse_pitch_string was introduced to modularize parsing logic, making the code cleaner, more maintainable, and robust.


Key Changes

  1. ACCIDENTAL_MAP Constant

    • Maps accidentals to semitone offsets:
      '#' / '♯' → +1
      'b' / '♭' → -1
      'x' / '𝄪' → +2  (double-sharp)
      '𝄫'       → -2  (double-flat)
      
  2. _parse_pitch_string Helper

    • Parses pitch strings like "C4", "A#10", "F𝄪5".
    • Supports:
      • Multi-digit octaves
      • Negative octaves
      • Unicode accidentals
      • Accidental accumulation (e.g., double-sharps/flats)
    • Returns { name, octave, pitchNumber }.
  3. _calculate_pitch_number Update

    • Correctly computes pitch numbers for all accidentals.
    • Handles double accidentals by adding ±2 semitones directly.
  4. Bug Fixes

    • Fixed incorrect pitch numbers for previously unsupported inputs.
    • Edge cases now correctly handled:
      getPitchInfo('D𝄫-1') // { name: 'D𝄫', octave: -1, pitchNumber: 0 }
      getPitchInfo('E##4')  // { name: 'E𝄪', octave: 4, pitchNumber: 66 }
      getPitchInfo('Fx10')  // { name: 'F𝄪', octave: 10, pitchNumber: 139 }
      getPitchInfo('Gb-1')  // { name: 'Gb', octave: -1, pitchNumber: 6 }
      getPitchInfo('F𝄪5')  // { name: 'F𝄪', octave: 5, pitchNumber: 79 }
      getPitchInfo('B𝄫5')  // { name: 'B𝄫', octave: 5, pitchNumber: 81 }
  5. Tests

    • 259 automated tests pass.
    • Edge cases for multi-digit, negative octaves, and double accidentals verified.

Verification

To test the fix locally:

console.assert(getPitchInfo('D𝄫-1').pitchNumber === 0, "D𝄫-1");
console.assert(getPitchInfo('E##4').pitchNumber === 66, "E##4");
console.assert(getPitchInfo('Fx10').pitchNumber === 139, "Fx10");
console.assert(getPitchInfo('Gb-1').pitchNumber === 6, "Gb-1");
console.assert(getPitchInfo('F𝄪5').pitchNumber === 79, "F𝄪5");
console.assert(getPitchInfo('B𝄫5').pitchNumber === 81, "B𝄫5");

## Fixes
- #5552 

@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
logo.test.js
logoconstants.test.js

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from 375adbd to 1db2ce1 Compare February 23, 2026 00:25
@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
musicutils.test.js
logo.test.js
synthutils.test.js
PitchActions.test.js
logoconstants.test.js

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from 1db2ce1 to fea6ab5 Compare February 23, 2026 00:27
@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
logo.test.js
logoconstants.test.js

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vyagh
Copy link
Contributor

vyagh commented Feb 23, 2026

browser test passes
the regex work in _parse_pitch_string is good and the edge case coverage is
second commit fixes unrelated test failures (logo, palette, logoconstants), try to avoid this.

Here's my concern:

  • Changing _calculate_pitch_number to take just (noteName, octave), you dropped tur.singer.pitchNumberOffset from the calculation, old version subtracted that offset.
    Now when someone uses the Set Pitch Number Offset block and then reads pitch number through the Pitch converter, the offset gets silently ignored. That needs to be restored in the legacy path.

  • also .replace("#", SHARP) only hits the first occurrence i.e. "F#Deleted # file is css path #5" would leave the second # unconverted and break the lookup. use .replaceAll() there. also found out a stale JSDoc block sitting above getPitchInfo now (the old one + your new one), just remove the old one.

Fix these and this should be good to go.

@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

logo.test.js
logoconstants.test.js

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from f191ee2 to b0c0224 Compare February 23, 2026 15:15
@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
logo.test.js
logoconstants.test.js

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from 4970fcf to b11ff8e Compare February 23, 2026 16:20
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from b11ff8e to 317a633 Compare February 23, 2026 16:54
@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
logo.test.js
logoconstants.test.js

1 similar comment
@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
logo.test.js
logoconstants.test.js

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from 603ded1 to 5a3ef85 Compare February 23, 2026 19:53
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the chore/implement-pitch-helpers branch from 5a3ef85 to b7c35be Compare February 23, 2026 19:59
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor Author

@walterbender

@vyagh
Copy link
Contributor

vyagh commented Feb 25, 2026

LGTM now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants