-
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 GH#16363: Accidentals in chord symbols rendered incorrectly when using custom XML file #22541
Fix GH#16363: Accidentals in chord symbols rendered incorrectly when using custom XML file #22541
Conversation
6e1b355
to
9cf11c6
Compare
bdab1b0
to
0690feb
Compare
src/engraving/dom/chordlist.cpp
Outdated
if (!code.startsWith(u"0x") && !code.startsWith(u"0") && !code.startsWith('&') && !code.endsWith(';')) { | ||
// fix broken chord lists, see https://github.com/musescore/MuseScore/issues/16363 | ||
code = u"0x" + code; | ||
} | ||
bool ok = true; | ||
char32_t val = code.toUInt(&ok, 0); |
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.
If code.startsWith('&')
is true, then this would fail anyway. Does this case occur at all?
If not, let's not check for it, because that only makes the code noisy
If yes, we should add proper handling for that case.
However, I'm not very confident whether I like this approach; it means that our String::number
method behaves differently from QString::number
, which seems a potential source of confusion.
Wouldn't the problem be fixed instantly if you do
char32_t val = code.toUInt(&ok, 0); | |
char32_t val = code.toUInt(&ok, 16); |
and drop all other changes? Or is it not certain that it will be a base16 number?
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.
If
code.startsWith('&')
is true, then this would fail anyway. Does this case occur at all? If not, let's not check for it, because that only makes the code noisy If yes, we should add proper handling for that case.
We don't have full control over the content of a chords.xml, it could contain ♭
(@MarcSabatella even recommeded the latter in the issue as a workaround, #16363 (comment)).
The toUInt(..., 0)
would fail on it (as would a toUInt(..., 16)
), but return the string unchanged, as far as I understand.
Be generous in what you accept, but strict in what you generate ;-) (and that's why toUInt(..., 0)
exists)
However, I'm not very confident whether I like this approach; it means that our
String::number
method behaves differently fromQString::number
, which seems a potential source of confusion. Wouldn't the problem be fixed instantly if you do
...
and drop all other changes? Or is it not certain that it will be a base16 number?
The problem isn't that toUInt()
doesn't return a hex number, but that this number as a string doesn't look like one to the writer because of the missing "0x".
Also, as said above, it might be an octal number od a string with an HTML entity, we don't have full control over the content of these files.
But we can leave the String::number() as it was and just do it like in the 1st commit and prepend the "0x" there,
after all we do know though that we write as hex there.
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 can understand your concerns reg. different behavoir of String::number()
vs. QString::number()
, so reverted that 2nd commit
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.
And we may not need the read fix at all, as save/close/open fixes the issue too, its just nice to have, isn't it?
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.
BTW: this would work too:
int base = 0; // guess, based on content
if (!code.startsWith(u"0") && !(code.startsWith(u"&#") && code.endsWith(u";"))) {
// fix broken chord lists, see https://github.com/musescore/MuseScore/issues/16363
base = 16; // force hex
}
bool ok = true;
char32_t val = code.toUInt(&ok, base);
And it does read that ♭
too, not a ©
though, toUInt()
(or rather rather std::strtol()
) probably just ignores all the non-alpha-numerical chars and (rightfully) interprets the rest as decimal.
7fcc2b6
to
8b69c04
Compare
Came up again in https://musescore.org/en/node/364348 |
Came up again in https://musescore.org/en/node/365531 |
8b69c04
to
a9c472e
Compare
Seems time to get this one merged as well? |
Yes, but I want/need to look into this a bit more closely, because I haven't quite understood exactly what's going on. At the moment I don't have time for that, but in a week, the academic year is over, which should improve that 😉 |
OK, then there's an alternative fix now too ;-) |
Came up again in https://musescore.org/en/node/365770 |
258be08
to
15bf26d
Compare
Would now be the time? ;-) |
It is, undeniably. I had to gather some courage but I'm on it right now :) |
Alright, I found the problem. Literally the only change that's needed is: Why is this needed? It was removed in https://github.com/musescore/MuseScore/pull/11892/files#diff-34fc1099345fb1b9e716cbe1602692db544d21858204ce45642aa36e99f779af. (Found using Git Blame with "blame prior to change …" a couple of times) Why are other changes not needed?
While this does prevent new scores from breaking, it doesn't fix already-broken scores.
|
Hmm I'm confused now, it was me removing that Or are you saying that your #22541 basically makes this PR here obsolete, as it fixes the issue on the next write? |
No, #23759 fixes an entirely different problem, namely that the chordlist was not written anymore at all after reopen-resave. The In your second commit, you basically undid both these changes, and implemented an alternative fix at So what you need to do is:
I hope that clarifies it! |
Ah, I see now! |
15bf26d
to
e2af1f1
Compare
Done |
…using custom XML file
e2af1f1
to
e5dee71
Compare
Resolves: #16363