Regression: insert an horizontal frame leads to bad layout on undo

• Mar 16, 2017 - 01:39
Reported version
2.2
Type
Functional
Severity
S3 - Major
Status
closed
Project
MuseScore

1. Load attached file in 2.1-dev a31f66f.
2. Go to the last page.
3. Make literally any edit—move a note, delete a chord symbol, it doesn't matter what.
4. Undo.

Result: not one, but two measures on the third system jump to the next line.

Analysis: almost definitely a side effect of a recent fix for #180286: Pushing a system with an horizontal frame causes layout problem, #180991: Layout jump due to bad cautionaryWidth() calculation, or #109021: Hairpin and other symbols are shifting when relayout occurs.

Attachment Size
Eyes_On_Me.mscz 25.15 KB

Comments

• Mar 16, 2017 - 13:36

BTW, does the layout fix itself next relayout (eg, Ctrl+A)? And does this affect all horizontal frames or is there something special abut this one? I'm guessing now that we're honoring the break hint for horizontal frames, there is somewhere else where it needs to be cleared for a horizontal frame in the middle of a system row.

The bug bug I fixed was just a temporary layout glitch (fixes itself on next relayout) that only horizontal frames at the end of a system row that do *not* have line breaks on them. This is probably not a common case. Reverting my fix would be fine if it causes worse problems and no one sees an easy fix.

Assuming of course that is the relevant change.

Reply

• Mar 16, 2017 - 15:03
Title REGRESSION: Layout shift in 2.1 on undo after any operation ⇒ Regression: insert an horizontal frame leads to bad layout on undo

Steps from scratch:

1) Load this file: First Score frame.mscz
2) Select the measure 2 and insert an horizontal frame
3) Edit the score (eg change the pitch of a note)
4) Undo

Alternatively: after step #2, do: undo-redo

Result: the horizontal frame toggles at the end of the first system.

- This nigthly works as expected on March 10: c90a7e0
- Not this one on March 12: 932675a

So, I presume, indeed, that'is an unexpected side effect of this commit: https://github.com/musescore/MuseScore/pull/3052/files
For fix: #180286: Pushing a system with an horizontal frame causes layout problem

Reply

• Mar 27, 2017 - 06:49

I'm back and can look at this tomorrow. I'm guessing there will turn out to be an easy fix involving clearing the break hint. But I do agree the original bug was kind of an odd corner case not all that worrisome, so simply reverting the fix is an option.

Reply

• Mar 28, 2017 - 02:53

OK, I see what is happening*, and have an idea on how to fix it, but I cannot be sure a fix would not have some other side effect like my last fix. The good news is, in both the original bug and in this one, the glitch is temporary - if happens only while in the "undo" state. Which is to say, it happens after an undo but fixes itself on next relayout. And any new side effects that are introduced by further attempts to fiddle with this code will similarly only product temporary undo glitches that fix themselves on next relayout.

*What is happening: we are setting the "break hint" on the last measure of each system, but due to the way we represent things internally a horizontal frame is considered to end a system even when it occurs in the middle of the line of music (called a "system row" internally). That is, a line of music containing some measures, a horizontal frame, and then more measures is treated as two separate systems (which is why you get a new staffname/clef/keysig after the frame) within a single system row. So we are setting this "break hint" on the frame even though there is no particular reason to - it ends the first system but not the system row, so there is no implied break there. After the undo we see that break hint and think we are supposed to break there.

I *think* the fix is to wait to set the break hints until we are finished collecting the entire system row, rather than setting it system by system. But this change feels a little scarier to me. Need to think about it more. Or have others think through it with me. But my first attempt seems to work, fixing both the original bug and this one. Will push a PR for review later.

Reply