Skip to content

Conversation

@it-ony
Copy link

@it-ony it-ony commented Dec 10, 2025

Added support to show fingerings as top annotations.

image

Ideally annotations would have a type so the renderer in vexflow could introduce a config SHOW_FINGERING to only display the finger numbers, when configured.

But as this is my first contribution to the project and I didn't find a CONTRIBUTION.md so that's my best guess.

Copy link
Collaborator

@jaredjj3 jaredjj3 left a comment

Choose a reason for hiding this comment

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

Would you go to tests/integration/w3c-musicxml.test.ts

uncomment this line:

// { filename: 'fingering-element-notation.musicxml', width: 900 },

then run:

npm run test -- --updateSnapshot

to update the snapshots for fingering annotations?

If you run into any issues, check out https://github.com/stringsync/vexml?tab=readme-ov-file#development. If you don't have Docker or the time, let me know and I can do it after you merge.


static fromFingering(config: Config, log: Logger, musicXML: { fingering: musicxml.Fingering }): Annotation | undefined {
const number = musicXML.fingering.getNumber();
if (!number) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if finger 0 is valid (I guess semantics are up to the transcriber), but would you make this check slightly more robust checking presence with typeof number !== 'number'?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I think my linter changed it to the falsely notation. Will change it if course.

Copy link
Author

Choose a reason for hiding this comment

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

I went with if (number === null || number === undefined) { at the end as I've seen not everything coming from the xml is actual a number.

Therefore, I'd say

export class Fingering {
  constructor(private element: NamedElement<'fingering'>) {}

  /** Returns the fingering number. Defaults to null. */
  getNumber(): number | null {
    return this.element.content().int();
  }
}

needs adjustment as well to support all the optional attributes and not force conversion to int.

Checking the xsd

	<xs:complexType name="fingering">
		<xs:annotation>
			<xs:documentation>Fingering is typically indicated 1,2,3,4,5. Multiple fingerings may be given, typically to substitute fingerings in the middle of a note. The substitution and alternate values are "no" if the attribute is not present. For guitar and other fretted instruments, the fingering element represents the fretting finger; the pluck element represents the plucking finger.</xs:documentation>
		</xs:annotation>
		<xs:simpleContent>
			<xs:extension base="xs:string">
				<xs:attribute name="substitution" type="yes-no"/>
				<xs:attribute name="alternate" type="yes-no"/>
				<xs:attributeGroup ref="print-style"/>
				<xs:attributeGroup ref="placement"/>
			</xs:extension>
		</xs:simpleContent>
	</xs:complexType>

example of a note I found.

      <note default-x="79.07" default-y="-145.00">
        <pitch>
          <step>C</step>
          <octave>3</octave>
          </pitch>
        <duration>4</duration>
        <voice>5</voice>
        <type>half</type>
        <stem>down</stem>
        <staff>2</staff>
        <notations>
          <technical>
            <fingering>C</fingering>
            </technical>
          </notations>
        </note>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice find, I was just thinking selfishly as a guitarist!

I think the right call is to update the signature of Fingering.getNumber to return a string | null (or string with empty string as the default).

getNumber(): number | null {
return this.element.content().int();
}

Let me know if you have other ideas.

@jaredjj3
Copy link
Collaborator

But as this is my first contribution to the project and I didn't find a CONTRIBUTION.md so that's my best guess.

@it-ony I deeply appreciate the contribution, you're pretty spot on! I'll write a CONTRIBUTION.md tomorrow.

@it-ony
Copy link
Author

it-ony commented Dec 10, 2025

But as this is my first contribution to the project and I didn't find a CONTRIBUTION.md so that's my best guess.

@it-ony I deeply appreciate the contribution, you're pretty spot on! I'll write a CONTRIBUTION.md tomorrow.

I struggled most to understand the differences/responsibilities for the similar types in the project. I'm right now in my mobile that's why I can only tell from my head, but note is read from xml, then there is a class representation, and I think I found two more note classes or types.

When rendered with vexflow (also here I took a while to understand the difference between vexml and vexflow) the click listener returned an object referencing a render note, which has a reference to a vexflow note, with similar but different properties.

If would be nice if the differences and responsibility could be pointed out.

@jaredjj3
Copy link
Collaborator

But as this is my first contribution to the project and I didn't find a CONTRIBUTION.md so that's my best guess.

@it-ony I deeply appreciate the contribution, you're pretty spot on! I'll write a CONTRIBUTION.md tomorrow.

I struggled most to understand the differences/responsibilities for the similar types in the project. I'm right now in my mobile that's why I can only tell from my head, but note is read from xml, then there is a class representation, and I think I found two more note classes or types.

When rendered with vexflow (also here I took a while to understand the difference between vexml and vexflow) the click listener returned an object referencing a render note, which has a reference to a vexflow note, with similar but different properties.

If would be nice if the differences and responsibility could be pointed out.

Appreciate the feedback! Each directory has a README.md, but they're very lackluster. I'll improve it and probably centralize all this information, too.

@it-ony
Copy link
Author

it-ony commented Dec 10, 2025

Appreciate the feedback! Each directory has a README.md, but they're very lackluster. I'll improve it and probably centralize all this information, too.

This is brilliant! It was me not spending time to actual look for this kind of documentation. I really enjoyed understanding the types for score/parts/voice etc... As I'm not a musician. Just building my first music product to learn piano better.

I found the readme in the root already stunning, but the ones with the directories with their goals/ no goals that's next level. I should adapt this for my projects.

@it-ony it-ony force-pushed the feat/fingering-annotations branch from f3adb0c to a907a5f Compare December 11, 2025 09:24
@jaredjj3
Copy link
Collaborator

I had an emergency. I'll take a look at this PR and add documentation for contributors soon.

@jaredjj3
Copy link
Collaborator

Appreciate the feedback! Each directory has a README.md, but they're very lackluster. I'll improve it and probably centralize all this information, too.

This is brilliant! It was me not spending time to actual look for this kind of documentation. I really enjoyed understanding the types for score/parts/voice etc... As I'm not a musician. Just building my first music product to learn piano better.

I found the readme in the root already stunning, but the ones with the directories with their goals/ no goals that's next level. I should adapt this for my projects.

Thanks! I'm glad you like them. I started writing directory-scoped intent docs to try to bridge the gap between humans and AI coding agents. Since then, the agents.md "standard" emerged and I felt like that did a much better job than what I was trying to do.

I tried a different idea that decouples intents (aka specs) from the file structure of a project: https://github.com/stringsync/spec. It's a lightweight version of the Kiro IDE or https://github.com/github/spec-kit. Maybe you'll find one of those projects inspiring or useful!

@jaredjj3
Copy link
Collaborator

I had an emergency. I'll take a look at this PR and add documentation for contributors soon.

Done: https://github.com/stringsync/vexml?tab=contributing-ov-file#readme

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