-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 #279770: lyrics align is wrong on lyrics containing numbers and space #5107
Conversation
I didn't build it myself, but the code looks good and the examples shown in the issue thread look good! |
Thanks for the official blessing from the Master of Melismas ;-) |
@@ -261,8 +261,8 @@ void Lyrics::layout() | |||
Lyrics trailing(*this); | |||
trailing.setPlainText(tp); | |||
trailing.layout1(); | |||
leftAdjust = leading.width(); | |||
centerAdjust = leading.width() - trailing.width(); | |||
leftAdjust = leading.width() + symWidth(SymId::noteheadBlack) * 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I would prefer to apply another solution for it. It looks more like a happy coincidence that adding exactly half of a notehead's width is necessary to resolve the issue for default lyrics font face and size (and score style settings perhaps?). Changing font face or font size with this code results in something like this (the second line is with default settings so is aligned correctly):
If you would still like some solution to be applied perhaps this commit could be considered (I mentioned it in the issue's thread). It doesn't work correctly if lyrics font or size is defined via controls available in text editing mode but works correctly for any font face and size provided they are set to lyrics via Inspector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. Maybe both, and my fix commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe they could be both, but I really fail to see how the problem of aligning a text line to the given position is related to a notehead width :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because currenly the start of the letter seems to get aligned to the center to the notehead, rather than to its left edge. Or the center of the letter to the noteheads right edge.
A few lines later in the code the same method is used, there too the nothead width gets determined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because currenly the start of the letter seems to get aligned to the center to the notehead, rather than to its left edge. Or the center of the letter to the noteheads right edge.
The experiment with changing font face/size (both via Inspector and text editing panel) seems to prove the opposite.
A few lines later in the code the same method is used, there too the nothead width gets determined
That code exactly tries to center the lyrics line under the notehead if alignment is set to HCENTER
. For that task it is indeed necessary to know the notehead width (although I wonder why noteheadBlack
is used while the actual notehead may differ). The problem which the discussed code is trying to solve is just to determine width of leading and trailing parts of the lyrics to use that in the alignment algorithm later, and those widths do not depend on a notehead size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, close this PR then and apply your fix. Main thing is to get this fixed.
The issue might come back though, should that #if 0
ever gets changed...
See e129be0. |
No description provided.