QML cursor rewind to start of selection bogus

• Mar 1, 2020 - 20:35
Reported version
3.2
Type
Plugins
Frequency
Once
Severity
S4 - Minor
Reproducibility
Always
Status
active
Regression
No
Workaround
No
Project
MuseScore

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, rewinds 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

• Mar 5, 2020 - 19:18

Indeed, running the "Note Names" plugin with this selection:
selection.png
yields this result:
result.png
But I should think that this kind of selection would be fairly rare. Or is there a more common case?

Reply

• Mar 5, 2020 - 22:30

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.

Reply

• Mar 5, 2020 - 23:09

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 of startStaff from cursor.staffIdx after calling cursor.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 to nextInTrack().

Reply

• Mar 6, 2020 - 14:46

In the example score you’ve given, precisely which ChordRest is the first element selected by cursor.rewind(SELECTION_START) before the nextInTrack() 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?

Reply

• Mar 6, 2020 - 16:37

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 tests cursor.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. When Cursor::Element() is called, it uses Cursor::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 the nextInTrack() call will automatically fix this issue for plugins that check for a null cursor.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 to cursor.rewind().

Reply

• Mar 6, 2020 - 17:00

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.

Reply