Fix #16396: Tie invisibility crossing system breaks

[XiaoMigros]

Resolves: #16396

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[cbjeukendrup]

I found that there are some similar problems:

• For ties, the same applies to the "color" property too
• For slurs, it also applies to the "color" property, but seemingly not to the "visible" property
• For glissandos, it applies to the "visible" property but seemingly not to the "color" property

There may be even more cases (I haven't tested bends for example). And perhaps there are even more properties to which this applies, for example the "auto-place" property.
So we should either fix all cases separately, or look if a more general solution can be found.

[Jojo-Schmitz]

tie color and glissando visibility seem easy to fix (1-liner each), slur color not so much (haven't found a way yet)

Actually that's a 1-liner too...

tie color:

diff --git a/libmscore/tie.cpp b/libmscore/tie.cpp
index 8d6f37c4c2..66f7f3bac3 100644
--- a/libmscore/tie.cpp
+++ b/libmscore/tie.cpp
@@ -1010,6 +1010,7 @@ TieSegment* Tie::layoutBack(System* system)
       TieSegment* segment = segmentAt(1);
       segment->setSystem(system);
       segment->setVisible(visible());
+      segment->setColor(color());

       qreal x = system->firstNoteRestSegmentX(true);

glissando visibility:

diff --git a/libmscore/glissando.cpp b/libmscore/glissando.cpp
index 4be6756624..6f36e1f9c1 100644
--- a/libmscore/glissando.cpp
+++ b/libmscore/glissando.cpp
@@ -82,7 +82,7 @@ void GlissandoSegment::draw(QPainter* painter) const
       painter->save();
       qreal _spatium = spatium();

-      QPen pen(curColor(visible(), glissando()->lineColor()));
+      QPen pen(curColor(glissando()->visible(), glissando()->lineColor()));
       pen.setWidthF(glissando()->lineWidth());
       pen.setCapStyle(Qt::FlatCap);
       painter->setPen(pen);

slur color

diff --git a/libmscore/slur.cpp b/libmscore/slur.cpp
index 28302c1b01..4aa7dba0c1 100644
--- a/libmscore/slur.cpp
+++ b/libmscore/slur.cpp
@@ -30,7 +30,7 @@ namespace Ms {

 void SlurSegment::draw(QPainter* painter) const
       {
-      QPen pen(curColor());
+      QPen pen(curColor(slur()->visible(), slur()->color()));
       qreal mag = staff() ? staff()->mag(slur()->tick()) : 1.0;

       //Replace generic Qt dash patterns with improved equivalents to show true dots (keep in sync with tie.cpp)

That's for 3.x though

[Jojo-Schmitz]

That's even better, much more consistent

[Jojo-Schmitz]

But @cbjeukendrup is right: AutoPlace is affected too, albeit different: it disconects regardless whether set before apply a system braek or after, so seems a different issue altogether

[XiaoMigros]

Thanks @Jojo-Schmitz for finding the places for slurs and glissandos! I agree that autoplace is a different issue, since this PR currently only changes the visual rendering of the elements and not any actual properties themselves.

[Jojo-Schmitz]

Same issue and fix seems to apply to GuitarBend(Segment and GuitarBendHoldSegment), as far as I can tell.
Might apply to Beam too

[XiaoMigros]

Guitar bends seem fine in master (both visibility and color)

[mike-spa]

There should be a more elegant solution IMO. We have a method called propertyDelegate which is used by SpannerSegments to "delegate" a given property to their Spanner. If you look at SpannerSegment::propertyDelegate you'll see that visible and color are already delegated. Problem is that calling the visible() method returns the internal value directly and bypasses the property routing. I think this may be solved by just calling item->getProperty(Pid::VISIBLE) rather then item->visible().

[Jojo-Schmitz]

I guess it'd need to be item->getProperty(Pid::VISIBLE).toBool()

But what about the color?
Would that be item->getProperty(Pid::COLOR).toInt()? Nah, that doesn't work at all

[XiaoMigros]

My understanding would be PropertyValue::fromValue(item->getProperty(Pid::COLOR))

[mike-spa]

My understanding would be PropertyValue::fromValue(item->getProperty(Pid::COLOR))

Yep!

[cbjeukendrup]

PropertyValue::fromValue(item->getProperty(Pid::COLOR)) doesn't make a lot of sense, because getProperty already returns a PropertyValue, so that expression would be equivalent to (but less performant than) just item->getProperty(Pid::COLOR).
If you want to retrieve the property value as a Color type, then item->getProperty(Pid::COLOR).value<Color>() is probably what you want.