-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
GSoC 2020 Milestone 1: add style options for H-bar multimeasure rests #6161
Conversation
b70a14f
to
9bbcc66
Compare
Ah, I see why vtests fail. As I allude to in the commit message (and will discuss in my blog post next week), I figured out that mmrests were being drawn with a difference between the visual ends of the vertical and horizontal strokes and the actual internal endpoints, due to Qt's default setting for lines being "a square line end that covers the end point and extends beyond it by half the line width" (see https://doc.qt.io/qt-5.9/qt.html#PenCapStyle-enum). For the vertical strokes, this meant that they stuck out just a tiny bit too far; for the horizontal, this meant some unnecessarily complex calculations in order to compensate. I changed both to |
@IsaacWeiss : I think there could also be a change in the mmrest skyline. If you look at bravura-mmrest, mmrest-8, mmrest-9, mmrest-10, the texts are now all shifted upwards. |
I asked Tantacrul about adding a glyph for a multimeasure rest to the icon font, and he very kindly obliged. mattmcclinch@2f7dbaf contains the updated icon font and uses the new glyph for the multimeasure rest icon. You will need to rerun the install scheme or else MuseScore will load the old font with the missing glyph. It is up to you whether or not you want to remove the definition of We can probably bring some of the new multimeasure rest options over to the inspector as well, but I thought I would share this much now, anyway. |
4a47ff2
to
8f35c56
Compare
Fixed the skyline issue. |
Looking good! BTW, for ongoing projects like this, I recommend not squashing commits until after code is code and reviews are satisfied and things are ready to merge. It makes it harder to track the discussions when comments end up being on outdated diffs, and I can't see what change you made to address any particular review (eg, how you fixed the skyline). |
I started out that way (initial version of #6108 was three commits), but when I kept having to rebase, it was much worse with multiple commits. Hopefully the huge changes to master will pause now, or this PR will be merged first. |
Speaking of risks with squashing commits, I squashed the two I cherry-picked from @mattmcclinch with each other, and that made me appear to be the author. The Inspector isn't really related to these Style changes—only the number position, which was previously implemented, really makes sense on a per-element basis—so I'll back that squashed commit out, and let @mattmcclinch make his own PR restoring the mmrest Inspector. |
8f35c56
to
f7c8224
Compare
Good point about the rebasing, I guess that could be a special reason to squash more often this year in particular. |
At any rate, I think this is complete, and I hope it can be merged before it needs rebasing again. Since the old-style mmrests are something of a separate feature, I will be working on a new branch; if this PR is not in master by then, I will still try to keep them in a separate commit. |
switch (propertyId) { | ||
case Pid::MMREST_NUMBER_POS: | ||
_mmRestNumberPos = v.toDouble(); | ||
triggerLayout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about setLayoutInvalid
? Doesn't trigger
layout compute lots of things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
triggerLayout()
is definitely the right method to use. setLayoutInvalid()
only applies to elements that are subtypes of TextBase
. And triggerLayout()
does not exactly compute lots of things, but it does invoke a number of functions. It is all very fast, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More specifically, triggerLayout tells MuseScore to relay our exactly the part of the score that this element affects. So in most cases it means only a single system or page gets laid out after each change rather than the whole score.
setLayoutInvalid basically means, we need to relook at the content of the text element to reprocess all the formatting within in it.
Do you think the style options for rests should really be on a separate page? I have a PR #5602 a long time ago that added a style option for rests to the Notes page (renamed to Notes/Rests). |
As I've mentioned (but seemingly not been understood) on Telegram, I'd prefer to see the existing style dialog be gutted and instead find a way to use the new Inspector to make style settings directly. Same code, same user interface; less maintenance, less user confusion; win/win. So that would render this question moot. That said, notes & rests are different enough, especially their style properties, that to me keeping them separate makes sense if we stay with this dialog-full-of-pages approach. I think there should be other style settings for rests, like the offset applied in mutlivoice context, etc. |
To me it actually does make sense to have options for notes and rests near each other, I just somehow hadn't realized there was already a Notes page. |
f7c8224
to
753c9e0
Compare
Huh, apparently I have no idea how vtests work. |
The commit about vtests is useless. |
cfb9e1e
to
addf63a
Compare
Dropped the vtests commit and added one to make the new class [EDIT: and class Rest too while I'm at it] match the new style guidelines. This PR is now temporarily not in a state of merge-readiness, as the new commit is separate for clarity. Once squashed, this PR will again be finalized and ready for merging. |
addf63a
to
69c155d
Compare
Create new class and ElementType for mmrests (fix #306192: lyrics entry with multimeasure rest selected) Use FlatCap for drawing lines and accordingly calculate layout differently Rename local variables and add comments in Score::createMMRest() Add new Style page with all new and existing mmrest options Automatically apply custom settings to mmrests for Bravura Rename class members and put public methods first according to new guidelines
69c155d
to
d318db1
Compare
This PR is now really truly finished. |
If this PR is to be merged, it would be better if it included the version of |
Given @mattmcclinch's reasonable point about the commit history of editstyle.ui, I've concluded it's impractical to keep multiple PRs open from different branches when the changes affect the same code. |
Resolves: https://musescore.org/en/user/101731/blog/2020/06/01/gsoc-2020-day-1-it-begins
New Style page with new options:
![Screen Shot 2020-06-03 at 11 26 18 PM](https://user-images.githubusercontent.com/11616153/83712142-06ec3200-a5f3-11ea-9573-1f1cd9173a0b.png)
Font-specific settings automatically applied:
![Bravura font style](https://user-images.githubusercontent.com/11616153/83712317-7c580280-a5f3-11ea-8877-4ab5b4b8097d.gif)
This PR includes everything from #6108, with real changes actually beneficial to users now added.