QML cursor rewind to start of selection bogus
For this mental exercise, consider a score with four staves, which I’ll number 1–4 (even though they’re 0‥3 internally) and sixteen measures (1–16).
Now imagine the user has selected measures 5–8 in staves 2 and 3, and starts a plugin, such as colornotes.qml
or notenames.qml
.
The plugin gets a cursor, rewind
s it to the start and end of the selection to retrieve startStaff
, endStaff
and endTick
, and then wishes to iterate over all voices of all selected staves. It uses code like this for that:
for (var staff = startStaff; staff <= endStaff; staff++) {
for (var voice = 0; voice < 4; voice++) {
cursor.rewind(Cursor.SELECTION_START);
// rewind reset the track so we have to reset staffIdx and voice
cursor.staffIdx = staff;
cursor.voice = voice;
Incidentally, each call of cursor.rewind(Cursor.SELECTION_START)
not only jumps back to the first selected stave (here 2) of the first selected measure, but then also calls the C++ nextInTrack()
, which jumps to the “first segment at or after _segment which has notes / rests in _track”.
Now imagine that measure 4 has different segments between staves 2 and 3:
… 2 … … … 3 … … … 4 … … … 5 … …
1| | | |
2| | | xx|xxxxx
3| | |yyyyyzz|zzzzz
4| | | |
This will cause the segments y
in stave 3 to be lost, as the rewind + setting track afterwards will only show the z
-marked segments.
I noticed that because I used the sequence rewind-then-set-track also with SCORE_START, which does not reset the track, and therefore needs the staffIdx and voice be assigned first. This setup lost some segments and behaved weird in general.
Now the behaviour of cursor.rewind(Cursor.SELECTION_START)
is generally sane (and matches SELECTION_END, which selects the last stave), but not useful for this particular case. My suggestion here is to make it possible to add a second and third argument, so this becomes…
for (var staff = startStaff; staff <= endStaff; staff++) {
for (var voice = 0; voice < 4; voice++) {
cursor.rewind(fullScore ? Cursor.SCORE_START : Cursor.SELECTION_START, staff, voice);
… and if the second and third argument are javascript undefined
, i.e. not passed, then the current behaviour is used. I’m attaching an example diff against current master (only the implementation, not the .h
file) how this could be implemented with a hypothetical nullable Integer type.
Comments
example.diff_.txt
Indeed, running the "Note Names" plugin with this selection:
yields this result:
But I should think that this kind of selection would be fairly rare. Or is there a more common case?
I do not think that cursor.rewind(SELECTION_START) should modify the track or advance the cursor past the selection's startSegment(). It makes sense for the SCORE_START case to call nextInTrack(), because the first segment in the score is not likely to be a ChordRest segment. But the startSegment() of a range selection will always be a ChordRest segment. My suggestion is to simply remove these two lines of code from Cursor::rewind(). Of course, it is possible that cursor.element might be null after rewinding the cursor, but plugin authors should already be checking for that. And even if a plugin carelessly attempts to read a property of cursor.element when it is null, the plugin will fail, but MuseScore will not crash.
Well, the "Note Names" plugin, at least, currently depends on the current behavior of
cursor.rewind(SELECTION_START)
setting the cursor's track, as it gets the value ofstartStaff
fromcursor.staffIdx
after callingcursor.rewind(1)
. So changing that effect at this stage could be problematic. But I see no reason why we can't simply remove that one call tonextInTrack()
.In the example score you’ve given, precisely which ChordRest is the first element selected by
cursor.rewind(SELECTION_START)
before thenextInTrack()
call?It can’t be the first C₅ because that’s outside of the selection. It can’t be the second C₅ because that’s past the first C₃.
Are you sure the first element is guaranteed to always be a ChordRest?
Also, isn’t the
nextInTrack()
call needed to “load” the first element anyway?Even so, the change I outlined in my initial request would greatly simplify plugin writing as it fixes this issue more generally, and perhaps more safely?
which ChordRest is the first element selected by cursor.rewind(SELECTION_START) before the nextInTrack() call?
cursor.element
will be null. This is handled correctly by the "Note Names" plugin, which testscursor.element
before trying to access any of its properties. This is good practice anyway, and if plugins are not doing this already, they probably should.Are you sure the first element is guaranteed to always be a ChordRest?
I'm pretty sure there is no way to begin a range selection on anything other than a ChordRest. Even when using lasso selection (Shift-Ctrl-drag) the selection snaps to the first ChordRest in the lasso.
isn’t the nextInTrack() call needed to “load” the first element anyway?
No, all
nextInTrack()
does is advance the cursor's segment. WhenCursor::Element()
is called, it usesCursor::currentElement()
to "load" the element from the current segment.The change that you outlined is based on the assumption that the call to
nextInTrack()
is needed. I am merely suggesting that it might not be needed after all. Removing thenextInTrack()
call will automatically fix this issue for plugins that check for a nullcursor.element
, and will break plugins that do not, if they are not already broken. Your change will have no effect on existing plugins until they are modified to use the new parameters tocursor.rewind()
.In reply to which ChordRest is the first… by mattmcclinch
You’re pretty sure, but I have the picture you posted in front of my eyes.
It begins on a ChordRest which is not in the first track of the selection (but in the fifth), AIUI. As I see it, it begins on the first C₃ (second beat in the measure), but the beginning of the selection is not in its first track.
In reply to You’re pretty sure, but I… by mirabilos
The selection begins on the segment that contains the first C₃. That segment does not contain an element in the first track, which is why
cursor.element
will be null the first time around.Ah okay, but
cursor.segment
won’t, so we just need some more looping, and so the code around https://github.com/mirabilos/mscore-plugins/blob/d21bbf15aac6cde654e749… would also be correct.OK, in that case, concerns resolved.