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

split / join measures in parts #19271

Merged
merged 5 commits into from Sep 25, 2023

Conversation

sammik
Copy link
Contributor

@sammik sammik commented Sep 4, 2023

Resolves: #17115
Resolves: #19283

split and join measures now always acts in masterScore (under the hood)

In joinMeasure it was deleteMeasures used before, to handle parts.
But it has side effect, that it also creates "lastDeletedKeySignature" in next measure.
This was fixed by InsertMeasureOption "moveSignaturesClef" (by omitting moveSignaturesClef = false).
But it has side effect, that if "next measure" already contains key signature, it is moved too (#19283).

Now undoRemoveMeasures function is used instead of deleteMeasures.

(Thats why utests failed - in undoRemoveMeasure, there is no check of missing initial key signature, so joining first two measures in score with no initial key sig doesn't create new one.)

Also correct repeat barlines are preserved now.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@sammik sammik marked this pull request as draft September 4, 2023 18:08
@sammik sammik marked this pull request as ready for review September 4, 2023 21:50
@sammik sammik force-pushed the split-join-measure-parts branch 2 times, most recently from a71fbf0 to c8c6a1b Compare September 6, 2023 12:23
@sammik
Copy link
Contributor Author

sammik commented Sep 8, 2023

@cbjeukendrup I have also fix to #19323
Please, should I push it here, as it is related too, or better to wait and create separate PR later?

Thank You

@sammik sammik changed the title split / join measure in parts split / join measures in parts Sep 8, 2023

deselectAll();
mScore->startCmd();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we should not call startCmd and endCmd inside this method. They are already called outside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbjeukendrup
Removing them caused utest spanners fails.
So I added them into test file.
Not sure, if it is OK (in case, if user would call joinMeasure or splitMeasure from command line)

@cbjeukendrup
Copy link
Contributor

@sammik Perhaps there is an even safer way to do this: make cmdJoinMeasures and cmdSplitMeasures only methods of the MasterScore class. That way, you can be certain that nobody will ever forget to cal mScore->... if these methods ever need to be modified. What do you think about that?

@cbjeukendrup
Copy link
Contributor

I have also fix to #19323 Please, should I push it here, as it is related too, or better to wait and create separate PR later?

A separate PR would be great!

@sammik
Copy link
Contributor Author

sammik commented Sep 10, 2023

@sammik Perhaps there is an even safer way to do this: make cmdJoinMeasures and cmdSplitMeasures only methods of the MasterScore class. That way, you can be certain that nobody will ever forget to cal mScore->... if these methods ever need to be modified. What do you think about that?

@cbjeukendrup yes, I agree, I was thinking of it too
please check now

@cbjeukendrup
Copy link
Contributor

And it looks like another rebase is needed since #19245 was merged

@sammik
Copy link
Contributor Author

sammik commented Sep 13, 2023

And it looks like another rebase is needed since #19245 was merged

Rebased and also changed joinMeasure in favour of #19245 (add new measures first, and delete old after, to prevent adding default initial key and time in some cases)

@cbjeukendrup cbjeukendrup merged commit e5c0c00 into musescore:master Sep 25, 2023
11 checks passed
@sammik sammik deleted the split-join-measure-parts branch September 26, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants