Vexflow 4.0.3

[ronyeh]

This PR replaces https://github.com/cuthbertLab/music21j/pull/172

It upgrades music21j to the current release of VexFlow.

[ronyeh]

127 tests completed in 503 milliseconds, with 2 failed, 0 skipped, and 0 todo. 908 assertions of 911 passed, 3 failed.

music21.vfShow.Renderer prepareTies in voices (2, 0, 2)[Rerun] music21.vfShow.Renderer prepareTies in voices across barline (1, 0, 1)[Rerun]

Is this expected? I don't see any other errors.

[ronyeh]

Fixed!

 grunt test_no_watch

>> 128 tests completed with 0 failed, 0 skipped, and 0 todo.
>> 912 assertions (in 652ms), passed: 912, failed: 0

[ronyeh]

[ronyeh]

Do you consider this a bug in VexFlow 4?

On Tue, Aug 23, 2022, 6:53 AM Joseph VanderStel @.***> wrote:

@.**** requested changes on this pull request.

@ronyeh https://github.com/ronyeh the PR looks great. I noticed that voices with no notes in them now raise TypeError: Cannot read properties of undefined (reading 'getMetrics'). A minimal example that reproduces the problem:

const s = new music21.stream.Stream();s.replaceDOM();

In vfShow.Renderer.vexflowVoice() we use a stream's duration.quarterLength to calculate the num_beats to pass to the new Vex.Flow.Voice instance. In VexFlow 4, the above TypeError occurs when num_beats is 0; this happens whenever an empty stream is rendered to the DOM. We'll just want to check for an empty stream before calling vexflowVoice(). I'm happy to help with this if you'd like, just let me know. With that, this PR will be ready to merge. Thanks!

— Reply to this email directly, view it on GitHub https://github.com/cuthbertLab/music21j/pull/196#pullrequestreview-1082220111, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2MCNCHG7NA4DMY7AATNLV2TJVPANCNFSM57KFIKTA . You are receiving this because you were mentioned.Message ID: @.***>

[ronyeh]

When do we want to format empty voices?

I created a test case like this:

    test('music21.stream.Stream.DOM voice with no notes', assert => {
        const s = new music21.stream.Stream();
        s.createNewDOM(100, 50);
        s.replaceDOM();
    });

And I tried "fixing" the issue by checking for an empty stream and returning an appropriate value (e.g., undefined, or an empty voice). However, each "fix" seems to trigger a different exception.

I guess I can push my commit and see what you all think.

[ronyeh]

Please see this commit: https://github.com/cuthbertLab/music21j/pull/196/commits/f67d18685daa13ffb64d87d3eb60a0facc57f22c

I added a test case based on your example, and verified that it does not raise an error.

However, please verify this is the intended behavior. (Or feel free to try out my PR and suggest an alternative solution.)

Thanks!

[mscuthbert]

I think that the expectation is that even a s = new music21.stream.Stream(); s.appendNewDOM() will put something out there; I know I use this in a lot of places to create a blank staff for writing on -- so maybe what we need to do is to put in an empty rest or something? -- Here's the current behavior on vf 1:

Work like this allows for adding the first note to an empty staff in a couple of editors based on music21j

I'm also concerned by the number of staves in the QUnit tests that have a whole note not on a staff -- has something changed on how estimateStaffLength() works that is making it think the staff can be extremely short?

For the num_beats: 0 is not allowed part -- I'm guessing that this is something that's happening in the VF formatter? So we could do our own tests and skip formatting/joining voices if there are no tickables in it?

Note also that zero-quarterLength streams can still have a lot of information in them. Clefs, Key Signatures, Time Signatures, barline types, etc. are zero duration.

[mscuthbert]

Do you consider this a bug in VexFlow 4?

If this wasn't an intended change in behavior then probably yes. If it's something that was discussed in the breaking-backwards compatibility part of things, then probably no.

[ronyeh]

Do you consider this a bug in VexFlow 4?

If this wasn't an intended change in behavior then probably yes. If it's something that was discussed in the breaking-backwards compatibility part of things, then probably no.

Can we make a tiny example that demonstrates this difference between legacy VexFlow and VexFlow 4? You can file a bug and we can take a look at it over at the 0xfe repo. Is the main issue that you'd like to be able to call format on an empty stave? We have lots of hello world examples with empty staves.... I wonder what's the difference between those examples and music21j's empty streams.

[mscuthbert]

Some of the other problems with tests seem to exist in current master and were broken before this PR. -- I think that if the changes I made in the last push look good we can merge.

Oh, heck, let's merge and work through other things later.

[mscuthbert]

CONGRATS @ronyeh and the whole Vexflow 4 team!

[ronyeh]

Awesome. Glad to help!

Please file any bugs you discover over on the VexFlow repo. We'll be happy to take a look. The more bugs we squash, the more robust VexFlow will be.

Let me know when you distill your empty stream bug into a self-contained VexFlow-only example. Can we trigger the exception by having an empty stave and calling format? If so, we can clean up those edge cases and push a new build for you.

Have a great weekend!