Improvements for double/half durations on range and list selection

[Jojo-Schmitz]

1. Fix #317188: Allow [Q] and [W] to work on a range selection.
   Port of Fix #317188: Allow [Q] and [W] to work on a range selection #7519 for 3.x

2. [Cmd: Double/Half Duration] Allow to apply to a list selection - equivalent to pressing a duration toggle. Shift also allowed
   Port of [Cmd: Double/Half Duration] Allow to apply to a list selection - equi… Jojo-Schmitz/MuseScore#609, fixes Enhancement: Allow Q/W to perform duration toggle on a list selection Jojo-Schmitz/MuseScore#608

   Fix #20962: Keep selection when deleting, changing note/rest lengths #20984 Updated so that the selection would retain the chords if the user has a list selection and performs duration changes by way of the pad toggle. Similar to Double/Half like a range selection, but inserts rests in-between the notes when making smaller form

   I (@worldwideweary) figured why not in addition let Q/W (with or without shift) do the same thing with the duration changes. That is

   1. Range based Half/Double exists already (no rests result)
   2. Do the recent up/down chord thing to convert to list and perform a delete, selection retained
   3. Now perform Q/W on the list... with Shift/W showing.
   qwList.webm

[Jojo-Schmitz]

Could this get rewiewed (again), please?

[Jojo-Schmitz]

What is going to happen with this PR?

[cbjeukendrup]

Sorry, we're really a bit too much behind with reviewing community PRs. Hopefully we can manage to get through most of them in the coming months. I've added it to 4.6.

[Ash-86]

This is cool. Does shift Q/W also work on range selection? it would be nice if it did.

[Jojo-Schmitz]

It should, but let me rebase, then you can check yourself

[Ash-86]

Thanks! Indeed, shift Q/W works with range selection but here are some bugs i found:

• shift Q/W dont work after having extended selection beyond a measure.
• pressing W on list selection crashes MU or corrupts measure if durations overflow.
• operating on a single note list selection: pressing shift W first time toggles "dotted note" from the taskbar, you need to press shift W again to actually change duration of the note. [outside note input mode]
• pressing Q/W on empty measures changes selection to two measures or half measure correspondingly. [not really a bug, but maybe it didn't feel like it was an intended behaviour (?)]
• shift Q on single note list selection skips undotted values. ex. goes from ¼dotted to ⅛dotted. [outside note input mode]

[Jojo-Schmitz]

Interesting, none of this happens with 3.7 Evolution (except your point 4. does, but as you mentioned, that's not really a bug), which contains the changes this PR here ports forward.

[Ash-86]

After a closer look, i pinpointed the correct conditions of the bugs, which ,btw, i also found to be present in 3.7

• range selection: shift+Q/W dos not work if the selection cointain both dotted and undotted notes.
• list selection: both Q/W and shift+Q/W don't work if selected notes have different durations.

[zacjansheski]

Thank you!

Tested on MacOS 15, Windows 11, Ubuntu 22.04.3. Approved
#24759 FIXED

[Jojo-Schmitz]

What prevents it from getting merged then?

[cbjeukendrup]

I noticed a change in behaviour for full-measure rests: for most time signatures, it no longer decreases the duration of a full-measure rest, but only rewrites it using a non-full-measure rest. Example: in 4/4, a centred full measure rest only changes into a non-centred 4/4 rest, instead of being split into two 2/4 rests. Similar situation in 3/4. I don't know if that's a blocker, but I did want to check it.

[cbjeukendrup]

On second thoughts, this fraction multiplication magic seems not the best implementation to me. A bit risky, and relying heavily on the assumption that nSteps == ±1 (and not 0, or 2), which wasn't the case with the previous implementation.

What is actually the reason for replacing the original implementation with shiftRetainDots and the DurationType::V_MEASURE check?
If it's only that it's a bit verbose, it could be moved into a static Fraction …(…) function.

[worldwideweary]

I can't speak for the changes not in accord with what I initially provided for 3.x (https://github.com/Jojo-Schmitz/MuseScore/pull/609/files), but I can say that the nSteps assumption was valid given that this function (cmdIncDecDuration) in 3.x is only used for some "wrapped" around functions with the following:

void cmdDoubleDuration()      { cmdIncDecDuration(-1, false); }
void cmdHalfDuration()        { cmdIncDecDuration( 1, false); }
void cmdIncDurationDotted()   { cmdIncDecDuration(-1, true); }
void cmdDecDurationDotted()   { cmdIncDecDuration( 1, true); }

The nSteps will always be ±1 with these. but I don't know if things have changed in the main upstream 4.x branch. It wasn't ever used besides those four functions, but it wouldn't hurt to safeguard just in case there is a change in the future of course.

As for the multiplication magic and the removal of the V_MEASURE test: @Jojo-Schmitz ? The V_MEASURE check still remains at the bottom of this function in 3.7 from what I can tell.

[Ash-86]

The first equation guarntees multiplication by 3/2 or 3/4. (for dotted augmentation/diminishing)
The second eq. sets multiplication by ½ or 2.
Both cases depend on nSteps being ±1.

[worldwideweary]

@Ash-86 et. al: Not sure, but it seems to me the multiplication isn't necessary since there are shiftRetainDots and shift functions already existing for TDuration (not fractions), like if the code were to do this:

for (auto cr : crs) {
    TDuration odt = cr->durationType();
    TDuration ndt(stepDotted ? odt.shiftRetainDots(nSteps, stepDotted) : odt.shift(nSteps));
    changeCRlen(cr, ndt);
}

[Ash-86]

Sure, whatever's easier to read :)

[Jojo-Schmitz]

What coded change would be needed then?

[Ash-86]

Ah, there was already a check for measure-rests!! Should bring back that, and let's use shift functions instead of doing fraction math, as per worldwideweary's suggestion, this becomes:

        for (ChordRest* cr : crs) {
            // if measure rest is selected as input, then the correct initialDuration will be the
            // duration of the measure's time signature, else is just the ChordRest's duration
            TDuration initialDuration = cr->durationType();
            if (initialDuration == 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;
                }
            }

            TDuration newDuration(stepDotted ? initialDuration.shiftRetainDots(nSteps, stepDotted) : initialDuration.shift(nSteps));

            if (cr->isChord() && (toChord(cr)->noteType() != NoteType::NORMAL)) {
                undoChangeChordRestLen(cr, newDuration);
            } else {
                changeCRlen(cr, newDuration);
            }
        }

In my test, everything seemed to work well.

[cbjeukendrup]

I've pushed one more fix commit that implements the above. If one of you could sanity-check it, then I'll go ahead and merge. Thanks!

[Jojo-Schmitz]

Fine by me

[cbjeukendrup]

Then let's merge, I think with all of us together we have tested it enough by now.

[Ash-86]

it's a shame this didn't make it to 4.6 :(

[Ash-86]

@Jojo-Schmitz could you rebase this, please?

[Jojo-Schmitz]

done