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 #280055: chord symbol stays high #4402

Merged
merged 1 commit into from
Dec 15, 2018

Conversation

MarcSabatella
Copy link
Contributor

Harmony layout can happen during ChordRest layout (because we need to assign the harmony shape to the chordrest for spacing purposes), but it also happens during the "normal" layout of segment elements later, which handles harmony not attached to a chordrest, and it is also when autoplace happens.

It can be dangerous to layout the harmony unless you know for sure an autoplace will follow, since the layout resets the harmony to default position. That's why just the yesterday I added a check to not layout the harmony it unnecessarily in ChordRest::shape, since there are times when this gets called without a subsequent autoplace (and this isn't the right time to do the autoplace). Yet, it's important that when we finally do the autoplace, theharmony has to have been laid out during this layout operation - otherwise a previously-autoplaced chord can get stuck up high, which is the bug here. We were sometimes skipping the layout before autoplace on the assumption it would was already done in ChordRest::shape, but with my (needed) change yesterday, that's no longer reliably true.

It's harmless to layout twice as long as you know an autoplace follows. So, the fix is to always do the layout before the autoplace, while still being careful not to layout unnecessarily during ChordRest::shape().

@dmitrio95 I wouldn't mind a look from you

@dmitrio95
Copy link
Contributor

I believe doing layout of Harmony twice should indeed do no harm as all internal layout of text blocks is done only if necessary, and position calculations inside Harmony::layout() do not seem to depend on the state of score layout. On the other hand, we still should call layout() at least once to reset position before committing an autoplace. So yes, I believe this patch should work well.

It may be useful also, in addition to that, to split horizontal and vertical layout like it is done for many other types of elements. Usually they are called layout1 and layout2 though we lack a consistent naming here. That way we could call only horizontal layout inside ChordRest (and, possibly, wherever we decide to do that for chord symbols without chords and rests) and then perform a vertical or total layout just before doing an autoplace. That should not change the results currently so should be probably postponed but it can potentially allow us to express our intentions about Harmony layout more explicitly making the results more predictable.

@anatoly-os anatoly-os merged commit f93a949 into musescore:master Dec 15, 2018
@MarcSabatella
Copy link
Contributor Author

I was thinking the same. Next time something breaks, that's going to be my strategy :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants