Fix #290929: crash with negative number of dots for rests

[Jojo-Schmitz]

See also PR #5235, use either that (done) or this (no longer) or both (still possible).

This here avoids the crash in this particular case, and in that only.

[dmitrio95]

That wouldn't probably help if -n > _dots.size() and _dots is not empty. It looks like this check should be moved into the loop instead.
Also some qDebug() or qWarning() message for this case could perhaps be useful, this situation shouldn't normally happen.

[Jojo-Schmitz]

I like the other fix (option 2) better anyhow, more fixing it at the deepest root, fixes it at read and write too and for rests and chords.
This one here is more curing the symptom, preventing that particular crash.

[Jojo-Schmitz]

Do you mean something like this?

  if (n < 0) {
        for (int i = 0; i < -n; ++i) {
              if (!_dots.size() || -n > _dots.size()) {
                    qWarning("no dots at all or number mismatch");
                    return;
                    }
              score()->undoRemoveElement(_dots.back());
              }
        }

[dmitrio95]

Something like this actually:

  if (n < 0) {
        for (int i = 0; i < -n; ++i) {
              if (_dots.empty()) {
                    qWarning("no dots at all or number mismatch");
                    return;
                    }
              score()->undoRemoveElement(_dots.back());
              }
        }

This will clear dots if -n > _dots.size() which seems sensible to do in this case.

But maybe the fix in #5235 should indeed be enough so let's apply that one first.

[Jojo-Schmitz]

I don't quite see the difference to my initial change though, except that now we go into the for loop once in the error case, something that the initial fix did not. To me this is just a more expensive way of doing the same thing?

[dmitrio95]

There is no difference if there is no dots initially. If there are some dots but -n appears to have a large value then the approach I propose will just clear all the dots without crashing the program. But I am not sure whether such situations actually happen.

[anatoly-os]

Could you add the test which opens the file from the bug report? Successful reading the file without a crash will verify the fix and keep the scenario working for the future changes.

[dmitrio95]

As far as I understand this pull request might be not needed at all since the other fix for the issue, #5235, has already been merged.

[anatoly-os]

The test won't be useless anyway. I agree, I didn't get why we need this PR. @Jojo-Schmitz ?)

[dmitrio95]

I agree, I didn't get why we need this PR

This one was created earlier :)

[Jojo-Schmitz]

This PR is not needed anymore, it was meant as an alternative to the other.
We never found how those broken scores bacem that broken, so a test file would be completly artificial and may not test the real sitiation.
I'd rather just close this...