fix: extra space before seconds with modifiers

[gristow]

Fixes #1578

Previously seconds with a left shifted notehead (those with a downward stem) would take more x space than needed:

vs.

And StringNumbers did not self-position correctly for seconds with a left shifted notehead:

This PR resolves both of these issues, adding an additional visual regression test:

[AaronDavidNewman]

Gregory, these changes look great. Thank you.

When I try to build on your branch, I get some errors. I don't see the same errors on the vexflow master branch when doing a clean install. Is that expected? I wasn't sure if this was a clean branch or had some of your proprietary stuff on it.

If the latter, I (or one of us) can just merge the files piecemeal.

In particular, npm install fails like so. I am on Windows 10.

npm ERR! code 1
npm ERR! path C:\Users\Daddy\Documents\GitHub\vex-ristow\vexflow\node_modules\slimerjs
npm ERR! command failed
npm ERR! command C:\WINDOWS\system32\cmd.exe /d /s /c node install.js
npm ERR! Downloading http://download.slimerjs.org/releases/0.9.6/slimerjs-0.9.6-win32.zip
npm ERR! Saving to C:\Users\Daddy\AppData\Local\Temp\slimerjs\slimerjs-0.9.6-win32.zip
npm ERR! Receiving...
npm ERR! Error requesting archive.
npm ERR! Status: 301
npm ERR! Request options: {
npm ERR!   "protocol": "http:",
npm ERR!   "slashes": true,
npm ERR!   "auth": null,
npm ERR!   "host": "download.slimerjs.org",

[gristow]

Hmmm, that should be unrelated to any changes I made. I branched off of the current vexflow master, and only changed accidental.ts, stringnumber.ts and stringnumber_tests.ts. (That is, I didn’t touch anything related to package.json or the build process.)

[gristow]

@rvilarl or @ronyeh -- if you have a chance, I'd love a code review for this. This definitely improves layout issues, but because modifier layout is not part of the codebase I've spent much time in before, I would definitely appreciate your eyes to be sure I've done this in a logical way.

[rvilarl]

@gristow thanks a lot. Only one change required, the unused variable. The variable names are quite long :) if we follow this path, the library will grow.

[mscuthbert]

I like the long variable names. They helped me review the PR in no time flat. Remember that variables that are not exported become "f" or whatever when minified.

[gristow]

Thanks for the review, @rvilarl, and for catching that unused variable. I've added a commit to fix that.

I agree the variable names are longer than usual, and I went back and forth on whether that was the right direction to go. For me, after it took about an hour just to grok what all of the code in accidental.ts was doing (especially given that I wrote a lot of that code myself in 2014), I decided the trade-off in length was probably worth it for helping future contributors understand what was going on.

[rvilarl]

I like the long variable names. They helped me review the PR in no time flat. Remember that variables that are not exported become "f" or whatever when minified.

@mscuthbert @ronyeh I was looking into that, Are we minifying including renaming of variables in our build process?