MuseScore
MuseScore copied to clipboard
Harp pedalling diagrams and UI
This pull request adds harp pedalling diagrams to MuseScore for GSoC 2022. (Builds on PR #12269)
Known issues
- [x] Opening via shortcut
- [x] Stacking of diagrams around other elements
- [ ] Placement of the popup on the score
- [x] The arrow at the top of the UI
- [ ] Probably some spacing within the UI
- [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 created the test (mtest, vtest, script test) to verify the changes I made
This PR needs a rebase too; this time all conflicts are just about the #includes.
Hi @miiizen! We have started working on MuseScore 4.1, and we would like to include this PR. It would be great if you could rebase it. I've also posted a few review comments that I had written earlier but apparently not yet published.
Hi @cbjeukendrup! That's great news thanks for letting me know. I'll have time to rebase and make these changes around the end of next week.
Had a poke at the diagram to see how things are going with it. Found a few things and thought I'd share. Sorry if this is a bit premature and you were planning on asking us to test later.
https://user-images.githubusercontent.com/21022311/235263589-30a26e9d-470c-4c01-a67d-56f4bd9343e5.mp4
@miiizen A rebase is needed again...
This PR looks great (and ready to be merged), thank you so much! The only issue that it has is the broken unit tests. If you can't fix them yourself (or don't have enough time), I suggest disabling them (I'll fix them later). Or you can try rebasing this PR one more time, maybe this will help
The utest failures are all the same:
<Image>
8: <Image>
8: - <z>8099</z>
8: + <z>8199</z>
...
I guess embedded images got moved into a higher z-level?
And a rebase is needed
Thanks for the feedback! Rebasing hasn't fixed them - afraid I won't have time to try to fix the tests til after Tuesday, feel free to have a look yourself.
Fortunately, I think the test failures are no mystery at all. The default z index of an element is defined as its ElementType casted to int
multiplied by 100. Because the HARP_DIAGRAM type got inserted, the default z-index for images was incremented (because the IMAGE
element type comes after HARP_DIAGRAM
).
The reason that the z index is written to the file in those specific files where the tests fail, is that it is "customised" in those places. And the reason that it is customised and gets changed while reading/writing the score, is BSymbol::add
(and Image
is derived from BSymbol
). What happens is that when reading the score, the z index of the inner image gets set to the z index of the outer image minus one, regardless of what's written in the file. The z index of the outer image was not customised, so will use the new default value. Therefore the z index of the inner image will be changed too.
So the fix would be to just update the contents of src/engraving/tests/parts_data/part-image.mscx
and src/engraving/tests/parts_data/part-image-parts.mscx
according to https://github.com/musescore/MuseScore/actions/runs/4952108865/jobs/8857989773?pr=12597#step:9:3323
(Of course, this behaviour of changing z index while reading is very questionable, but that's a separate problem.)