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

Add support for MEI export and import of MuseScore IDs #22978

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lpugin
Copy link
Contributor

@lpugin lpugin commented May 26, 2024

Adds an option so enable the use of MuseScore EIDs in the MEI export.

image

The option is not selected by default at this stage.

When selected, MEI elements take the MuseScore EID as @xml:id whenever appropriate, always prefixed with mscore-

<note xml:id="mscore-1786706395156" dur="8" pname="b" oct="4">

The LastEID is stored in the mei@xml:id, also prefixed with mscore-. The values are preserved in the importer. When re-exporting the file, only the mei@xml:id is expected to change in the same way is does with MuseScore files.

@rettinghaus
Copy link
Contributor

Great, thank you.

Still two problems with the current approach:

  • If someones manipulates the IDs, MuseScore will crash on trying to read the file
  • If you copy and paste passages, you get duplicated xml:ids

Try this file to see duplicated IDs:

DotsPlacement.zip

Also there needs to be an additional check on assigning them on load if they are valid.

@lpugin
Copy link
Contributor Author

lpugin commented May 26, 2024

If someones manipulates the IDs, MuseScore will crash on trying to read the file

Yes, this needs to be documented and users should be explained MuseScore IDs should not been edited or made up. I am planning to add that in the documentation.

If you copy and paste passages, you get duplicated xml:ids

Yes, this should be fixed, but it can be done separately since this is not strictly related to the MEI exporter.

@rettinghaus
Copy link
Contributor

@cbjeukendrup Anything in the way for this to be merged?

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Dec 5, 2024

Sorry, I had lost track of it. I'll look into it again tonight. At this point, it would be good to rebase it, since there have been some changes to MuseScore's id system recently.

(Update: it looks fine in principle, but really needs a rebase to fix conflicts not detected by Git, since #25666.)

@rettinghaus
Copy link
Contributor

@lpugin could you please do a rebase and adjust this to the changes in EID?

@lpugin lpugin force-pushed the develop-mei-mscore-ids branch from e0dc070 to a45ffbe Compare January 10, 2025 07:20
@lpugin lpugin marked this pull request as draft January 10, 2025 08:19
@lpugin
Copy link
Contributor Author

lpugin commented Jan 10, 2025

There is now one issue with string EID containing /, which is not allowed in MEI xml:ids. Any ideas?

@lpugin lpugin marked this pull request as ready for review January 10, 2025 11:20
}
String eidStr = String::fromStdString(eid.toStdString().c_str());
xmlId << "mscore-" << eidStr.replace('/', '.').replace('+', '-').toStdString();
m_mei.append_attribute("xml:id") = xmlId.str().c_str();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_mei.append_attribute.operator= makes a copy of the string being assigned, right? Otherwise, a pointer to (very) temporary memory is being stored here...

hasRootXmlId = true;
String xmlIdStr = String(xmlId.value());
if (xmlIdStr.startsWith(u"mscore-")) {
// Keep a global flag since we are going to read them only if mei@xml:id is given with LastEID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LastEID is not a thing anymore now that MuseScore uses randomly generated IDs. So perhaps this comment should be adjusted.

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.

3 participants