Skip to content
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 Cubase XML chord symbol import #24967

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jojo-Schmitz
Copy link
Contributor

@cbjeukendrup
Copy link
Contributor

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

@Jojo-Schmitz
Copy link
Contributor Author

Youz 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
Copy link
Contributor

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
Copy link
Contributor Author

Jojo-Schmitz commented Sep 29, 2024

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

@cbjeukendrup
Copy link
Contributor

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
Copy link
Contributor Author

Jojo-Schmitz commented Sep 29, 2024

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.

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 29, 2024
Follow up to #637, `bass-alter` needs the same treatment as `root-alter` and some others too, as per musescore#24967 (comment)
@cbjeukendrup
Copy link
Contributor

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
Copy link
Contributor Author

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

@Jojo-Schmitz
Copy link
Contributor Author

oops

@miiizen
Copy link
Contributor

miiizen commented Sep 30, 2024

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
Copy link
Contributor Author

As that uses AsciiStringView, this isn't really trivial

@Jojo-Schmitz Jojo-Schmitz marked this pull request as draft September 30, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MusicXML import ignores chord accidentals in files created by Cubase
3 participants