-
Notifications
You must be signed in to change notification settings - Fork 3k
Text line style #30151
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
Text line style #30151
Conversation
afc3245
to
263300f
Compare
Looks great! From a quick scan, the bits of the original task yet to be resolved are:
We could create follow-up issues for both. Was there anything else? |
263300f
to
ae4599d
Compare
@avvvvve I believe that's it. I would recommend double click to edit to be implemented at the same time or after independent styles for beginning/middle/end text. |
ae4599d
to
c970497
Compare
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.
Very few comments, mostly just questions. Great work!
bool onSameScore(const EngravingObject* other) const; | ||
const MStyle& style() const; | ||
|
||
virtual EngravingObject* propertyDelegate(Pid) const { return nullptr; } |
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.
Could you explain why this change was necessary? I'm not sure I get it from just looking at the code
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.
This one wasn't 100% necessary - I wanted to add a property delegate call to propertyFlags
and casting to EngravingItem
immediately for such a basic operation felt wrong. It's also the only property related function which isn't in EngravingObject
.
: TextLineBaseSegment(ElementType::HAIRPIN_SEGMENT, sp, parent, ElementFlag::MOVABLE | ElementFlag::ON_STAFF) | ||
{ | ||
m_text->setTextStyleType(propertyDefault(Pid::TEXT_STYLE).value<TextStyleType>()); | ||
m_endText->setTextStyleType(propertyDefault(Pid::TEXT_STYLE).value<TextStyleType>()); |
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.
Perhaps can be rewritten more concisely as m_text->resetProperty(Pid::TEXT_STYLE)
?
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.
Using resetProperty
calls initTextStyleType
which resets all text style properties. For text lines, these are stored in TextLineBase
. Because this is being called in the segment constructor, this happens at layout. This caused issues with opening files, as it would wipe any custom formatting the user had saved on first layout of the score.
return parent(); | ||
default: | ||
return nullptr; | ||
} |
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.
This function reads a bit clunky. For a function this simple I'd perhaps put the positive case first so it's easier to read, e.g.
if (id == Pid::TEXT_STYLE && parent()->isTextLineBaseSegment()) {
return parent();
}
return nullptr;
Also, I think parent() may be null in some cases so perhaps nullchecking it before using it is worth it just to be safe
const TextFragment* tf = ldata->textBlock(static_cast<int>(startRow)).fragment(static_cast<int>(selectionStartCol)); | ||
CharFormat resultFormat = tf ? tf->format : CharFormat(); | ||
|
||
bool allBlocksEmpty = true; |
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.
Could you explain what problem this is solving and why normal text (i.e. text that doesn't belong to a TextLineBase) didn't need this?
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.
The issue here was that getting font face/size/style from empty text with no fragments returned "" for font face, 12 for size and FontStyle::Normal
for style no matter the value for these properties in the text style. This wasn't visible before because selecting an empty text item isn't possible and editing text to be empty doesn't remove the last fragment.
It is now visible because we are using the properties panel to edit the properties of at least 2 text items at once. For example, a volta has beginning text of "1." and end text of "". Without these changes, "1." has a font face of "Edwin" while the end text has a font face of "". This puts the properties panel in an indeterminate state:
switch (propertyId) { | ||
case Pid::BEGIN_FONT_FACE: | ||
setPropertyValue(m_elementList, Pid::CONTINUE_FONT_FACE, newValue); | ||
setPropertyValue(m_elementList, Pid::END_FONT_FACE, newValue); |
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 wonder if it would be more solid to handle this in the setProperty method. That would guarantee that any time anybody sets Pid::BEGING_FONT_... you also set CONTINUE_FONT_... and END_FONT_....
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.
Ah, maybe that's not possible because of back compatibility when reading older files.
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.
Yes, we need to use the settings to enforce this for compatibility
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.
Couple of comments. I haven't completely finished reviewing yet, so some more might follow later.
src/engraving/rw/read410/tread.cpp
Outdated
{ | ||
if (tag == propertyName(pid)) { | ||
if (pid == Pid::BEGIN_TEXT_OFFSET) { | ||
LOGI() << "ofs"; |
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.
Left-over log
src/engraving/dom/whammybar.cpp
Outdated
#include "dom/text.h" | ||
#include "score.h" | ||
#include "system.h" |
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.
Nitpick:
#include "dom/text.h" | |
#include "score.h" | |
#include "system.h" | |
#include "score.h" | |
#include "system.h" | |
#include "text.h" |
same in other files. But I can also clean this up myself, I'll be doing an includes cleanup PR anyway.
src/engraving/dom/textlinebase.cpp
Outdated
case Pid::BEGIN_FONT_FACE: | ||
case Pid::BEGIN_FONT_SIZE: | ||
case Pid::BEGIN_FONT_STYLE: | ||
return style().styleV(getPropertyStyle(propertyId)); |
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.
Why do specifically the begin
properties have this, and why can't it go via SLine/EngravingObject::propertyDefault?
src/engraving/dom/textlinebase.cpp
Outdated
bool TextLineBase::setProperty(Pid id, const PropertyValue& v) | ||
{ | ||
switch (id) { | ||
/// In >=4.6 text line begin, continue, and end text should all have the same style |
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.
The comment doesn't seem to match the method; perhaps the comment should rather be in the inspector code? The fact that they all have to be the same is (and should be) only a temporary UI limitation, but in the engraving code nothing essential is changed, is it?
Return defaults
Text line styles use different Pids. Ensure the correct Pid is used.
Also tidy the files a bit
e7c5113
to
7ab75ea
Compare
Resolves: #14238 (almost entirely)
This PR enables styling of text line's text via the properties panel. Currently, beginning, continuing and end text are all controlled by the same text panel and therefore must have the same style. However, MuseScore 3 scores with different styles for beginning, continuing and end text will still preserve these differences.

Screen.Recording.2025-09-29.at.14.22.14.mov
The PR also fixes a bug involved in applying text line text styles (eg. Volta) to regular text (eg. Staff Text) and vice versa.