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.38k stars 2.68k forks source link

MusicXml parser is broken when comment follows empty node #24704

Open wizofaus opened 2 months ago

wizofaus commented 2 months ago

Issue type

Import/export issue

Description with steps to reproduce

  1. Try to open the attached XML file (below), or any valid musicxml file that has, for instance <attributes></attributes><!--xml comment--> somewhere in it.
  2. Expected: should be able to parse & import it. Actual: nothing after the <!--xml comment--> is imported.

Supporting files, videos and screenshots

XMLFile1.zip

What is the latest version of MuseScore Studio where this issue is present?

4.4

Regression

I was unable to check

Operating system

Windows 11

Additional context

This is a musicxml file exported from Encore, and it does still have problems, however the main issue is that the XML parser doesn't handle the specific case of <node></node><!--- comment -->. This is due to code in XmlStreamReader.cpp:

    if (!sibling || sibling->ToElement() || sibling->ToText()) {
        return { currentNode, XmlStreamReader::TokenType::EndElement };
    }

That needs to also check for sibling->ToComment().

Checklist

wizofaus commented 2 months ago

I'm happy to fix this, and I'd like to improve the error reporting too, as it always currently reports line number/column as 0 for most parsing errors.

lvinken commented 2 months ago

Same issue may be found when importing MusicXML files produced by Sibelius, see https://musescore.org/en/node/367552. Initially I suspected it was caused by the empty measures, but further investigation points towards the comment. Apparently introduced in MuseScore 4.4. Attached zip contains two MusicXML files, differing only in one comment line. One imports correctly, the other doesn't. Both files import OK into 4.3. xml_import_issue.zip

Jojo-Schmitz commented 2 months ago

Seems to be a Mu3 regression

Even if Mu3 imports that only with major corruptions, but Mu4 does too, with or without the changes from the PR

Edit: oh, sorry, a 4.3 regression as per @lvinken

wizofaus commented 2 months ago

Happy to have a look at those cases too.

wizofaus commented 2 months ago

Seems to be a Mu3 regression

Even if Mu3 imports that only with major corruptions, but Mu4 does too, with or without the changes from the PR

Edit: oh, sorry, a 4.3 regression as per @lvinken

Are you saying you have a music xml file that fails to import even with my fix?

The bug I fixed has been there since May 2022 anyway.

Jojo-Schmitz commented 2 months ago

No, it does Import, but with major corruptions

FrancRos31 commented 2 months ago

This issue is still in the "Done" folder of 4.4.3 project.

cbjeukendrup commented 2 months ago

I believe that's expected; the part that was supposed to go into 4.4.3 has indeed been ported to 4.4.3. The rest, being more of a 'nice to have', will go into 4.5 only, to reduce risk. @oktophonie Is that right?

wizofaus commented 2 months ago

You mean parts of my commit were cherry-picked into the 4.4.3 branch? Nothing's in master...

cbjeukendrup commented 2 months ago

That one line in xmlstreamreader was cherry-picked via https://github.com/musescore/MuseScore/pull/24819.