-
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: prepare for changes to mmrests and fix #306192 #6108
Conversation
//--------------------------------------------------------- | ||
|
||
void Score::createMMRest(Measure* m, Measure* lm, const Fraction& len) | ||
void Score::createMMRest(Measure* firstMeasure, Measure* lastMeasure, const Fraction& len) |
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.
Yes thanks for verbosity! At least as useful as comments!
libmscore/mmrest.cpp
Outdated
painter->setPen(pen); | ||
qreal x1 = pw * .5; | ||
qreal x2 = _mmWidth - x1; | ||
if (_mmRestNumberPos > 0.7 && _mmRestNumberPos < 3.3) { // hack for when number encounters horizontal line |
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.
This will cause conflict with PR #6105.
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.
Sure, if yours is merged first I will rebase.
Most probably needs PR #6103 being merged into master to pass the mtests on Travis. Not sure though what the vtest fails are about, @AntonioBL? Shouldn't this have been fixed with PR #6092? Or is that missing on master currently? |
Actually, vtest script seems to be working properly.
|
Ah! So a real issue with the code. |
I guess it's file compatibility. I should have known I couldn't leave that for the end. |
5696b73
to
0b0d306
Compare
Failures in mtests that are due to this PR, all but the guitar pro ones |
If you rebase now you'll see your mtest fails only, the guitar pro ones are fixed now |
0b0d306
to
66f71ff
Compare
Why is it erroring now? I'm not seeing what's different in the vest artifact. |
mtest failures, https://travis-ci.org/github/musescore/MuseScore/jobs/691524463#L4369-L4389 and https://travis-ci.org/github/musescore/MuseScore/jobs/691524463#L4409-L4940 I guess the vtest issue is the same as before |
24ffbd7
to
31e43cb
Compare
|
In general, any time one changes the class structure, one has to go through the source to find places where there are explicit tests against the old types and see what needs updating. Since presumably the old isRest(0 tests were retuning true for mmrests too, one solution is to just redefine it accordingly now. Make it explicitly return true for either a rest or an mmrest. Or, define a new isRestType() that returns true for either, and change all existing isRest() calls to this excerpt where it really doesn't make sense. Regarding the numeric values of the types, it sucks that we rely on specific values anywhere, and potentially there is an opportunity to figure out a way to make the test not depend on that any more. But it's also an opportunity to just move your new type to the end of the list, if that doesn't mess anything up (some things do depend on the actual order). |
@MarcSabatella @mattmcclinch Thank you both very much. I think the right approach is to explicitly check for |
You opened the can of worms when you separated |
So is it wrong that |
It has caused problems in the past when an element that was known to be a |
Interesting, thanks. If I make |
I would be hesitant to make |
I see this as an opportunity to clean some of these kinds of things up. That could totally mean changing the class hierarchy so RepeatMeasure is not in fact a subclass of Rest at all, for example. This could perhaps be coordinated with @Kartikay26 's project. |
So I half expected there might be bugs fixed or prevented as a side benefit of distinguishing the classes. I've just come across at least one concrete example where it is problematic for So far, I've gone through about half of the calls to |
31e43cb
to
3c3f135
Compare
"apt-get install failed"? What did I do now? |
Push again and hope for the best. Sometimes this kind of thing happens, and it has nothing to do with anything you did; it is just Travis being temperamental. |
3c3f135
to
1862c70
Compare
Woohoo, all checks passed! |
1862c70
to
025b799
Compare
Changed commit message to reference fixing https://musescore.org/en/node/306192. Should be ready to merge. |
025b799
to
8bdde31
Compare
Not going to try implementing the mmrest Inspector again yet. I may adjust my project schedule to try learning QML towards the end, but if not, multimeasure rests are just one among the other elements that don't have Inspectors yet. |
Add class MMRest and ElementType::MMREST (fix #306192: Lyrics entry with multimeasure rest selected) Add new Style page Rename local variables and add comments in createMMRest()
8bdde31
to
8087803
Compare
You are going to want to fetch the latest changes to the master branch and rebase. There should not be any conflicts, but that should allow the checks to pass. As for the inspector, you might want to take a look at mattmcclinch@b956c5f. There is currently no glyph for multimeasure rests in the icon font, so I have it show a question mark for now. |
@mattmcclinch, you are a superhero. And you're just another volunteer? |
Yup, just another volunteer. And I am only too happy to help. |
Superseded by the much more complete #6161. |
This PR adds no new functionality and introduces no bugs,* but simply lays the groundwork so future larger changes will be easier, in keeping with the incremental process recommended in today's GSoC conference call. The upcoming changes are described in my GSoC introductory blog post: https://musescore.org/en/user/101731/blog/2020/05/17/gsoc-2020-week-2-bc-coding-repeats-rests-and-counting-overview.
In passing, this also fixes https://musescore.org/en/node/306192.
*Of course, I cannot guarantee this, but I think it's safe.