New implementation for writing links to file by mike-spa · Pull Request #27799 · musescore/MuseScore

mike-spa · 2025-04-23T12:04:33Z

Old implementation

As explained in the issue, our current way of writing linked elements in our files is by adding a linkedMain tag to the element in the master score

and a linked tag to the corresponding element in the part.

The problem is that this method does not establish the actual relationship, it just states that the relationship exists. It says "this is linked", but it doesn't say to what. The way we actually reconstruct the link when reading the file is, practically, by guess: we search for an element of the same type at the same corresponding Location in the master score.

This is inadequate for many reasons:

A small error in a Location tag can cause the link to get lost, resulting in parts going out of sync, file corruptions, etc. See for instance Ensure that the Ornament item inside the trill always has score and parent properly set #23681 but there have been many other examples.
It makes it impossible to even think of more complex relationships which we'll certainly need in future: orchestral cues (where we need to link specific notes in a part to a whole different stave/instrument), stave sharing (where the concept of stave and instruments themselves will likely change) etc.

New implementation

Now that we have unique EIDs, the new implementation is very trivial (see the third commit): for any element A we simply write <linkedTo>EID_OF_B</linkedTo>. On read, we just retrieve B from its EID and link it to A. That's it.

The less trivial part is that, as almost all of our unit tests consist of file comparisons, now we must make EIDs work in the unit tests too (so far, they don't get written to file in test mode). This brings an additional complication because the EIDs are randomly generated, which would make the tests non-repeatable. As discussed here, there is no "nice" solution to this.

The best solution I could find at the moment is to make EID generation sequential (as opposed to random) when in test mode, which guarantees test repeatability. But this is still not perfect. For instance, if I delete an element from a file and insert an identical one in the same place, I'd expect the end file to be identical to the starting one, but it isn't because the new element obviously has a different EID. A more drastic approach could be to exclude from the test results the diffs related to EIDs, but that has downsides too.

miiizen · 2025-04-23T15:11:58Z

src/engraving/rw/read400/tread.cpp

                LOGW("EngravingItem::readProperties: could not link %s at staff %d", item->typeName(), mainLoc.staff() + 1);
            }
        }
-    } else if (tag == "lid") {


Is this safe to remove? Were <linked> tags used in addition to this id in pre 301 scores? read400/tread is used in all older readers

Yes, as far as all I can tell this "lid" (which I assume is short for linkID) tag only ever existed in 2.x versions and was only meant as a debugging tool. For all I can tell it had no functional use whatsoever

miiizen · 2025-04-23T15:13:09Z

src/engraving/rw/read206/read206.cpp

@@ -3463,10 +3454,6 @@ Ret Read206::readScore(Score* score, XmlReader& e, ReadInOutData* out)
    }

    int id = 1;


id is now unused

miiizen · 2025-04-23T15:14:59Z

src/engraving/rw/read206/read206.cpp

@@ -1910,9 +1910,6 @@ static void convertDoubleArticulations(Chord* chord, XmlReader& e, ReadContext&
        for (Articulation* a : pairableArticulations) {
            chord->remove(a);
            if (a != newArtic) {


ctx can be UNUSED or removed as an argument.

cbjeukendrup · 2025-04-29T13:29:37Z

src/engraving/tests/barline_data/barlinedelete-ref.mscx

    <Part id="1">
-      <Staff id="1">
+      <Staff>
+        <eid>C_C</eid>


Here, we write staves with the new ID system...

cbjeukendrup · 2025-04-29T13:30:40Z

src/engraving/tests/barline_data/barlinedelete-ref.mscx

@@ -73,115 +75,145 @@
      </Part>
    <Staff id="1">


But here, still with the old system. Is that desirable?

Shouldn't make any difference, but nope thanks! Fixed

@cbjeukendrup Sorry, I was wrong here. The first <Staff> tag writes the properties of the Staff itself, so it makes sense to write its EID and linking properties here. The second tag instead writes the content of the staff, so we don't need to repeat the EID and link properties. And here the "id" tag still serves as an index, it's basically how the reader knows where it is, so I can't just remove it at the moment. It would be probably quite trivial to do since both the write and the read are sequential, so writing a progressive staff ID is unnecessary. But I'd prefer to do that in a follow up

mike-spa · 2025-04-29T13:52:49Z

@mike-spa Hi! What should be tested here?

@DmitryArefiev the most important thing to test is that when saving and reopening a file with this new PR, all the linking is preserved correctly. Also taking old files, maybe orchestral scores with parts already generated, saving them in the new PR, reopening them, checking that all the linking is correct. That kind of stuff :)

Because that's how it happens in reality. If the masterScore isn't saved EIDs may have not been yet created.

to also take into account situations with local key signatures

cbjeukendrup

Looks good to me; very elegant! The only thing is that we have to be very sure that all changes to read410 that happened in the meantime have been integrated into read460, and that all currently open PRs that touch read410 are updated to touch read460 as well.

DmitryArefiev · 2025-04-30T16:16:53Z

Tested on Win10, Mac13.7.2, LinuxUbuntu24.04.2 LTS - PASS

mercuree · 2025-04-30T18:54:53Z

Trying to reproduce the corruption for this issue #26207 (I failed, which is good), I got a Crash on opening.
I don't know if this is due to the fact that double barlines didn't work correctly in the old implementation or if there is a bug in the new implementation. In any case, I thought it was worth mentioning, otherwise it would get lost.

crash_barline_parts.mp4

DmitryArefiev · 2025-05-01T17:30:42Z

Trying to reproduce the corruption for this issue #26207 (I failed, which is good), I got a Crash on opening.
I don't know if this is due to the fact that double barlines didn't work correctly in the old implementation or if there is a bug in the new implementation. In any case, I thought it was worth mentioning, otherwise it would get lost.

@mercuree Thanks! It's a new crash (I can't reproduce it in the previous nightly build)

Logged as #27918

mike-spa requested review from a user and miiizen April 23, 2025 12:05

miiizen self-assigned this Apr 23, 2025

miiizen reviewed Apr 23, 2025

View reviewed changes

mike-spa force-pushed the newImplementationForWritingLinksToFile branch 2 times, most recently from b77bc35 to f9a2eec Compare April 28, 2025 13:57

mike-spa requested a review from cbjeukendrup April 29, 2025 12:59

mike-spa assigned miiizen and DmitryArefiev and unassigned miiizen Apr 29, 2025

ghost approved these changes Apr 29, 2025

View reviewed changes

cbjeukendrup reviewed Apr 29, 2025

View reviewed changes

mike-spa added 15 commits April 30, 2025 11:18

New Read460 module

45de879

Remove old linking implementation

7803074

Reimplent linking using EIDs

df64a45

Update all tests to 4.60

0a1b5d9

Remove leftover mmRest linked elements when removing mmRests

dfdebe4

Ensure masterScore is always saved before parts in tests

ed31f0b

Because that's how it happens in reality. If the masterScore isn't saved EIDs may have not been yet created.

Create test mode for EIDs

90406fc

Fix Engraving tests - 1

640cf09

Fix Engraving tests - 2

a6b7221

Fix Capella tests

1886dde

Fix GP tests

39e5fdb

Fix MEI tests

c78f261

Fix XML tests

c5d9ca5

Fix compiler warnings

304e35f

Don't add elements to linked staves when cloning them for mmRests

eb501cb

mike-spa added 4 commits April 30, 2025 11:18

Rebase correction

2ed6278

Improve logic for disabling keySig segments

ac7df45

to also take into account situations with local key signatures

Never clone "announce" segments to parts

05b7de4

Remove unnecessary debug log

938c95b

mike-spa force-pushed the newImplementationForWritingLinksToFile branch from e104ba7 to 7da2bf8 Compare April 30, 2025 09:36

Move readItemLink and readItemEID to separate function

2b351b5

mike-spa force-pushed the newImplementationForWritingLinksToFile branch from 7da2bf8 to 2b351b5 Compare April 30, 2025 09:52

cbjeukendrup approved these changes Apr 30, 2025

View reviewed changes

DmitryArefiev merged commit 1d0893d into musescore:master Apr 30, 2025
12 checks passed

DmitryArefiev mentioned this pull request May 1, 2025

Crash after deleting double barline, saving, closing and reopening score #27918

Closed

4 tasks

bkunda mentioned this pull request Aug 13, 2025

Crash on opening specific score containing parentheses #29215

Closed

4 tasks

mercuree mentioned this pull request Sep 5, 2025

Barlines (except repeats) make score with parts corrupted #26207

Closed

4 tasks

		@@ -3463,10 +3454,6 @@ Ret Read206::readScore(Score* score, XmlReader& e, ReadInOutData* out)
		}

		int id = 1;

Conversation

mike-spa commented Apr 23, 2025

Old implementation

New implementation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-spa commented Apr 29, 2025

Uh oh!

cbjeukendrup left a comment

Choose a reason for hiding this comment

Uh oh!

DmitryArefiev commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mercuree commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DmitryArefiev commented May 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DmitryArefiev commented Apr 30, 2025 •

edited

Loading

mercuree commented Apr 30, 2025 •

edited

Loading