290694 import xml barline span #8146

wizofaus · 2021-05-24T05:57:25Z

Resolves: https://musescore.org/en/node/290694

[MU3] fix #290694 - ensure that final barline created during musicxml import (and elsewhere) spans across multi-staff parts

[ x] I signed CLA
[ x] I made sure the code in the PR follows the coding rules
[ x] I made sure the code compiles on my machine
[ x] I made sure there are no unnecessary changes in the code
[ x] I made sure the title of the PR reflects the core meaning of the issue you are solving
[ x] I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
[ x] I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

wizofaus · 2021-05-24T05:58:12Z

The "squash and merge" option doesn't seem available? I don't know how to squash commits when some have already been pushed...I thought git push -f would work but apparently not.
Also I put in [MU3] as the bug is in MU3, but this fix is for the MU4 master branch. I wouldn't expect there'd be another release of MU3 with this fix...

Jojo-Schmitz · 2021-05-24T09:36:27Z

This one here should not contain any of the commits for a prior changed, just the last commit.
The change is agains master and as such for MU4 and that only.
for MU3 a backport would beeded needed, but unlikely to get accepted

wizofaus · 2021-05-24T09:40:29Z

This one here should not contain any of the commits for a prior changed, just the last commit.
The change is agains master and as such for MU4 and that only.
for MU3 a backport would beeded needed, but unlikely to get accepted

I honestly don't know why it's showing those commits, anyway, I'll probably just create a new branch with the changes, was just trying to get the hang of this whole PR creation thing.
At any rate I'd want to write a unit test for this change in particular.

Jojo-Schmitz · 2021-05-24T09:41:53Z

src/libmscore/measure.cpp

-        bl->setTrack(track);
+        bl->setTrack(track); 
+        Part* part = score()->staff(track / VOICES)->part();
+        if (part != NULL && part->nstaves() > 1)


Code style issue (for master) braces are mandatory (the GitHub workflow would show this, it run)

Also use either part !=nullptr (NULL is C, not C++) or (better IMHO) just part.

Thanks, anyway, will have to close this one and create a new one with the right branch.

But... is the fix #xxxxxx supposed to refer to an issue on musescore.org or on github?

You can git rebase -i them away
"Fix #xxxxxx" refers to issues on musescore.org

on musescore.org

Jojo-Schmitz · 2021-05-24T10:19:53Z

It is probably because for your other PR you used the master branch. And this here is a branch off of the change master then

wizofaus · 2021-05-24T23:32:20Z

It is probably because for your other PR you used the master branch. And this here is a branch off of the change master then

Actually I realised the problem was because when I ran git push --set-upstream I forgot to specify -f. Anyway it should be OK now.

Jojo-Schmitz · 2021-05-25T07:14:34Z

Looks OK now indeed. You still want to add a unit test for this?

wizofaus · 2021-05-25T07:15:54Z

Looks OK now indeed. You still want to add a unit test for this?

Yes, I don't know what else this change might impact either.

iveshenry18 · 2021-05-25T21:26:31Z

@wizofaus, thank you for this fix! If there are no objections, I'm going to port this change for 3.6.2_backend (a unique branch I'm working on for a backend conversion job; see PR #8116 for more details on this).

wizofaus · 2021-05-25T21:29:16Z

@wizofaus, thank you for this fix! If there are no objections, I'm going to port this change for 3.6.2_backend (a unique branch I'm working on for a backend conversion job; see PR #8116 for more details on this).

It's open source, I'd think you can do what you like on your own branch! But do let me know if it causes any unexpected behaviour, I haven't done a lot of testing on it.

iveshenry18 · 2021-05-25T21:32:37Z

Sounds good! I'll put together some tests for it that you can maybe port back to this branch.

And yes, you're right that that's how open source works; just being polite!

wizofaus · 2021-05-31T20:30:41Z

BTW the only reason this is still in Draft mode is because I still haven't figured out how to build/run unit tests in MSVC 2019, indeed it seems the current unit tests for XML import at least can't even be compiled.
I'm fine with it being merged anyway but note that it's not a regression to MU3.6, though apparently it was working as expected in earlier versions.

Jojo-Schmitz · 2021-05-31T20:39:42Z

This is why I (additionally) do PRs against 3.x, esp. if, like in this case, the very same code change applies to 3.x too. There the mtest do all run.

wizofaus · 2021-05-31T23:48:51Z

This is why I (additionally) do PRs against 3.x, esp. if, like in this case, the very same code change applies to 3.x too. There the mtest do all run.

Haven't had any luck getting tests to run in the 3.6 branch either, though at least I can compile and launch them fine (but then they quite immediately as various required files don't seem to be in place)

wizofaus · 2021-06-01T01:58:31Z

Update: after several hours of frustration I was able to get the tests running under the debugger for the 3.6 branch.
But specifically the tests I want to change are the ones that import XML files (tst_mxml_io.cpp). Currently however these tests only determine that after importing an XML and exporting again, that the generated files matches a pre-saved reference XML.
This is no good because I need to check that the actual score structure is correct - specifically that the end barline for the final measure is set to span staffs, which is NOT a property that affects XML export.
I've started adding some core into tst_mxml_io.cpp to do that check, basically it will look like this:

      if (score->parts().first()->nstaves() > 1) {
          Measure* measure = score->lastMeasure();
          Segment* segment = measure->findSegment(SegmentType::EndBarLine, measure->endTick());
          BarLine* barline = (BarLine*)(segment->element(0));
          QVERIFY(barline->spanStaff());
      }

But I'm not quite sure where to put it - I specifically want it called after importing XML files with multi-staff parts, and currently mxmlIoTestRef seems to be what does that, for now I have it in "fixupScore()" which seemingly gets called after every import test.

Either way, I don't want to spend too much time on this until I can I figure out how to get tst_mxml_io.cpp building (I guess it's the tst_mxml_io project?) in MU4.

src/importexport/musicxml/tests/tst_mxml_io.cpp

Jojo-Schmitz · 2021-06-02T05:55:06Z

Did you notice that (and how) #8175 fixes this issue for the 3.6.2_backend branch? The change in measure.cpp is identical (except for an added comment), the changes for tst_mxml_io.cpp are different though and there are test files too.

wizofaus · 2021-06-02T06:07:15Z

Did you notice that (and how) #8175 fixes this issue for the 3.6.2_backend branch? The change in measure.cpp is identical (except for an added comment), the changes for tst_mxml_io.cpp are different though and there are test files too.

Hmm, yeah he said he was going to do some work for his own branch. It may be well a more "thorough" fix than mine.
I like/prefer the use of the mscx ref file, I had thought to do that myself, but I couldn't see anywhere it was actually checking system brackets in a reference mscx file.

wizofaus · 2021-06-02T06:13:28Z

Did you notice that (and how) #8175 fixes this issue for the 3.6.2_backend branch? The change in measure.cpp is identical (except for an added comment), the changes for tst_mxml_io.cpp are different though and there are test files too.

Hmm, yeah he said he was going to do some work for his own branch. It may be well a more "thorough" fix than mine.
I like/prefer the use of the mscx ref file, I had thought to do that myself, but I couldn't see anywhere it was actually checking system brackets in a reference mscx file.

void systemBrackets3() { mxmlImportTestRef("testSystemBrackets3"); } is missing from the MU4 source. Will look into restoring it.

Jojo-Schmitz · 2021-06-02T06:21:11Z

Yes, @iveshenry18 's MusixcXML fixes for the 3.6.2_backend branch have not (yet) been ported over to master

wizofaus · 2021-06-02T06:30:31Z

Yes, @iveshenry18 's MusixcXML fixes for the 3.6.2_backend branch have not (yet) been ported over to master

There's a bigger problem though - the latest changes that add the "engraving" module break this project entirely for me, still trying to figure out what's going on.

Jojo-Schmitz · 2021-06-02T06:33:36Z

A rebase should sort that, I think? Actually it seems you did that and I can't see any issue with it

wizofaus · 2021-06-02T06:39:26Z

A rebase should sort that, I think? Actually it seems you did that and I can't see any issue with it

Had already done a rebase. Anyway seems closing and reopening my local solution (which is the only way I can get the musicxml tests to run currently) did.