Skip to content

Fix #17548 Allow melisma to be deleted (or adjusted) on chordrest duration change or lyric entry #17575

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.

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

Merged
merged 3 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/engraving/layout/v0/lyricslayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ void LyricsLayout::layout(Lyrics* item, LayoutContext& ctx)
ChordRest* cr = item->chordRest();
if (item->_removeInvalidSegments) {
item->removeInvalidSegments();
} else if (item->_ticks > Fraction(0, 1) || item->_syllabic == LyricsSyllabic::BEGIN || item->_syllabic == LyricsSyllabic::MIDDLE) {
}
if (item->_ticks > Fraction(0, 1) || item->_syllabic == LyricsSyllabic::BEGIN || item->_syllabic == LyricsSyllabic::MIDDLE) {
if (!item->_separator) {
item->_separator = new LyricsLine(ctx.mutDom().dummyParent());
item->_separator->setTick(cr->tick());
Expand Down Expand Up @@ -279,13 +280,15 @@ void LyricsLayout::layout(LyricsLine* item, LayoutContext& ctx)
ps = ps->prev1(SegmentType::ChordRest);
}
if (!ps || ps == lyricsSegment) {
// no valid previous CR, so try to lengthen melisma instead
// either there is no valid previous CR, or the previous CR is the one the lyric starts on
// we don't want to make the melisma longer arbitrarily, but there is a possibility that the next
// CR won't extend the melisma, so let's check it
ps = ns;
s = ps->nextCR(lyricsTrack, true);
EngravingItem* e = s ? s->element(lyricsTrack) : nullptr;
// check to make sure we have a chord
if (!e || e->type() != ElementType::CHORD) {
// nothing to do but set ticks to 0
if (!e || e->type() != ElementType::CHORD || ps->tick() > item->tick() + item->ticks()) {
// nope, nothing to do but set ticks to 0
// this will result in melisma being deleted later
item->lyrics()->undoChangeProperty(Pid::LYRIC_TICKS, Fraction::fromTicks(0));
item->setTicks(Fraction(0, 1));
Expand Down
8 changes: 7 additions & 1 deletion src/engraving/layout/v0/tlayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5327,6 +5327,9 @@ SpannerSegment* TLayout::layoutSystemSLine(SLine* line, System* system, LayoutCo

SpannerSegment* TLayout::layoutSystem(LyricsLine* line, System* system, LayoutContext& ctx)
{
if (!line->lyrics()) {
return nullptr; // a lyrics line with no lyrics shouldn't exist
}
Fraction stick = system->firstMeasure()->tick();
Fraction etick = system->lastMeasure()->endTick();

Expand Down Expand Up @@ -5402,7 +5405,10 @@ SpannerSegment* TLayout::layoutSystem(LyricsLine* line, System* system, LayoutCo
}

TLayout::layout(lineSegm, ctx);

if (!line->lyrics()) {
// this line could have been removed in the process of laying out surrounding lyrics
return nullptr;
}
// if temp melisma extend the first line segment to be
// after the lyrics syllable (otherwise the melisma segment
// will be too short).
Expand Down
2 changes: 1 addition & 1 deletion src/engraving/libmscore/line.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,7 @@ PointF SLine::linePos(Grip grip, System** sys) const
}
}
}
} else if (isLyricsLine() && toLyrics(explicitParent())->ticks() > Fraction(0, 1)) {
} else if (isLyricsLine() && explicitParent() && toLyrics(explicitParent())->ticks() > Fraction(0, 1)) {
// melisma line
// it is possible CR won't be in correct track
// prefer element in current track if available
Expand Down
35 changes: 31 additions & 4 deletions src/engraving/libmscore/lyrics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,37 @@ bool Lyrics::isEditAllowed(EditData& ed) const
return TextBase::isEditAllowed(ed);
}

void Lyrics::adjustPrevious()
{
Lyrics* prev = prevLyrics(toLyrics(this));
if (prev) {
// search for lyric spanners to split at this point if necessary
if (prev->tick() + prev->ticks() >= tick()) {
// the previous lyric has a spanner attached that goes through this one
// we need to shorten it
Segment* s = score()->tick2segment(tick());
if (s) {
s = s->prev1(SegmentType::ChordRest);
if (s->tick() > prev->tick()) {
prev->undoChangeProperty(Pid::LYRIC_TICKS, s->tick() - prev->tick());
} else {
prev->undoChangeProperty(Pid::LYRIC_TICKS, Fraction::fromTicks(1));
}
}
}
prev->setRemoveInvalidSegments();
prev->triggerLayout();
}
}

//---------------------------------------------------------
// endEdit
//---------------------------------------------------------

void Lyrics::endEdit(EditData& ed)
{
TextBase::endEdit(ed);

triggerLayoutAll();
}

Expand Down Expand Up @@ -373,6 +397,9 @@ PropertyValue Lyrics::getProperty(Pid propertyId) const

bool Lyrics::setProperty(Pid propertyId, const PropertyValue& v)
{
ChordRest* scr = nullptr;
ChordRest* ecr = nullptr;

switch (propertyId) {
case Pid::PLACEMENT:
setPlacement(v.value<PlacementV>());
Expand All @@ -389,14 +416,14 @@ bool Lyrics::setProperty(Pid propertyId, const PropertyValue& v)
// endTick info is wrong.
// Somehow we need to fix this.
// See https://musescore.org/en/node/285304 and https://musescore.org/en/node/311289
ChordRest* ecr = score()->findCR(endTick(), track());
ecr = score()->findCR(endTick(), track());
if (ecr) {
ecr->setMelismaEnd(false);
}
}

scr = score()->findCR(tick(), track());
_ticks = v.value<Fraction>();
if (_ticks <= Fraction(0, 1)) {
if (scr && _ticks <= scr->ticks()) {
// if no ticks, we have to relayout in order to remove invalid melisma segments
setRemoveInvalidSegments();
layout()->layoutItem(this);
Expand Down Expand Up @@ -520,8 +547,8 @@ void Lyrics::removeInvalidSegments()
_removeInvalidSegments = false;
if (_separator && isMelisma() && _ticks < _separator->startCR()->ticks()) {
setTicks(Fraction(0, 1));
_separator->setTicks(Fraction(0, 1));
_separator->removeUnmanaged();
delete _separator;
_separator = nullptr;
setAlign(propertyDefault(Pid::ALIGN).value<Align>());
if (_syllabic == LyricsSyllabic::BEGIN || _syllabic == LyricsSyllabic::SINGLE) {
Expand Down
3 changes: 2 additions & 1 deletion src/engraving/libmscore/lyrics.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class Lyrics final : public TextBase
Fraction endTick() const;
void removeFromScore();
void setRemoveInvalidSegments() { _removeInvalidSegments = true; }
void adjustPrevious();

using EngravingObject::undoChangeProperty;
void paste(EditData& ed, const String& txt) override;
Expand Down Expand Up @@ -141,7 +142,7 @@ class LyricsLine final : public SLine

Lyrics* lyrics() const { return toLyrics(explicitParent()); }
Lyrics* nextLyrics() const { return _nextLyrics; }
bool isEndMelisma() const { return lyrics()->ticks().isNotZero(); }
bool isEndMelisma() const { return lyrics() && lyrics()->ticks().isNotZero(); }
bool isDash() const { return !isEndMelisma(); }
bool setProperty(Pid propertyId, const PropertyValue& v) override;

Expand Down
5 changes: 5 additions & 0 deletions src/engraving/libmscore/textedit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,11 @@ void TextBase::endEdit(EditData& ed)
} else {
triggerLayout();
}
if (isLyrics()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this code here (in the base class) and not inside Lyrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because these are adjustments that have to be made to the lyric when finished editing a different lyric. We could move this, but I'm not sure the best place to do it. Would we set a flag here and the lyric is adjusted the next time it lays out?

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 idea behind this section is that when a lyric is finished editing, it checks to see if there is a previous lyric with a line that extends through the current lyric's tick, and if so, resizes the previous lyric's line (or removes it) so that there is no overlap.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could move this, but I'm not sure the best place to do it

This code looks like it should be inside the Lyrics class (since it's a lyrics-specific logic and TextBase is the base class for all text elements)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still elected to call it from the base class, because the base class endEdit finishes the undo command, and it also has some early return logic that I felt would be a giant waste to copy and paste into the lyrics class. If this is still not good enough, I can move it into the lyrics class and replicate the early return checks from the base class.

// we must adjust previous lyrics before the call to commitText(), in order to make the adjustments
// part of the same undo command. there is logic above that will skip this call if the text is empty
toLyrics(this)->adjustPrevious();
}

static const double w = 2.0;
score()->addRefresh(canvasBoundingRect().adjusted(-w, -w, w, w));
Expand Down