fix(Chords): Fix rare error with chords having accidentals on the wrong note (#944)

fix #944

Vexflow needs the notes (keys) sorted in the reverse order OSMD likes to have them originally,
and in most cases we had them sorted correctly for Vexflow,
but for some cases like chords the sorting could still be wrong.

src/MusicalScore/Graphical/GraphicalVoiceEntry.ts

@@ -31,12 +31,34 @@ export class GraphicalVoiceEntry extends GraphicalObject {
      * Notes need to be sorted for Vexflow StaveNote creation.
      * Note that Vexflow needs the reverse order, see VexFlowConverter.StaveNote().
      */
-    public sort(): void {
+    public sort(): GraphicalNote[] {
         this.notes.sort((a, b) => {
             return b.sourceNote.Pitch.getHalfTone() - a.sourceNote.Pitch.getHalfTone();
         });
+        // note that this is the reverse order of what vexflow needs
+        return this.notes;
     }
 
+    /** Sort notes for vexflow (bottom to top), which needs them in the reverse order OSMD likes to have them.
+     *  Note that sort() and reverse() replace the array in place,
+     *  so to avoid changing the array one could copy it first, see sortedNotesCopyForVexflow() (commented),
+     *  though copying the array is also unnecessary (time+memory) for now.
+     */
+    public sortForVexflow(): GraphicalNote[] {
+        this.notes.sort((a, b) => {
+            return a.sourceNote.Pitch.getHalfTone() - b.sourceNote.Pitch.getHalfTone();
+        });
+        return this.notes;
+    }
+
+    // probably unnecessary, can just go through the array in reverse
+    // public sortedNotesCopyForVexflow(): GraphicalNote[] {
+    //     // we need a copy since sort replaces the array (in place sorting)
+    //     let sortedArray = Array.from(this.notes.sort());
+    //     sortedArray.reverse();
+    //     return sortedArray;
+    // }
+
     /** (Re-)color notes and stems
      */
     public color(): void {

src/MusicalScore/Graphical/VexFlow/VexFlowConverter.ts

@@ -197,13 +197,17 @@ export class VexFlowConverter {
      * @returns {Vex.Flow.StaveNote}
      */
     public static StaveNote(gve: GraphicalVoiceEntry): Vex.Flow.StaveNote {
-        // sort notes
-        /* seems unnecessary for now
-        if (gve.octaveShiftValue !== undefined && gve.octaveShiftValue !== OctaveEnum.NONE) {
-            gve.sort(); // gves with accidentals in octave shift brackets can be unsorted
-        } */
-        // VexFlow needs the notes ordered vertically in the other direction:
-        const notes: GraphicalNote[] = gve.notes.reverse();
+        // if (gve.octaveShiftValue !== OctaveEnum.NONE) { // gves with accidentals in octave shift brackets can be unsorted
+        gve.sortForVexflow(); // also necessary for some other cases, see test_sorted_notes... sample
+        //   sort and reverse replace the array anyways, so we might as well directly sort them reversely for now.
+        //   otherwise we should copy the array, see the commented GraphicalVoiceEntry.sortedNotesCopyForVexflow()
+        //   another alternative: don't sort gve notes, instead collect and sort tickables in an array,
+        //     then iterate over the array by addTickable() in VexFlowMeasure.graphicalMeasureCreatedCalculations()
+        const notes: GraphicalNote[] = gve.notes;
+        // for (const note of gve.notes) { // debug
+        //     const pitch: Pitch = note.sourceNote.Pitch;
+        //     console.log('note: ' + pitch?.ToString() + ', halftone: ' + pitch?.getHalfTone());
+        // }
         const rules: EngravingRules = gve.parentStaffEntry.parentMeasure.parentSourceMeasure.Rules;
 
         const baseNote: GraphicalNote = notes[0];

src/MusicalScore/Graphical/VexFlow/VexFlowMeasure.ts

@@ -1126,7 +1126,8 @@ export class VexFlowMeasure extends GraphicalMeasure {
                         resolution: Vex.Flow.RESOLUTION,
                     }).setMode(Vex.Flow.Voice.Mode.SOFT);
 
-            const restFilledEntries: GraphicalVoiceEntry[] =  this.getRestFilledVexFlowStaveNotesPerVoice(voice);
+            const restFilledEntries: GraphicalVoiceEntry[] = this.getRestFilledVexFlowStaveNotesPerVoice(voice);
+                    // .sort((a,b) => a.)
             // create vex flow voices and add tickables to it:
             for (const voiceEntry of restFilledEntries) {
                 if (voiceEntry.parentVoiceEntry) {

test/data/test_sorted_notes_chord_vexflow_keys_order.musicxml

@@ -0,0 +1,72 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 3.1 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">
+<score-partwise version="3.1">
+  <work>
+    <work-title></work-title>
+  </work>
+  <movement-title></movement-title>
+  <!-- Test sample: sorted notes (chord) vexflow keys order).
+  movement title etc intentionally empty for variety in samples -->
+  <identification>
+    <encoding>
+      <software>Dorico 3.5.0.1020</software>
+      <encoding-date>2021-01-01</encoding-date>
+    </encoding>
+  </identification>
+  <part-list>
+    <score-part id="P3">
+      <part-name>CHORD</part-name>
+    </score-part>
+  </part-list>
+  <part id="P3">
+    <measure number="1">
+      <attributes>
+        <divisions>4</divisions>
+        <key number="1">
+          <fifths>0</fifths>
+          <mode>none</mode>
+        </key>
+        <staves>1</staves>
+        <clef number="1">
+          <sign>G</sign>
+          <line>2</line>
+        </clef>
+      </attributes>
+      <note>
+        <pitch>
+          <step>F</step>
+          <alter>1</alter>
+          <octave>5</octave>
+        </pitch>
+        <duration>16</duration>
+        <voice>1</voice>
+        <type>whole</type>
+        <accidental>sharp</accidental>
+        <stem>down</stem>
+        <staff>1</staff>
+      </note>
+      <note>
+        <chord/>
+        <pitch>
+          <step>B</step>
+          <octave>4</octave>
+        </pitch>
+        <duration>16</duration>
+        <voice>1</voice>
+        <type>whole</type>
+        <stem>down</stem>
+        <staff>1</staff>
+      </note>
+      <note>
+        <rest/>
+        <duration>16</duration>
+        <voice>1</voice>
+        <type>whole</type>
+        <staff>1</staff>
+      </note>
+      <barline location="right">
+        <bar-style>light-heavy</bar-style>
+      </barline>
+    </measure>
+  </part>
+</score-partwise>