Staff text offset applied to wrong above/below position
This is kind of hard to describe, and although it relates to the placement setting (above/below), I don't think it's necessarily related to the other issues with placement settings getting lost - here, the placement is preserved, but the offset is lost. Or more particularly, the wrong offset is applied. But it is probably related to #279033: Expression text badly placed by default, placement not saved.
First, here is where the issue was originally reported: https://musescore.org/en/node/280603. And here is my post where I list specific steps to reproduce the issue as originally discussed: https://musescore.org/en/node/280603#comment-882710
However, here is a much simpler test case:
1) new empty score
2) enter staff text on the rest in measure 1
3) use Inspector to set placement to below, then set as style
4) enter a vertical offset of, say, 6sp in Inspector, then set as style
Now, enter another staff text in another measure. Right away, you see the 6sp is ignored, and the staff text is placed directly below the staff. Not sure what's going on there, I guess we're taking the offset from the position above rather than position below?
Next, press "X" twice to flip it above then below the staff. Now it has the proper offset and sits 6sp below the staff like the first staff text.
Now, save/close/reload.
Result: both texts are now sitting right below the staff again. So whatever confusion happened when creating that second staff text is also happening on read, which is far worse because it's loss of work. It is fixed if you do a reset, but then lost again on next reload, etc.
Comments
Similar issues for other elements, just saw this with pedal markings and other lines, for instance. Worse in that the "X" command doesn't even mark the placement property as unstyled - well, I think it does for the individual line segment but not for the line itself.
As for the offset issue, we have a function getPropertyStyle() that returns the correct Sid (either position above or below) for Pid::OFFSET), but we don't use this anywhere but the Inspector, it seems. I suspect if we used this in layout and in read/writing of properties, we'd see better results.
This is getting a bit too architectural for me to feel comfortable with, I do hope someone else takes over from here. I am preparing a PR with some other changes involving the management of PLACEMENT especially on 2.3.2 import and with expression text, maybe we wait on this until that PR is in, as the code may cross.
FWIW, I submitted a PR (https://github.com/musescore/MuseScore/pull/4551) that deals with the problems in the Placement property itself (so, the "X" command now does mark a line as unstyled). But that PR does not address the offset issue at all. Depending on how we choose to address this issue, I think my existing PR would probably not needed to be modified.
In theory, the fix for the offset issue could be to actually change the associate between Sid and Pid when updating the Placement style setting. That is, if the "Staff Text" style itself gets changed so that Placement switches from Above to Below, also change it so Sid::staffTextPosBelow gets associated with Pid::OFFSET. Maybe that's not really feasible, but that's kind of what needs to happen in effect if not literally. But it seems it would be a drag to have to do this for each element type, which is why I kind of think a more "architecural" solution may be in order.
I decided to take a look at this last night and was able to reproduce, and now this morning, I can't. I have a strong feeling that it was fixed as part of https://github.com/musescore/MuseScore/pull/4827, as this definitely hit some of the same code that would be required. Very possibly this commit in particular: https://github.com/musescore/MuseScore/pull/4827/commits/bddbad2618a7fa…. The code I added to in Segment::add() was intended to make sure we got the right scaled offset for elements on small staves, but I think it also had the beneficial side effect of making sure we got the right offset according to placement. I think probably it worked for elements other than text even before that change because they generally grabbed the offset during layout, but text elements specifically don't; that code is commented out of TextBase::layout() with a TODO.
More testing required, but I think there is a very good chance this is fixed.
I can't reproduce any more problem, and I do understand why that commit would have this effect. So, fixed it is!
Automatically closed -- issue fixed for 2 weeks with no activity.