Tremolo customizations lost on second save/reload

• Sep 17, 2020 - 22:38
Reported version
3.5
Priority
P1 - High
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
MuseScore

The new facility to set placement and stroke style for tremolo unfortunately doesn't survive save & reload - at least, not two of them. Steps to reproduce:

1) create score with two quarter notes
2) add tremolo between them
3) set to stroke to attached to stems
4) save / close / reload
5) change anything else in the score
6) save / close / reload

Result: the customization is lost. What is happening is that on the read at step 4, we are setting the stroke style, so it appears to have survived the save and reload, but the property flag never got set to UNSTYLED. So the customization is lost on the write at step 6, since STYLED properties are not written.

Same for the placement.

Normally, the fix would be to replace the explicit handling of these two tags in Tremolo::read() with a single call to readStyledProperty() - see for example how it's done in Hairpin::read() or Stem::read() or any of the other uses of readStyledProperty().

However, that actually doesn't work here. That's because when it comes time for readStyledProperty() to actually set the property it just read, it calls setProperty(), and Tremolo::setProperty() refuses to set the stroke style because it is checking customStrokeStyleApplicable() to see if it's worth the trouble, and it's deciding no it isn't, because the duration actually has not yet been set.

So either Tremolo::read() needs to handle the property flags itself (just setting them to UNSTYLED should be good), or if we do switch to using readStyledProperty(), somehow we need to make sure it can actually set the property. Like, by changing customStrokeStyleApplicable() to accept a duration of V_INVALID. No idea if that breaks anything else. Or just ripping that code out - does it really hurt for setProperty() to actually allow the property to be set even not not applicable? Not sure if there was a user-visible bug that that (recent) change fixed.

Comments

• Sep 18, 2020 - 03:13

Thanks for the analysis!

No it actually doesn't hurt to not check customStrokeStyleApplicable() in setProperty(), at least not now, because in the actual layout I made sure to check it too, if it isn't applicable the default layout is forced. It's just with the new inspector logic for 4.0, it will eventually become possible for those which don't accept custom stroke styles to have redundant information, if we don't check when doing setProperty(). As far as I can think of, this kind of redundancy might cause problems for exports to other formats, for example if MusicXML supports tremolo stroke styles (I don't know if it does), those which don't support custom ones may get a tag of a custom style too. Regardless, this kind of redundancy is never good, so I think we need to do best to avoid it.

I think I prefer the second solution (switch to readStyledProperty() and allow V_INVALID).

Reply

• Sep 20, 2020 - 08:39

Well, turns out if I allow V_INVALID, staffType() can be nullptr and therefore crashes the app when executing (staffType()->group() != StaffGroup::TAB). I'll switch to the first solution to see if it works.

Reply

Fix version
3.5.1