Fix GH#16363: Accidentals in chord symbols rendered incorrectly when using custom XML file #22541

Jojo-Schmitz · 2024-04-23T12:27:54Z

Resolves: #16363

src/engraving/dom/chordlist.cpp

src/engraving/dom/page.cpp

cbjeukendrup · 2024-04-23T14:39:28Z

src/engraving/dom/chordlist.cpp

+ 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);


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

Suggested change

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?

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 from QString::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.

I can understand your concerns reg. different behavoir of String::number() vs. QString::number(), so reverted that 2nd commit

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?

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.

Jojo-Schmitz · 2024-05-21T09:06:25Z

Came up again in https://musescore.org/en/node/364348

Jojo-Schmitz · 2024-06-23T08:56:15Z

Came up again in https://musescore.org/en/node/365531

Jojo-Schmitz · 2024-06-23T21:12:17Z

Seems time to get this one merged as well?

cbjeukendrup · 2024-06-23T21:25:00Z

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 😉

Jojo-Schmitz · 2024-06-23T21:27:05Z

OK, then there's an alternative fix now too ;-)
(Actually pushed by accident, but hey, just let me know whether it is good or not)

Jojo-Schmitz · 2024-07-01T15:03:14Z

Came up again in https://musescore.org/en/node/365770

Jojo-Schmitz · 2024-07-24T16:42:16Z

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 😉

Would now be the time? ;-)

cbjeukendrup · 2024-07-24T22:33:20Z

It is, undeniably. I had to gather some courage but I'm on it right now :)

cbjeukendrup · 2024-07-25T01:40:25Z

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 was it removed? My guess: Human Error.

Why are other changes not needed?

Explicitly setting the base is not necessary because String::toUInt uses std::strtol which behaves pretty much exactly the same as QString's toUInt. Passing a base of 0 decides based on whether the string is prefixed with 0x, 0b, 0 or nothing. So reading files containing 0x Just Works.
What about ♭ or ♭ notation? That also Just Works, because that is already substituted by the XML reader (I suppose; or at least, somewhere earlier in the pipeline). So instead of receiving ♭, we receive a ♭ symbol. That can obviously not be converted to a number, so we hit this case:
```
if (!ok) {
    cs.code = 0;
    cs.value = code;
} else if (Char::requiresSurrogates(val)) {
```
(the bottom one, I suppose).
Also, note that in this notation, &#nnnn; interprets nnnn as a decimal number, while &#xnnnn; (without leading 0) interprets nnnn as a hexadecimal. This is standard, and works out of the box, apparently even with our XML reader.

While this does prevent new scores from breaking, it doesn't fix already-broken scores.
I propose we don't do anything about this, for the following reasons:

it would make the code even more complicated
deciding between "a hex number with 0x", "a potentially-but-not-necessarily multi-byte character", "a decimal number" (that might be seen in a user-made chords file), and "a hex number that misses 0x", is basically impossible, especially because of the ambiguity between the latter two.
these broken chords xml files that MS4 wrote, never survived very long, because of a different bug I just discovered (and fixed)... see Fix that custom ChordList was not written to file anymore after reopening and resaving #23759

Jojo-Schmitz · 2024-07-25T06:50:31Z

Hmm I'm confused now, it was me removing that u"0x" + , along with a comment // write hex numbers with a "0x" prefix, so they can convert back properly on read, in my 2nd commit (alternative fix) of this PR here.

Or are you saying that your #22541 basically makes this PR here obsolete, as it fixes the issue on the next write?

cbjeukendrup · 2024-07-25T13:08:22Z

No, #23759 fixes an entirely different problem, namely that the chordlist was not written anymore at all after reopen-resave.

The u"0x" + was not there before this PR; as I said, it was accidentally lost in https://github.com/musescore/MuseScore/pull/11892/files#diff-34fc1099345fb1b9e716cbe1602692db544d21858204ce45642aa36e99f779af.
Your first commit added the u"0x" + back (along with a comment). And that turns out to be the correct solution, as I found when researching this yesterday.
But your first commit also does something with &#…; notation, and as I pointed out, that is unnecessary, so that change should be dropped.

In your second commit, you basically undid both these changes, and implemented an alternative fix at String::toUInt level. I think this alternative is not preferable; I'd like to prevent confusions and keep our String in sync with Qt's behaviour.

So what you need to do is:

drop the alternative fix (second commit), because the initial one was correct
drop the &#…; change from the first commit, because that is unnecessary

I hope that clarifies it!

Jojo-Schmitz · 2024-07-25T13:13:45Z

Ah, I see now!

Jojo-Schmitz · 2024-07-25T13:17:04Z

Done

…using custom XML file

Jojo-Schmitz force-pushed the broken-chord-symbols branch from 6e1b355 to 9cf11c6 Compare April 23, 2024 12:31

Jojo-Schmitz commented Apr 23, 2024

View reviewed changes

src/engraving/dom/chordlist.cpp Show resolved Hide resolved

Jojo-Schmitz force-pushed the broken-chord-symbols branch 3 times, most recently from bdab1b0 to 0690feb Compare April 23, 2024 13:43

Jojo-Schmitz commented Apr 23, 2024

View reviewed changes

src/engraving/dom/page.cpp Outdated Show resolved Hide resolved

cbjeukendrup reviewed Apr 23, 2024

View reviewed changes

Jojo-Schmitz force-pushed the broken-chord-symbols branch 6 times, most recently from 7fcc2b6 to 8b69c04 Compare April 29, 2024 10:07

Jojo-Schmitz force-pushed the broken-chord-symbols branch from 8b69c04 to a9c472e Compare June 23, 2024 08:57

Jojo-Schmitz force-pushed the broken-chord-symbols branch from 258be08 to 15bf26d Compare July 24, 2024 15:09

Jojo-Schmitz force-pushed the broken-chord-symbols branch from 15bf26d to e2af1f1 Compare July 25, 2024 13:16

Fix GH#16363: Accidentals in chord symbols rendered incorrectly when …

e5dee71

…using custom XML file

Jojo-Schmitz force-pushed the broken-chord-symbols branch from e2af1f1 to e5dee71 Compare July 25, 2024 13:19

cbjeukendrup approved these changes Jul 25, 2024

View reviewed changes

cbjeukendrup merged commit 7f55299 into musescore:master Jul 25, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Jojo-Schmitz commented Apr 23, 2024

cbjeukendrup Apr 23, 2024

Jojo-Schmitz Apr 23, 2024 •

edited

Loading

Jojo-Schmitz Apr 23, 2024

Jojo-Schmitz Apr 23, 2024

Jojo-Schmitz Apr 23, 2024 •

edited

Loading

Jojo-Schmitz commented May 21, 2024

Jojo-Schmitz commented Jun 23, 2024

Jojo-Schmitz commented Jun 23, 2024

cbjeukendrup commented Jun 23, 2024

Jojo-Schmitz commented Jun 23, 2024 •

edited

Loading

Jojo-Schmitz commented Jul 1, 2024

Jojo-Schmitz commented Jul 24, 2024

cbjeukendrup commented Jul 24, 2024

cbjeukendrup commented Jul 25, 2024 •

edited

Loading

Jojo-Schmitz commented Jul 25, 2024 •

edited

Loading

cbjeukendrup commented Jul 25, 2024

Jojo-Schmitz commented Jul 25, 2024

Jojo-Schmitz commented Jul 25, 2024

	char32_t val = code.toUInt(&ok, 0);
	char32_t val = code.toUInt(&ok, 16);

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

Jojo-Schmitz commented Apr 23, 2024

cbjeukendrup Apr 23, 2024

Choose a reason for hiding this comment

Jojo-Schmitz Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Apr 23, 2024

Choose a reason for hiding this comment

Jojo-Schmitz Apr 23, 2024

Choose a reason for hiding this comment

Jojo-Schmitz Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz commented May 21, 2024

Jojo-Schmitz commented Jun 23, 2024

Jojo-Schmitz commented Jun 23, 2024

cbjeukendrup commented Jun 23, 2024

Jojo-Schmitz commented Jun 23, 2024 • edited Loading

Jojo-Schmitz commented Jul 1, 2024

Jojo-Schmitz commented Jul 24, 2024

cbjeukendrup commented Jul 24, 2024

cbjeukendrup commented Jul 25, 2024 • edited Loading

Jojo-Schmitz commented Jul 25, 2024 • edited Loading

cbjeukendrup commented Jul 25, 2024

Jojo-Schmitz commented Jul 25, 2024

Jojo-Schmitz commented Jul 25, 2024

Jojo-Schmitz Apr 23, 2024 •

edited

Loading

Jojo-Schmitz Apr 23, 2024 •

edited

Loading

Jojo-Schmitz commented Jun 23, 2024 •

edited

Loading

cbjeukendrup commented Jul 25, 2024 •

edited

Loading

Jojo-Schmitz commented Jul 25, 2024 •

edited

Loading