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.

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

fix #289211: bad layout of lyrics dashes at end of system #5032

Merged
merged 1 commit into from
May 21, 2019

Conversation

MarcSabatella
Copy link
Contributor

See https://musescore.org/en/node/289211

This code in my previous PR #4946 was meant to apply only to melisma lines - they are the only ones for which lyric ticks is relevant. But I forgot we use the same data structure for dashes, distinguished only by this very value (zero ticks means dashes, non-zero means melisma). So I've just added a check for that to reinstate the original behavior for dashes.

@anatoly-os
Copy link
Contributor

@MarcSabatella isn't its better to introduce two methods like isMelisma and isDashes and encapsulate the zero-check there?

@MarcSabatella
Copy link
Contributor Author

No doubt. Will do, this has bugged me long enough.

@MarcSabatella
Copy link
Contributor Author

Done. I called the function is isEndMelisma() because technically, it's a melisma even when dashes are used if multiple notes are spanned (and there is already a Lyrics::isMelisma() specifically to test for that). I also added isDash() but never had an opportunty to use it, the code as it is always just tests the end melisma case; the dash case is just an "else" when it happens.

@@ -126,6 +126,8 @@ class LyricsLine final : public SLine {

Lyrics* lyrics() const { return toLyrics(parent()); }
Lyrics* nextLyrics() const { return _nextLyrics; }
bool isEndMelisma() const { return lyrics()->ticks().isNotZero(); }
bool isDash() const { return lyrics()->ticks().isZero(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Using !isEndMelisma is better to reduce number of code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@anatoly-os anatoly-os merged commit 77105cd into musescore:master May 21, 2019
@anatoly-os
Copy link
Contributor

anatoly-os commented May 21, 2019

Merged. Btw, there is still an inconsistency in my mind.

Why is isDash a reverse meaning of isEndMelisma? Shouldn't it be isDash and isMelisma? If the latter is true, why do we check the melisma presence by checking the zero tick?

@MarcSabatella
Copy link
Contributor Author

Confusing, I agree, but the way I have it is accurate.

"Melisma" is a general term for any syllable that spans multiple notes. It includes all end melismas (created with an underscore and displayed with a solid line, traditionally used at the end of a word like "I wanna hold your hand_ _ _ _ _ _ _") as well as melismas in the middle of words (created with a dash). But not all dashes indicate melismas. "Ma-ry had a lit-tle lamb" - those dashes don't indicate melismas, because the syllable "Ma" and "lit" don't span multiple notes. But the dashes in "Glo - - - - - - - - - - - - - - - ri-a in excelsis deo" do indicate a melisma.

So, the "isMelisma()" function for lyrics returns true for both the "hand_" and "glo-ria" cases, which is important because those need to be left aligned rather than centered. But the new functions are not for lyrics, they are for the lines themselves, and the dashes don't actually care if they represent melismas or not. They just need to know whether to draw a solid line or to draw dashes. So, isMelisma() isn't a useful distinction here. We just need to know if the line should be drawn solid (true for end melismas like "hand_") or dashed (true for both "Mar-ry" and "Glo-ria", even though one is a melisma and the other isn't). So, isEndMelisma() versus isDash() for the lyricsline is what captures this. And that is the distinction my code for the PR needed to make.

Probably some mechanism other than checking the ticks on the lyric should have been invented for representing this, but historically, lyricsline wasn't even a thing - all of these lines were generated on the fly during layout. But it didn't work well across systems breaks, which is why the concept of the lyricsline as an "unmanaged spanner" was introduced for 2.0.

Probably more than you wanted to know :-)

@anatoly-os
Copy link
Contributor

"they are for the lines themselves, and the dashes don't actually care if they represent melismas or not. " That's why I asked the question. The methods are in the Lyrics related class. That's why I would expect the meaning of isMelisma be aligned with the lyrics semantic, not the lines semantic. I suggest renaming to isMelismaLine and isDashLine which changes the semantic.

@MarcSabatella
Copy link
Contributor Author

The methods are in LyricsLine class, not the Lyrics class. They are defined in the same header (again, historical reasons).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants