musescore / MuseScore

MuseScore is an open source and free music notation software. For support, contribution, bug reports, visit MuseScore.org. Fork and make pull requests!
https://musescore.org
Other
12.33k stars 2.66k forks source link

Fix Cubase XML chord symbol import #24967

Closed Jojo-Schmitz closed 1 month ago

Jojo-Schmitz commented 1 month ago

Follow up to #24915

Resolves: #24865 and https://musescore.org/en/node/368990#comment-1260723

cbjeukendrup commented 1 month ago

I wonder if a more general approach is needed, such as letting readInt always call .simplified. @miiizen what do you think?

Jojo-Schmitz commented 1 month ago

You mean like this?

int String::toInt(bool* ok, int base) const
{
    ByteArray ba = simplified().toUtf8();
    return static_cast<int>(toInt_helper(ba.constChar(), ok, base));
}
cbjeukendrup commented 1 month ago

I was actually thinking of something in XmlStreamReader::readInt. Note that AsciiStringView::toInt is used here, rather than String::toInt. Making the change directly at the String/AsciiStringView level may or may not be a good idea; I don't have a strong opinion about that.

Also, It may be better to use String::trimmed instead of simplified; trimmed only removes leading and trailing whitespace, while simplified simplifies all whitespace including mid-string. But if there was any mid-string whitespace, then toInt would fail anyway, so no reason to try to simplify that mid-string whitespace first.

Jojo-Schmitz commented 1 month ago

I'd prefer to use simplified() or maybe trimmed() in just those 2 locations in importmxmlpass2.cpp.

cbjeukendrup commented 1 month ago

But I think we can't assume that those are the only places where we will ever encounter leading/trailing whitespace. The XML specification says that whitespace around numbers is allowed and should be ignored by the parser. More precisely,

Jojo-Schmitz commented 1 month ago

As per https://www.w3.org/2021/06/musicxml40/musicxml-reference/data-types/semitones/ that makes up for 5 occurences in in importmxmlpass2.cpp. Still pretty managable, to be done individually there. There are many more toInt() though, not listed there. I won't like to "disturb" those.

cbjeukendrup commented 1 month ago

But of course it does not only apply to the semitones type, but to basically all cases where a number has to be parsed from an XML tag's content. Hence my suggestion to make the change in XmlStreamReader::readInt.

Jojo-Schmitz commented 1 month ago

It'd also be readText().toInt()

Jojo-Schmitz commented 1 month ago

oops

miiizen commented 1 month ago

Yes, we should probably do this in XmlStreamReader - my only hesitation is that this code is more often used to read MuseScore files, which really shouldn't need this sanitisation. However, I doubt this will have a huge impact on performance

Jojo-Schmitz commented 1 month ago

As that uses AsciiStringView, this isn't really trivial

miiizen commented 1 month ago

I struggled to find a more general solution without incurring more overheads than necessary in the xml reader. I'd still prefer we fix this in XmlStreamReader, but I'm happy with this solution for the moment.

Jojo-Schmitz commented 1 month ago

Rebased to fix alleged but non-existent merge conflicts