Paste note fixes #8825

wizofaus · 2021-08-10T07:14:27Z

Resolves: #8824

Ensure that pasting notes works as expected in all circumstances

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

…el_restoring_state [MU4] Various fixes for the instruments panel

wizofaus · 2021-08-10T07:15:18Z

src/engraving/libmscore/chordrest.cpp

@@ -1433,7 +1433,7 @@ void ChordRest::removeMarkings(bool /* keepTremolo */)
 bool ChordRest::isBefore(const ChordRest* o) const
 {
    if (!o || this == o) {
-        return true;
+        return false;


Prevent asserts due to unstable std::sort predicates.

wizofaus · 2021-08-10T07:18:16Z

src/engraving/libmscore/edit.cpp

@@ -453,7 +453,7 @@ Rest* Score::setRest(const Fraction& _tick, int track, const Fraction& _l, bool
            // compute list of durations which will fit l
            //
            std::vector<TDuration> dList;
-            if (tuplet || staff->isLocalTimeSignature(tick)) {
+            if (tuplet || staff->isLocalTimeSignature(tick) || f == Fraction(0, 1)) {


was crashing without this when pasting, e.g. a whole note onto a multi-measure rest, the 'else' part of this test definitely doesn't work if f is 0 anyway

wizofaus · 2021-08-10T07:19:32Z

src/engraving/libmscore/paste.cpp

+                    } else if (target->isChordRest()) {
+                        newEl = replaceNote(toChordRest(target), toNote(newEl), duration);
+                    } else {
+                        target->drop(ddata);


would prefer that above logic was all inside Note::drop( ) and ChordRest::drop( ) but would require adding even more info (duration) to the already-bloated EditData structure...

Jojo-Schmitz · 2021-08-10T07:20:53Z

There's a merge commit that doesn't belong into this PR.
Is this supposed to fix https://musescore.org/en/node/321809 for master? Alternative to the 2nd commit for #8818?

wizofaus · 2021-08-10T07:22:49Z

There's a merge commit that doesn't belong into this PR.
Is this supposed to fix https://musescore.org/en/node/321809 for master? Alternative to the 2nd commit for #8818?

That came from a rebase. I tried to squash but it wouldn't play nicely. Need to fix the code-style thing though.

wizofaus · 2021-08-10T07:24:33Z

And when I say "fix", I mean screw up my sensible formatting to suit the current version of uncrustify used by the git workflows.

Jojo-Schmitz · 2021-08-10T10:04:05Z

As far as I can tell this PR, on dropping a notehead of a quarter note onto an MMRest, pastes a quarter note, not a note equal to a the full lenghth of the measure?

wizofaus · 2021-08-10T10:11:28Z

It preserves the length of the note you copied, which is consistent with how it works when pasted onto anything else.
That's a new behaviour of MU4 (MU3 only ever transferred the pitch as the clipboard data didn't even include the note duration).

Jojo-Schmitz · 2021-08-10T10:17:49Z

But that then is not what you describe as the desired outcome in section a) of #8824

wizofaus · 2021-08-10T10:19:10Z

Yes it is, because pasting on to a whole measure rest in MU4 does preserve the length of the copied note (adding/removing rests as required to fit it on).

Jojo-Schmitz · 2021-08-10T10:21:46Z

Pasting a note on to a multi-measure rest should behave the same as pasting onto a whole rest,
That's not what it does
Hmm, OK, not what it did in 3.x...

wizofaus · 2021-08-10T11:04:08Z

BTW this is not ready for merge yet, still a number of issues I just discovered.

RobFog · 2021-08-10T13:11:11Z

BTW this is not ready for merge yet, still a number of issues I just discovered.

You can mark the pull request as a draft.

wizofaus · 2021-08-10T23:10:42Z

I'd looked for that option (convert to draft), couldn't see it, it's not very obvious!

wizofaus · 2021-08-11T10:13:57Z

src/engraving/libmscore/chordrest.cpp

+        chord->setTicks(ticks());
+        chord->setTuplet(tuplet());
+        chord->add(note);
+        score()->undoAddCR(chord, seg->measure(), seg->tick());


existing method failed to transfer all properties and didn't work with tuplets etc.

wizofaus · 2021-08-11T10:17:52Z

src/engraving/libmscore/cmd.cpp

        // the returned gap ends at the measure boundary or at tuplet end
-        Fraction dd = makeGap(segment, track, sd, cr ? cr->tuplet() : 0);
+        Fraction dd = makeGap(segment, track, sd, tuplet);


the changes here are about ensuring that a) if the element at the segment position is a rest, then any other notes that would be overlapped get removed instead of being "split" into component durations - actually I'd rather simplify this so that setNoteRest always does that but it may have unexpected side effects and b) only the initial tuplet is preserved where possible - the existing logic tried to preserve all tuplets that fell inside the duration, which didn't make sense for any case I could think of

Jojo-Schmitz · 2021-08-11T11:07:18Z

src/engraving/libmscore/cmd.cpp.uncrustify

@@ -0,0 +1,4438 @@
+/*
+ * SPDX-License-Identifier: GPL-3.0-only


I guess this file doesn't belong into the PR

I'd removed it within minutes!

But not pushed it equally fast I guess? Anyway, issue is sorted now.

Backport of musescore#8825, better fix than musescore#8816, better even than my alternative, which this commits reverts. It does however change the way noteheads are pasted: now with the duration of the source chord rather than the duration of the target.

wizofaus · 2021-08-11T23:32:33Z

src/engraving/libmscore/paste.cpp

+    return target;
+}
+
+static bool canPasteStaff(XmlReader& reader, const Fraction& scale)


to prevent crashing when using "paste half length" on a range with 1/1024th notes, a bit kludgy but I tried a few other ways and this was the more reliable and least code by far

wizofaus · 2021-08-11T23:33:34Z

src/engraving/libmscore/paste.cpp

-            if (!pasteStaff(e, cr->segment(), cr->staffIdx(), scale)) {
-                return;
+            if (canPasteStaff(e, scale)) {
+                XmlReader e(data);


XmlReader doesn't have a rewind...should add one perhaps?

You already declared XmlReader e(data) at line 1166. We will get a warning here

Declared? Rather called. So it might get called twice now.

Correct, you can't rewind it so you have to create a new one... happy to add a rewind function

But you used the same name twice in two different declarations. You can move this code in a separate method or just rename the second reader

They're in different scopes so the behavior is well defined, but I agree stylistically some may prefer avoiding reusing the name (actually I don't like single letter variable names generally!).

Took your suggestion of creating a separate method, creating a rewind method was a bit painful/dangerous due to inner workings of Qt.

Backport of musescore#8825, better fix than musescore#8816, better even than my alternative, which this commits reverts. It does however change the way noteheads are pasted: now with the duration of the source chord rather than the duration of the target.

vshalkevich · 2021-08-16T09:12:15Z

Works fine

Backport of musescore#8825, better fix than musescore#8816, better even than my alternative, which this commits reverts. It does however change the way noteheads are pasted: now with the duration of the source chord rather than the duration of the target.

Merge pull request musescore#8821 from RomanPudashkin/instruments_pan…

5c72065

…el_restoring_state [MU4] Various fixes for the instruments panel

wizofaus commented Aug 10, 2021

View reviewed changes

wizofaus force-pushed the paste_note_fixes branch from ed27d23 to 0fd10fe Compare August 10, 2021 07:27

Jojo-Schmitz mentioned this pull request Aug 10, 2021

[MU4] Fix crash when pasting tie onto rest #8818

Merged

Jojo-Schmitz mentioned this pull request Aug 10, 2021

[MU3 Issue] MuseScore 3.6.3 or 3.7.0, PRs to be merged and issues to be fixed. #7449

Closed

wizofaus marked this pull request as draft August 10, 2021 23:10

wizofaus force-pushed the paste_note_fixes branch from 0fd10fe to a88a567 Compare August 11, 2021 10:11

wizofaus commented Aug 11, 2021

View reviewed changes

wizofaus force-pushed the paste_note_fixes branch 2 times, most recently from 3de4786 to 3d1f2e1 Compare August 11, 2021 10:54

Jojo-Schmitz reviewed Aug 11, 2021

View reviewed changes

wizofaus force-pushed the paste_note_fixes branch from 3d1f2e1 to f011e9a Compare August 11, 2021 12:05

wizofaus marked this pull request as ready for review August 11, 2021 12:06

Jojo-Schmitz mentioned this pull request Aug 11, 2021

[MU3] fix #321809: refuse dropping a MMRest (avoid crash) #8816

Closed

8 tasks

wizofaus force-pushed the paste_note_fixes branch from 86c5dae to e637400 Compare August 11, 2021 23:28

wizofaus commented Aug 11, 2021

View reviewed changes

wizofaus force-pushed the paste_note_fixes branch from e637400 to 7245915 Compare August 12, 2021 00:05

[MU4] fix musescore#8824 - fix pasting notes

dc91cae

wizofaus force-pushed the paste_note_fixes branch from 7245915 to dc91cae Compare August 12, 2021 09:44

RomanPudashkin approved these changes Aug 13, 2021

View reviewed changes

RomanPudashkin merged commit 494582a into musescore:master Aug 13, 2021

Jojo-Schmitz mentioned this pull request Aug 13, 2021

Fix pasting ties #8848

Closed

8 tasks

Jojo-Schmitz mentioned this pull request Sep 1, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

worldwideweary mentioned this pull request Apr 16, 2024

[Fix] Pasting repeat barline (probably other things also potentially) Jojo-Schmitz/MuseScore#421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paste note fixes #8825

Paste note fixes #8825

wizofaus commented Aug 10, 2021 •

edited

wizofaus Aug 10, 2021

wizofaus Aug 10, 2021

wizofaus Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021

wizofaus commented Aug 10, 2021

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 •

edited

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 •

edited

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 •

edited

wizofaus commented Aug 10, 2021

RobFog commented Aug 10, 2021

wizofaus commented Aug 10, 2021

wizofaus Aug 11, 2021

wizofaus Aug 11, 2021

Jojo-Schmitz Aug 11, 2021

wizofaus Aug 11, 2021

Jojo-Schmitz Aug 12, 2021

wizofaus Aug 11, 2021

wizofaus Aug 11, 2021

RomanPudashkin Aug 12, 2021

Jojo-Schmitz Aug 12, 2021

wizofaus Aug 12, 2021

RomanPudashkin Aug 12, 2021

wizofaus Aug 12, 2021

wizofaus Aug 12, 2021

vshalkevich commented Aug 16, 2021

Paste note fixes #8825

Paste note fixes #8825

Conversation

wizofaus commented Aug 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz commented Aug 10, 2021

wizofaus commented Aug 10, 2021

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 • edited

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 • edited

wizofaus commented Aug 10, 2021

Jojo-Schmitz commented Aug 10, 2021 • edited

wizofaus commented Aug 10, 2021

RobFog commented Aug 10, 2021

wizofaus commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vshalkevich commented Aug 16, 2021

wizofaus commented Aug 10, 2021 •

edited

Jojo-Schmitz commented Aug 10, 2021 •

edited

Jojo-Schmitz commented Aug 10, 2021 •

edited

Jojo-Schmitz commented Aug 10, 2021 •

edited