Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Paste note fixes #8825

Merged
merged 2 commits into from Aug 13, 2021
Merged

Paste note fixes #8825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c72065
Merge pull request #8821 from RomanPudashkin/instruments_panel_restor…
Eism Aug 9, 2021
dc91cae
[MU4] fix #8824 - fix pasting notes
Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/engraving/libmscore/chordrest.cpp
View file
Open in desktop
Expand Up @@ -519,16 +519,17 @@ Element* ChordRest::drop(EditData& data)

case ElementType::NOTE: {
Note* note = toNote(e);
NoteVal nval;
nval.pitch = note->pitch();
nval.tpc1 = note->tpc1();
nval.headGroup = note->headGroup();
nval.fret = note->fret();
nval.string = note->string();
score()->setNoteRest(segment(), track(), nval, ticks(), Direction::AUTO);
delete e;
Segment* seg = segment();
score()->undoRemoveElement(this);
Chord* chord = new Chord(score());
chord->setTrack(track());
chord->setDurationType(durationType());
chord->setTicks(ticks());
chord->setTuplet(tuplet());
chord->add(note);
score()->undoAddCR(chord, seg->measure(), seg->tick());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return note;
}
break;

case ElementType::HARMONY:
{
Expand Down Expand Up @@ -1433,7 +1434,7 @@ void ChordRest::removeMarkings(bool /* keepTremolo */)
bool ChordRest::isBefore(const ChordRest* o) const
{
if (!o || this == o) {
return true;
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevent asserts due to unstable std::sort predicates.

}
int otick = o->tick().ticks();
int t = tick().ticks();
Expand Down
15 changes: 11 additions & 4 deletions src/engraving/libmscore/cmd.cpp
View file
Open in desktop
Expand Up @@ -777,15 +777,19 @@ Segment* Score::setNoteRest(Segment* segment, int track, NoteVal nval, Fraction
Element* nr = nullptr;
Tie* tie = nullptr;
ChordRest* cr = toChordRest(segment->element(track));

Tuplet* tuplet = cr && cr->tuplet() && sd <= cr->tuplet()->ticks() ? cr->tuplet() : nullptr;
Measure* measure = nullptr;
bool targetIsRest = cr && cr->isRest();
for (;;) {
if (track % VOICES) {
expandVoice(segment, track);
}

if (targetIsRest && !cr->isRest()) {
undoRemoveElement(cr);
segment = addRest(segment, track, cr->ticks(), cr->tuplet())->segment();
}
// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (dd.isZero()) {
qDebug("cannot get gap at %d type: %d/%d", tick.ticks(), sd.numerator(),
Expand Down Expand Up @@ -846,7 +850,10 @@ Segment* Score::setNoteRest(Segment* segment, int track, NoteVal nval, Fraction
note->setTieFor(tie);
}
}
ncr->setTuplet(cr ? cr->tuplet() : 0);
if (tuplet && sd <= tuplet->ticks()) {
ncr->setTuplet(tuplet);
}
tuplet = 0;
undoAddCR(ncr, measure, tick);
if (addTie) {
undoAddElement(addTie);
Expand Down
2 changes: 1 addition & 1 deletion src/engraving/libmscore/edit.cpp
View file
Open in desktop
Expand Up @@ -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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dList = toDurationList(l, useDots);
std::reverse(dList.begin(), dList.end());
} else {
Expand Down
5 changes: 5 additions & 0 deletions src/engraving/libmscore/note.cpp
View file
Open in desktop
Expand Up @@ -2009,6 +2009,11 @@ Element* Note::drop(EditData& data)
n->setTpc2(Ms::transposeTpc(n->tpc1(), v, true));
// replace this note with new note
n->setParent(ch);
if (this->tieBack()) {
n->setTieBack(this->tieBack());
n->tieBack()->setEndNote(n);
this->setTieBack(nullptr);
}
score()->undoRemoveElement(this);
score()->undoAddElement(n);
}
Expand Down
117 changes: 80 additions & 37 deletions src/engraving/libmscore/paste.cpp
View file
Open in desktop
Expand Up @@ -1031,6 +1031,66 @@ void Score::pasteSymbols(XmlReader& e, ChordRest* dst)
} // inner while readNextstartElement()
} // pasteSymbolList()

static ChordRest* replaceWithRest(ChordRest* target)
{
target->score()->undoRemoveElement(target);
return target->score()->addRest(target->segment(), target->track(), target->ticks(), target->tuplet());
}

static Note* prepareTarget(ChordRest* target, Note* with, const Fraction& duration)
{
if (!target->segment()->element(target->track())) {
return nullptr; // target was removed by previous operation, ignore this
}
if (target->isChord() && target->ticks() > duration) {
target = replaceWithRest(target); // prevent unexpected note splitting
}
Segment* segment = target->segment();
if (segment->measure()->isMMRest()) {
Measure* m = segment->measure()->mmRestFirst();
segment = m->findSegment(SegmentType::ChordRest, m->tick());
}
segment = target->score()->setNoteRest(segment, target->track(),
with->noteVal(), duration, Direction::AUTO, false, {}, false, &target->score()->inputState());
return toChord(segment->nextChordRest(target->track()))->upNote();
}

static Element* prepareTarget(Element* target, Note* with, const Fraction& duration)
{
if (target->isNote() && toNote(target)->chord()->ticks() != duration) {
return prepareTarget(toNote(target)->chord(), with, duration);
}
if (target->isChordRest() && toChordRest(target)->ticks() != duration) {
return prepareTarget(toChordRest(target), with, duration);
}
return target;
}

static bool canPasteStaff(XmlReader& reader, const Fraction& scale)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (scale != Fraction(1, 1)) {
while (reader.readNext() && reader.tokenType() != XmlReader::TokenType::EndDocument) {
QString tag(reader.name().toString());
int len = reader.intAttribute("len", 0);
if (len && !TDuration(Fraction::fromTicks(len) * scale).isValid()) {
return false;
}
if (tag == "durationType") {
if (!TDuration(TDuration(reader.readElementText()).fraction() * scale).isValid()) {
return false;
}
}
}
}
return true;
}

inline static bool canPasteStaff(const QByteArray& mimeData, const Fraction& scale)
{
XmlReader reader(mimeData);
return canPasteStaff(reader, scale);
}

//---------------------------------------------------------
// cmdPaste
//---------------------------------------------------------
Expand All @@ -1052,55 +1112,36 @@ void Score::cmdPaste(const QMimeData* ms, MuseScoreView* view, Fraction scale)
if (!el) {
return;
}
duration *= scale;
if (!TDuration(duration).isValid()) {
return;
}

QList<Element*> els;
if (_selection.isSingle()) {
els.append(_selection.element());
} else {
els.append(_selection.elements());
}

Element* newEl = 0;
for (Element* target : els) {
el->setTrack(target->track());
Element* nel = el->clone();
addRefresh(target->abbox()); // layout() ?!
EditData ddata(view);
ddata.dropElement = nel;
ddata.dropElement = el.get();
if (target->acceptDrop(ddata)) {
if (el->isNote()) {
// dropping a note replaces and invalidates the target,
// so we need to deselect it
ElementType targetType = target->type();
deselect(target);

// perform the drop
target->drop(ddata);

TDuration durationType(duration);
if (target->isChordRest()) {
ChordRest* targetCR = dynamic_cast<ChordRest*>(target);
score()->changeCRlen(targetCR, durationType);
} else if (target->isNote()) {
ChordRest* targetCR = dynamic_cast<Note*>(target)->chord();
score()->changeCRlen(targetCR, durationType);
if (!el->isNote() || (target = prepareTarget(target, toNote(el.get()), duration))) {
ddata.dropElement = newEl = el->clone();
Element* dropped = target->drop(ddata);
if (dropped) {
newEl = dropped;
}

// if the target is a rest rather than a note,
// a new note is generated, and nel becomes invalid as well
// (ChordRest::drop() will select it for us)
if (targetType == ElementType::NOTE) {
select(nel);
}
} else {
target->drop(ddata);
}
if (_selection.element()) {
addRefresh(_selection.element()->abbox());
}
} else {
delete nel;
}
}
if (newEl) {
select(newEl);
}
} else if ((_selection.isRange() || _selection.isList()) && ms->hasFormat(mimeStaffListFormat)) {
ChordRest* cr = 0;
if (_selection.isRange()) {
Expand Down Expand Up @@ -1128,10 +1169,12 @@ void Score::cmdPaste(const QMimeData* ms, MuseScoreView* view, Fraction scale)
if (MScore::debugMode) {
qDebug("paste <%s>", data.data());
}
XmlReader e(data);
e.setPasteMode(true);
if (!pasteStaff(e, cr->segment(), cr->staffIdx(), scale)) {
return;
if (canPasteStaff(data, scale)) {
XmlReader e(data);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

e.setPasteMode(true);
if (!pasteStaff(e, cr->segment(), cr->staffIdx(), scale)) {
return;
}
}
}
} else if (ms->hasFormat(mimeSymbolListFormat)) {
Expand Down