spanner.py backward compatible changes

[gregchapman-dev]

New features in spanner.py: (1) Let pending spanner element assignments have an associated required offset and clientInfo (if not specified, behaves as before). (2) new API insertFirstSpannedElement (3) new API popPendingSpannedElements. Also a fix that saves/restores spanner element offset/activeSite around operations that clear them.

[gregchapman-dev]

This is the second split-out PR from PR #1768

[coveralls]

coverage: 93.069% (+0.001%) from 93.068% — gregchapman-dev:gregc/pendingSpannerAssignmentWithOffset into cuthbertLab:master

[gregchapman-dev]

All merged, and it looks like the tests are passing. Ready for review.

[gregchapman-dev]

Argh. Gotta test those new APIs that no-one is calling yet. Hang on.

[mscuthbert]

Hi Greg -- can you talk through your proposed contribution with another music21 user to help get down on paper what exactly the benefit to the library of this contribution is? I feel like I've tried many times to understand what is happening and I still don't see it, and I need to get to other projects until I do. Thanks.

[gregchapman-dev]

Maybe I should discuss this with @jacobtylerwalls, since he has been in the MusicXML reader a lot.

[gregchapman-dev]

I've tried to explain carefully in the doctests exactly what is intended here, in terms of how these APIs have been used in the past (which still works) and how they can be used in the future (and why).

I restored the old doctests to show that they still pass (although I tweaked the order of calls a bit to match the order of the "old-style" calls that come from xmlToM21.py). Then I added the new doctests, and enhanced them (and their comments) to show how a parser can use the new parameters to deal with the fact that sometimes there is no "next note" that is at the correct offset to start the spanner, so a SpannerAnchor should be used instead.

The next PR will be the one that changes xmlToM21.py to take advantage of the new-style calls, to handle <direction> with <offset> by inserting a SpannerAnchor at the correct offset.

[mscuthbert]

make it staffKey: int|None -- now it makes sense why we might only want to free up spanned elements on one staff. Making something too hypothetical doesn't help me get why we want to add this complexity to the system.

Just please show that the way things were working before will still work -- I don't normally need to know the offset of the pending assignment in order to get it. But if there are multiple pending I can free just one by offset? Just show that -- then it's a new feature.

[gregchapman-dev]

Ooops, I was editing my comment while you were commenting on my previous comment: Here's my new one again:

I've tried to explain carefully in the doctests exactly what is intended here, in terms of how these APIs have been used in the past (which still works) and how they can be used in the future (and why).

I restored the old doctests to show that they still pass (although I tweaked the order of calls a bit to match the order of the "old-style" calls that come from xmlToM21.py). Then I added the new doctests, and enhanced them (and their comments) to show how a parser can use the new parameters to deal with the fact that sometimes there is no "next note" that is at the correct offset to start the spanner, so a SpannerAnchor should be used instead.

The next PR will be the one that changes xmlToM21.py to take advantage of the new-style calls, to handle <direction> with <offset> by inserting a SpannerAnchor at the correct offset.

[gregchapman-dev]

I think this is ready for re-review now (assuming the tests pass, which they did locally).

[gregchapman-dev]

Oops, forgot to change the version number(s). All ready now.

[gregchapman-dev]

Hopefully the new doctests and description will make the benefit of this new feature clear. I am adding a feature that xmlToM21.py will use soon to import accurately <direction>s that have <offset>s.

[gregchapman-dev]

Oh wow, changing version from 9.7.2a7 to 9.7.3 lowered coverage by 3 lines, because "a7" was not parsed. Before I made that change, coverage was actually increased (a very little) by this PR.

[mscuthbert]

Closing and reopening for new tests, etc. (after resolving conflicts)

[mscuthbert]

Hi Greg -- I worked my way through the PR again and it is a significant improvement that will greatly enhance getting spanners from MusicXML parsing correct.

I've accepted the offsetInScore path, but in the last passes, I've removed staffKey completely from spanner.py. That offset is used to filter pending assignments but staffKey is not - ended up confusing me and the spanner.py code too much. But PendingAssignementRef is now a frozen dataclass and is returned from setPendingSpannedElementAssignment which offers an easy path to using staff keys. For the MusicXML parsing branch I suggest doing this:

self.pendingAssignmentRefIdToStaff = {}
...
ref = bundle.setPendingAssignmentRef(spanner, 'Note', offsetInScore)
self.pendingAssignmentRefIdToStaff[id(ref)] = staffKey

Let me know if that works -- merging this now, and we can get other changes in a fresh PR.

[mscuthbert]

Approving!

[mscuthbert]

eventually all of these should take in an actual Class/type object instead of str.