Skip to content

Commit

Permalink
fix #151121 fix #286497 fix #298638 : fix courtesy clef visbility and…
Browse files Browse the repository at this point in the history
… add tests
  • Loading branch information
AntonioBL committed Apr 22, 2020
1 parent 66c33b0 commit 1e141fd
Show file tree
Hide file tree
Showing 5 changed files with 981 additions and 31 deletions.
8 changes: 0 additions & 8 deletions libmscore/clef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,6 @@ void Clef::layout()
_clefTypes = staff()->clefType(Fraction(0,1));
}

Measure* meas = clefSeg->measure();
if (meas && meas->system() && !score()->lineMode()) {
const auto& ml = meas->system()->measures();
bool found = (std::find(ml.begin(), ml.end(), meas) != ml.end());
bool courtesy = (tick == meas->endTick() && (meas == meas->system()->lastMeasure() || !found));
if (courtesy && (!showCourtesy() || !score()->styleB(Sid::genCourtesyClef) || meas->isFinalMeasureOfSection()))
show = false;
}
// if clef not to show or not compatible with staff group
if (!show) {
setbbox(QRectF());
Expand Down
66 changes: 49 additions & 17 deletions libmscore/measure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3735,19 +3735,54 @@ qreal Measure::createEndBarLines(bool isLastMeasureInSystem)
// if end repeat, clef goes after, otherwise clef goes before
Segment* clefSeg = findSegmentR(SegmentType::Clef, ticks());
if (clefSeg) {
Segment* s1;
Segment* s2;
if (repeatEnd()) {
s1 = seg;
s2 = clefSeg;
}
else {
s1 = clefSeg;
s2 = seg;
bool wasVisible = clefSeg->visible();
int visibleInt = 0;
for (int staffIdx = 0; staffIdx < nstaves; ++staffIdx) {
int track = staffIdx * VOICES;
Clef* clef = toClef(clefSeg->element(track));
if (clef) {
bool showCourtesy = score()->genCourtesyClef() && clef->showCourtesy(); // normally show a courtesy clef
// check if the measure is the last measure of the system or the last measure before a frame
bool lastMeasure = isLastMeasureInSystem || (nm ? !(next() == nm) : true);
if (!nm || isFinalMeasureOfSection() || (lastMeasure && !showCourtesy)) {

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Jan 28, 2021

Contributor

I think this code is probably responsible for https://musescore.org/en/node/311788 - extra space between end of staff and end barline in the presence of a courtesy clef on an invisible staff. This code seems to be setting things up to mark the clef segment visible for no obvious reason.

It would seem the "obvious" fix is to simply skip invisible staves in this loop. Somehow, we end up doing the right thing in the presence of hide empty staves but not truly invisible staves. I'm guessing that's because for hide empty staves, we actually fix this up later. I'm still working on understanding that.

But I want to check to see if I am on the right track here, since as I mentioned in my comments in the original PR here, there are often unintended consequences to changes in this part of the code, and I don't want to fix one thing only to break another because I am missing something.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Jan 28, 2021

Contributor

BTW, I notice it also add the clef's shape to the skyline, which I can see if I turn on skyline display in the debug menu when view the score attached to that issue. Annoyingly, while skipping the invisible staves does fix the extra space issue, it doesn't fix the skyline, so I know there is more to this.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Jan 28, 2021

Contributor

Never mind about the sklyine. It's actually fine, it's the drawing of it in ScoreView::paint() that isn't skipping the invisible staff. So, same fix there.

As soon as I'm understanding better why hide empty staves works, I'm considering my fix good, but would still appreciate any comments.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Jan 28, 2021

Contributor

Understood, it's because wasVisible was true going in and remains the same in the comparison below, so no adjustment to measure width is made. Thus, the barline is at the right place - but the measure is a little too wide. Not something to deal with here, though.

// hide the courtesy clef in the final measure of a section, or if the measure is the final measure of a system
// and the score style or the clef style is set to "not show courtesy clef",
// or if the clef is at the end of the very last measure of the score
clef->clear();
clefSeg->createShape(staffIdx);
if (visibleInt == 0)
visibleInt = 1;
}
else {
clef->layout();
clefSeg->createShape(staffIdx);
visibleInt = 2;
}
}
}
if (s1->next() != s2) {
_segments.remove(s1);
_segments.insert(s1, s2);
if (visibleInt == 2) // there is at least one visible clef in the clef segment
clefSeg->setVisible(true);
else if (visibleInt == 1) // all (courtesy) clefs in the clef segment are not visible
clefSeg->setVisible(false);
else // should never happen
qDebug("Clef Segment without Clef elements at tick %d/%d",clefSeg->tick().numerator(),clefSeg->tick().denominator());
if ((wasVisible != clefSeg->visible()) && system()) // recompute the width only if necessary
computeMinWidth();
if (seg) {
Segment* s1;
Segment* s2;
if (repeatEnd()) {
s1 = seg;
s2 = clefSeg;
}
else {
s1 = clefSeg;
s2 = seg;
}
if (s1->next() != s2) {
_segments.remove(s1);
_segments.insert(s1, s2);
}
}
}

Expand Down Expand Up @@ -4111,11 +4146,8 @@ void Measure::addSystemTrailer(Measure* nm)
}
if (clefSegment) {
Clef* clef = toClef(clefSegment->element(track));
if (clef) {
clef->setSmall(true);
if (!nm || !score()->genCourtesyClef() || isFinalMeasure || !clef->showCourtesy())
clef->clear(); // make invisible
}
if (clef)
clef->setSmall(true);
}
}
if (s)
Expand Down
Loading

0 comments on commit 1e141fd

Please sign in to comment.