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 #311788: gap between staff and end barline with courtesy clef #7361

Merged

Conversation

MarcSabatella
Copy link
Contributor

Resolves: https://musescore.org/en/node/311788

A recent commit fixed some issues with courtesy clefs,
but introduced a regression - courtesy clefs on invisible staves
were not being ignored, causing the segment to be marked visible
and rhen in turn the measure width to be recalculated
as if the clef were visible.
This puts the barline too far to the right,
with gap between the end of the staff and the barline.
The clef itself was still ignored in the width calculation,
so the discrepancy is not large -
only the size of the margins before and after the clef.

Fix is to skip invisible staves in the code that is checking
for measures that need their width recalculated.

AIn debugging this, I became aware this clef on an invisible staff
was also appearing to affect the skyline of the visible staff,
leading me on a wild goose chase,
But that turns out to be an illusion -
the clef is not added to the skyline of the visible staff at all.
It's just that the skyline drawing code
(used when turning "Show Skylines" in the Debug menu)
is also failing to skip invisible staves.
So I have fixed that as well.

Resolves: https://musescore.org/en/node/311788

A recent commit fixed some issues with courtesy clefs,
but introduced a regression - courtesy clefs on invisible staves
were not being ignored, causing the segment to be marked visible
and rhen in turn the measure width to be recalculated
as if the clef were visible.
This puts the barline too far to the right,
with gap between the end of the staff and the barline.
The clef itself was still ignored in the width calculation,
so the discrepancy is not large -
only the size of the margins before and after the clef.

Fix is to skip invisible staves in the code that is checking
for measures that need their width recalculated.

AIn debugging this, I became aware this clef on an invisible staff
was also appearing to affect the skyline of the visible staff,
leading me on a wild goose chase,
But that turns out to be an illusion -
the clef is not added to the skyline of the visible staff at all.
It's just that the skyline drawing code
(used when turning "Show Skylines" in the Debug menu)
is *also* failing to skip invisible staves.
So I have fixed that as well.
@AntonioBL
Copy link
Contributor

AntonioBL commented Jan 29, 2021

Thank you very much Marc for the investigation.
Indeed, I had the same spacing problem when courtesy clefs were hidden, that's why I introduced the measure width recalculating step. However, I didn't take into account the fact that courtesy clef segments in invisible staves do not necessarily return visible=false.

Maybe a possible different fix is for the segment to return false when queried as segment->visible() if it contains only elements belonging to invisible staves. But maybe this would break other things in the code.

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented Jan 29, 2021

Actually, Segment::visible() does already do that, and that's why the initial assignment of "wasVisible" in the code here was "false". The problem is that the subsequent calculations are not taking staff visibility into account, so we think there is a clef to show, and thus we set visibleInt to 2. And then a couple of lines later, that results in the call to setVisible(true). Thus, the later comparison of "wasVisible != clefSeg->visible()" is true, and that's why we are calling computeMinWidth() when we shouldn't.

@AntonioBL
Copy link
Contributor

Ah, ok. So completely jumping that part of code if the staff is invisible, as you did, is probably the only way to solve it.
Thank you. 😃

@vpereverzev vpereverzev merged commit b79e923 into musescore:3.x Feb 3, 2021
@igorkorsukov
Copy link
Contributor

@MarcSabatella Could you please port the changes to the master

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.

4 participants