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

MusicXml parser is broken when comment follows empty node #24704

Open
4 tasks done
wizofaus opened this issue Sep 13, 2024 · 10 comments · May be fixed by #24709
Open
4 tasks done

MusicXml parser is broken when comment follows empty node #24704

wizofaus opened this issue Sep 13, 2024 · 10 comments · May be fixed by #24709
Assignees
Labels
MusicXML P2 Priority: Medium

Comments

@wizofaus
Copy link
Contributor

wizofaus commented Sep 13, 2024

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

  • This report follows the guidelines for reporting bugs and issues
  • I have verified that this issue has not been logged before, by searching the issue tracker for similar issues
  • I have attached all requested files and information to this report
  • I have attempted to identify the root problem as concisely as possible, and have used minimal reproducible examples where possible
@wizofaus
Copy link
Contributor Author

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

lvinken commented Sep 14, 2024

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

Jojo-Schmitz commented Sep 14, 2024

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

Happy to have a look at those cases too.

@wizofaus
Copy link
Contributor Author

wizofaus commented Sep 19, 2024

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

No, it does Import, but with major corruptions

@oktophonie oktophonie closed this as completed by moving to Done in MuseScore Studio 4.4.3 Sep 20, 2024
@oktophonie oktophonie reopened this Sep 20, 2024
@bkunda bkunda added MusicXML P2 Priority: Medium labels Sep 20, 2024
@FrancRos31
Copy link

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

@cbjeukendrup
Copy link
Contributor

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

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

@cbjeukendrup
Copy link
Contributor

That one line in xmlstreamreader was cherry-picked via #24819.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MusicXML P2 Priority: Medium
Projects
Status: Done
Status: In Progress
Development

Successfully merging a pull request may close this issue.

7 participants