-
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
Properly deal with </font>
#21972
base: master
Are you sure you want to change the base?
Properly deal with </font>
#21972
Conversation
a3e4503
to
662f6be
Compare
0f867ec
to
6d975ee
Compare
I suddenly wondered: can |
Well, yes, theoretically they could, but as in the use case I have in mind they'd be manually added, we might neglect that problem? |
As far as I can tell MuseScore itself never writes |
Ah, yes, I see. MuseScore rather writes I wonder a little bit though whether it makes sense at all to try to support reading constructions that MuseScore itself never writes. But okay, this change won't do any harm probably, and it's quite logical. Maybe we should someday adjust the writing code to this :) |
6d975ee
to
cb0b648
Compare
Added protection from storing prev font size and face when not using an end tag |
8f468ac
to
de120c3
Compare
For QA: this PR just needs to be tested for regressions in saving-and-reloading and copy-pasting of formatted text, since the added functionality is not visible to users |
26f4474
to
daf4541
Compare
The last commits ain't really mandatory... More a 1st step towards that Maybe we should someday adjust the writing code to this |
d98742c
to
902a21a
Compare
The unit test failure of my latest changes (2nd to last commit) reveal that this code is also used for MusicXML im-/export. Sorry, @cbjeukendrup, guess I'd need another review |
Ah, I know now what causes this: the space in the font face name is causing the string separation to fail... |
Maybe something in the direction of: } else if (token.startsWith(u"font ")) {
String remainder = token.mid(5); // maybe add `.trimmed() here just to be safe?
for (; !remainder.empty();) {
if (remainder.startsWith(u"size=\"")) {
// the value of the `size` attribute starts at index 6
// find the first index of a double quote _after_ index 6
size_t endQuoteIndex = remainder.indexOf(u'"', 6);
double fontSize = remainder.mid(6, endQuoteIndex - 6).toDouble();
// now do things with fontSize
// and adjust `remainder` for the next attribute
remainder = remainder.mid(endQuoteIndex + 1).trimmed(); // remove whitespace from beginning/end
} else if (remainder.startsWith(u"face=\"")) {
// basically the same but without `toDouble()`
} else {
LOGD("cannot parse html property <%s> in text <%s>", muPrintable(s), muPrintable(m_text));
return false;
}
}
return true;
} else if (token == u"/font") { Of course those lengthy messy comments are only for illustration :) |
ac002fc
to
ffd77be
Compare
OK, I believe this PR is OK now |
@Jojo-Schmitz @cbjeukendrup Tested on Win10. It doesn't save font size settings (after copy/paste text) after reopening the score. bandicam.2024-04-05.18-44-09-052.mp4 |
Also, there is an issue when formatting text in Staff/Part properties dialog>Text frame, but the same in master (will log it separately) bandicam.2024-04-05.18-50-21-864.mp4 |
Not really the purpose of the PR. It is to allow partial formating strings that can't be partially formatted using the UI, like instrument names, and header and footer texts. |
The lost linefeed on save/reload is indeed odd. |
I think that already exists as #19189 |
@Jojo-Schmitz Fact is, that saving/reloading fails for texts where the font face or size of (part of) the text is changed, which is a regression on master. |
} | ||
format.setFontFamily(face); | ||
} else { | ||
LOGD("cannot parse html property <%s> in text <%s>", muPrintable(remainder), muPrintable(m_text)); |
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 code seems to get hit with the /
from <font />
, maybe that is related to the problem
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.
OK, let's break
earlier on that case
That (alone) doesn't help though.
I'm at a loss to find where and why it fails
5bc9686
to
cf319a2
Compare
instead of ignoring it altogether, reset font face and -size back to what it was before `<font ...>`
to enable the reset to style button
cf319a2
to
bfcf2aa
Compare
Rebased to check whether #22475 makes a difference |
Hmm, why is this engraving test failing? 9: --- /home/runner/work/MuseScore/MuseScore/src/engraving/tests/expression_data/expression-1-ref.mscx 2024-04-20 09:36:41.516683602 +0000
9: +++ expression-1.mscx 2024-04-20 09:59:49.129177961 +0000
9: @@ -146,6 +146,7 @@
9: <Expression>
9: <placement>above</placement>
9: <snapToDynamics>0</snapToDynamics>
9: + <italic>0</italic>
9: <offset x="0" y="-4.72943"/>
9: <text><i>expr </i><u>text</u></text>
9: </Expression>
9: <diff -u /home/runner/work/MuseScore/MuseScore/src/engraving/tests/expression_data/expression-1-ref.mscx expression-1.mscx failed, code: 1 As the italic is explicity set by the Quite a few musicxml tests fail too, seems to be a very similar issue, adding a tag that doesn't really seem needed, but isn't wrong either: 14: +++ testWedgeOffset.mscx 2024-04-20 11:07:30.792268251 +0000
14: @@ -122,6 +122,7 @@
14: </Text>
14: <Text>
14: <style>subtitle</style>
14: + <size>16</size>
14: <offset x="0" y="9.92425"/>
14: <text><font size="16"/>MuseScore Testcase</text>
14: </Text>
14: <diff -u /home/runner/work/MuseScore/MuseScore/src/importexport/musicxml/tests/data/testWedgeOffset_ref.mscx testWedgeOffset.mscx But only for subtitle, not for title?!? Anyway, none these failed prior to the rebase as far as I remeber, what other changes might have caused this?
But maybe it has fixed this? |
instead of ignoring it altogether, reset font face and -size back to what it was before
<font ...>
To allow stuff like this in instrument names:
Which currently results in the last part of the string ("Alt") to also be in size (8) of the middle part instead of having the same size as the 1st part ("Sopran"):
Now looks like this>
Also useful for header/footer texts, which also can't get formatted with different parts