[MU4] Fix #332925: Half duration shortcut crashes musescore when time signature is 5/4

[Jojo-Schmitz]

Crash happens only with Debug builds, not with Release or RelWithDebInfo builds, as those have asserts disabled.
Probably happens on any time sig, where the full measure rest is not having the same duration as a whole rest, and also does happen on "half dotted duration" as well as "double duration" and "double dotted duration".

Resolves: https://musescore.org/en/node/332925

Same issue and fix for MU3

[Jojo-Schmitz]

Might no longer be needed, at least I can't reproduce the issue anymore?

[Jojo-Schmitz]

The assert is still in place though:

// Longest TDuration that fits into Fraction. Must fit exactly if truncate = false.
TDuration::TDuration(const Fraction& l, bool truncate, int maxDots, DurationType maxType)
{
#ifdef NDEBUG
    UNUSED(truncate);
#endif
    setType(maxType);   // use maxType to avoid testing all types if you know that l is smaller than a certain DurationType
    setDots(maxDots);
    truncateToFraction(l, maxDots);
    assert(truncate || (fraction() - l).numerator() == 0);   // check for exact fit
}

So it seems my testing is flawed, as for halfing a measure rest in 5/4 would never be an exact match (and indeed isn't, it gets turned into 3 rests, half, and half)

[Jojo-Schmitz]

Could this please get reviewed? It sits here since 11 months

[cbjeukendrup]

The change itself makes sense to me, but the resulting behaviour is maybe not entirely what I would expect. If I select a full measure rest in a 5/4 measure, then I'd expect it to be shortened to 4/4 first, and then to 2/4 (or 2/4 dotted when using Shift). But currently, it will immediately go from 5/4 to 2/4 (or 2/4 dotted).

Fortunately, I don't think that needs to be difficult to implement: when nSteps is negative (i.e. we are decrementing), just check if the fraction of the truncated TDuration is already shorter than the timesig fraction; in that case, don't shorten further.

[Jojo-Schmitz]

I'm confused now... where exactly check what exactly?

[cbjeukendrup]

In TDuration(cr->measure()->timesig(), true), you are constructing a TDuration with the length of the measure, but truncating it to a duration that is representable using a single rest with zero or more dots.

For instance, if the time signature is 5/4, this will be truncated to a whole note (duration 4/4). Then, by initialDuration.shiftRetainDots, this is shortened to a dotted half note, but I suggest that this should not happen.

So, the truncation needs to be detected, and that can be done by comparing the fraction of the truncated TDuration to the timesig fraction. Concretely:

if (initialDuration.fraction() < cr->measure()->timesig()) {
    // then the duration was already shortened by the truncation; don't shorten it again
}

Of course, we should only do this when decrementing the duration; when incrementing it, the result is already correct.

So I'd suggest to rewrite the surrounding code like this:

TDuration initialDuration;
if (cr->durationType() == DurationType::V_MEASURE) {
    initialDuration = TDuration(cr->measure()->timesig(), true);
   
    if (initialDuration.fraction() < cr->measure()->timesig() && nSteps < 0) {
        // Duration already shortened by truncation; shorten one step less
        ++nSteps;
    }
} else {
    initialDuration = _is.duration();
}

Then you could add an optimisation for the case that nSteps has become 0; for example, TDuration d = (nSteps != 0) ? initialDuration.shiftRetainDots(nSteps, stepDotted) : initialDuration;.

[Jojo-Schmitz]

OK, done.
That was very clear ;-)

[cbjeukendrup]

@Jojo-Schmitz Sorry, turns out I was confused. Contrary to what I thought, positive nSteps represents decrementing (and that has to do with the order of the cases of the DurationType enum...). So, the < should be > and the ++ should be --.

[Jojo-Schmitz]

Glad that not just I was confused ;-)
Done.

[cbjeukendrup]

@abariska This is ready for testing!

[abariska]

🟢 TESTED
Please, merge to master