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

Fixes to various aspects of stafftype #4935

Merged
merged 2 commits into from May 1, 2019

Conversation

Projects
None yet
2 participants
@MarcSabatella
Copy link
Contributor

commented Apr 17, 2019

This started as what I thought would be a simple fix for an issue with scaling of chord symbols when making a staff small - but it turned out to open a can of worms. I saw that we were calling spatiumChanged() upon setting the "scale" property but not for "small", and that furthermore, we were doing this globally and thus not just for the staff in question or taking staff type changes into account. So I put the mostly-unused localSpatiumChanged functions to work, updating elements only for the staff and tick range affected. And along the way, needed to revamp how stafftype changes get added and removed, as neither delete nor undo were doing what they should.

Here is the list of issues fixed:

https://musescore.org/en/node/276620
https://musescore.org/en/node/287852
https://musescore.org/en/node/287839
https://musescore.org/en/node/287885
https://musescore.org/en/node/287807

I left TODO notes in the code regarding how we manage the stafftype changes on undo/redo. What I have works a lot better than what we have now, but it does mean we throw away the properties associated with the stafftype change on delete. Undo brings the stafftype change back, but with a fresh set of properties as if it were newly added. There would be ways around this if we created a copy constructor and/or assignment operator for stafftype, but as it is, there is no way to remove the stafftype from the list on delete and then still have it be valid on undo.

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

There would be ways around this if we created a copy constructor and/or assignment operator for stafftype

Shouldn't the default copy constructor and assignment operator work well for StaffType? At first glance I don't see anything that would require special handling. Moreover, they are in fact already used in StaffTypeList when inserting new staff types there.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

They should indeed, I probably gave up on that approach too soon. Looking again.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:stafftypechange branch from b16658c to b2bba97 Apr 18, 2019

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Thanks for calling me on that, the version I just pushed works as expected now.

// setStaffType add this list and returns pointer to that element within list
// we won't need the original after that
nst = staff->setStaffType(tick(), *st);
delete st;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 18, 2019

Contributor

AddressSanitizer complains that in one test the StaffType deleted here actually belongs to a staff as it was already added there in StaffTypeChange::read():

if (staff())
_staffType = staff()->setStaffType(measure()->tick(), st);
else
_staffType = new StaffType(st); // drag&drop operation

As an option to fix it, maybe StaffTypeChange::read() should not add a staff type to staff since it is handled by Measure::add() after this patch.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 18, 2019

Author Contributor

Huh, I had not considered that. Thanks, I will walk through this a bit more and hopefully push another update later.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:stafftypechange branch from b2bba97 to dee8662 Apr 18, 2019

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Sure enough it crashed after some point after read in real life too :-). Fixed now, and also made sure layout was properly updated after undo of delete.

@dmitrio95 dmitrio95 added the review label May 1, 2019

@dmitrio95 dmitrio95 merged commit 53dc888 into musescore:master May 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.