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

fix #19373 move all hairpins, when inserting measure into part #19405

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

sammik
Copy link
Contributor

@sammik sammik commented Sep 15, 2023

Resolves: #19373
Resolves: #19587
Resolves partialy (second issue) of #19476

undoInsertTime "acts on the linked scores as well", but only in score, there are all elements to be linked
inserting Measures always acts in masterScore and adds measures to all scores
inserting Frames acts in local score

  • 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
Copy link
Contributor Author

sammik commented Sep 15, 2023

this is somehow similar as #19271

question is, if undoInsertTime should also be always called from master score, or in other cases, it is good to call it from "local" score

@cbjeukendrup ?

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Sep 15, 2023

Yes, it would make sense to me to perform all operations relating to inserting/removing measures via the MasterScore, just like in #19271.
Same for the "remove trailing empty measures" command: #17350

@sammik
Copy link
Contributor Author

sammik commented Sep 17, 2023

Yes, it would make sense to me to perform all operations relating to inserting/removing measures via the MasterScore, just like in #19271.

It is not possible, as insertMeasureOtions have property addToAllScores, so in some situations, it is performed on just a particular excerpt score.

I can imagine something like

Score::insertMeasure(ElementType type, MeasureBase* beforeMeasure, const InsertMeasureOptions& options)
{
    if (options.addToAllScores && !score().isMaster()) {
        ...
        masterScore().insertMeasures(insertMeasure(type, masterBeforeMeasure, options);
        ...
        return ...;
    }
    ...
}

Not sure, if it is good idea.
@cbjeukendrup

@cbjeukendrup
Copy link
Contributor

Perhaps we should split the method into two methods: one for the elementary operation of adding measures to one Score instance only, and one for the score editing operation of adding measures globally.
The elementary local operation should be in the Score class; the score editing operation should be in the MasterScore class. The global operation should probably call the local operation for all scores from scoresList().
You might need to add another method to Score that is a wrapper around the global method, for example to facilitate selecting the newly inserted measures in the current score.

What do you think about that idea?

@sammik
Copy link
Contributor Author

sammik commented Sep 21, 2023

A little bit a different solution.

I found some problems with Your propose - there are bits of code, which would make sense to be called just once, in both cases - after we do some job on just local score / multiple scores

It would require to split whole method to multiple fragments.

So I did it this way:

  • splitMeasure is acting always on masterScore (in local score there is wrapper to get local inserted measure)
  • I removed option addToAllScores as it has to be always true now
  • in only single case, where addToAllScores = false (only local score) were used: creating header frame for excerpts on scores without header frame on mastrer score I changed to be without insertMeasure method

@cbjeukendrup

@cbjeukendrup
Copy link
Contributor

@sammik That seems a good solution! But now a rebase is needed, to include the changes we made to insertMeasure in that other PR...

@sammik sammik force-pushed the insert-measure-hairpins branch 2 times, most recently from c3f272f to fc0d776 Compare September 30, 2023 21:46
@sammik
Copy link
Contributor Author

sammik commented Sep 30, 2023

@cbjeukendrup done!

It seems, it needs some additional work, in case of score-parts linking PR.

@sammik sammik marked this pull request as draft October 1, 2023 00:28
cbjeukendrup
cbjeukendrup previously approved these changes Oct 1, 2023
@cbjeukendrup cbjeukendrup dismissed their stale review October 1, 2023 01:06

The PR was marked as draft

@sammik sammik force-pushed the insert-measure-hairpins branch 2 times, most recently from 26133a1 to 20d0778 Compare October 2, 2023 22:26
@sammik sammik marked this pull request as ready for review October 2, 2023 22:36
@sammik
Copy link
Contributor Author

sammik commented Oct 2, 2023

@cbjeukendrup
it should be finished now

I am sorry, but I needed to change many things, so it requires another proper review

I split original insertMeasure into

  • MasterScore::insertMeasure which is used to inserting measures, and adds new measure to all scores
  • Score::insertBox which is used to insert frames, and adds frame to single score only

@cbjeukendrup
Copy link
Contributor

Apart from these last two comments it looks good to me now!

@sammik
Copy link
Contributor Author

sammik commented Oct 8, 2023

Apart from these last two comments it looks good to me now!

@cbjeukendrup resolved and rebased.

All changes are in new commits

@sammik
Copy link
Contributor Author

sammik commented Nov 9, 2023

@oktophonie please, could You look at this?

Logically similar one (acting measure operations always in masterScore) was merged while ago, and there is also active work on other PRs regarding boxes (frames), so I think, it would be good to megre this one first.

@oktophonie
Copy link
Contributor

With this PR I can't get 'Insert [bar] before selection' to do anything at all.

@sammik
Copy link
Contributor Author

sammik commented Nov 9, 2023

With this PR I can't get 'Insert [bar] before selection' to do anything at all.

Thx, Ill check it tonight.

@sammik
Copy link
Contributor Author

sammik commented Nov 9, 2023

@oktophonie done

@cbjeukendrup cbjeukendrup merged commit b719b39 into musescore:master Nov 10, 2023
11 checks passed
@sammik sammik deleted the insert-measure-hairpins branch November 10, 2023 21:56
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