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

Cleaned up implementation of MMRest and MeasureRepeat #10616

Merged
merged 7 commits into from
Feb 26, 2022

Conversation

cbjeukendrup
Copy link
Contributor

Follow-up for #9886

Appearance on various kinds of staves:
Schermafbeelding 2022-02-19 om 00 35 20

Inspector:
Schermafbeelding 2022-02-19 om 01 28 32 Schermafbeelding 2022-02-19 om 01 31 41

@oktophonie
Copy link
Contributor

oktophonie commented Feb 19, 2022

Actually now I look at the tests: since the default mmRestNumberPos has now been changed in styledef.cpp all the tests/templates will need to be updated too. (@asattely can perhaps help make this much less painful as he's just had to do a similar thing in #10613 - maybe the easiest thing is for him to add this change to that PR.)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 19, 2022

I wonder whether the numbers atop these (Multi)MeasureRepeats (!) should be optional (like they are for mmrests, and non-existent for the single MeasureRepeat)? They do seem redundant to me. Also have the potential to collide with barlines (e.g. in the bottom staff of Piano).
Also the "reset to default" and "set to style" buttons seems to be missing? Or is that something MU4 doesn't provide anymore in general?

Also see #10613 (comment), so @asattely is on the templates (actually he did them already apparently)

@oktophonie
Copy link
Contributor

oktophonie commented Feb 19, 2022

Indeed, the numbers are somewhat redundant on multi-measure repeats, since all the bars actually shown.

The reset/set options are available via the three-dot menu:
image

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 19, 2022

Not quite sure why these numbers had been added in the first place, @IsaacWeiss ?

Ah, a well hidden option ;-)
I'd rather have these buddons visible directly, to easily see whether a setting is deviating from the default or not.

@oktophonie
Copy link
Contributor

I suppose if the multi-measure repeats were condensed to a single 'bar' (like mm rests) then having the number would make sense, i.e. something like
image
but whether that's ever been done in real life I must admit I don't know :)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 19, 2022

In that case they a) would make sense and b) won't collide with barlines.

@cbjeukendrup
Copy link
Contributor Author

I updated the templates and everything else, just to make the tests pass on this PR (but maybe that change should still ultimately be part of #10613). I'm not totally convinced yet that it is a good idea to store all style defaults in all score files though, but that is a separate discussion.

I agree it seems sensible to make the number for measure repeats optional. I should probably better do that in a separate PR though, and maybe it should not have much priority right now.

@Jojo-Schmitz
Copy link
Contributor

Optional and even off by default? But yes, in a separate PR

@cbjeukendrup
Copy link
Contributor Author

In the VTests, it looked like the mmrest numbers got shifted to the right a tiny bit. After some puzzling, I found a solution. Now, all changes seem expected.

@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Feb 20, 2022
@RomanPudashkin RomanPudashkin merged commit b5c39df into musescore:master Feb 26, 2022
@cbjeukendrup cbjeukendrup deleted the mmrest_mrepeat_cleanup branch February 26, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants