Crash when deleting a measure containing a measure-repeat sign

• Jan 31, 2013 - 10:08
Type
Functional
Severity
S4 - Minor
Status
closed
Project
MuseScore

By investigating and trying to reproduce issue #19880, I found another bug.
The problem is in DELETING a measure with a repeat sign (%).
Steps:
1- Create a new score.
2- Drag and drop the measure-repeat sign (%) from the Palette(->Repeats) onto a measure.
3- Select the MEASURE containing the measure-repeat sign.
4- Delete such measure, for example by hitting canc, or ctrl+X (depending on OS), or Edit->Cut.

Result:
- crash

Windows XP SP3
Revision: a92b31d
(personal build)

Notes:
If only the repeat sign by itself is selected and deleted, there is no crash.
The crash happens if the measure containing the repeat sign is selected.

Analysis:
The problem comes from edit.cpp at lines 1269-1271 (method cmdDeleteSelection).
The segment containing the REPEAT_MEASURE element is a SegChordRest segment, so the program casts the variable "cr" as a ChordRest, while it is a REPEAT_MEASURE.
The element is deleted, but there are two problems.
The first happens at line 1127-1131 of undo.cpp, where the segment containing the (already deleted) element is appended to the list of segments to be deleted and, shortly after, deleted.
For ChordRest elements, the function which was calling (nested) undo.cpp, i.e. removeChordRest, deletes afterwards the segment (line 1540). There is then a crash because the segment has already been deleted.
The second problem is, back to cmdDeleteSelection, at line 1317, and therefore line 1320, beacuse the duration of REPEAT_MEASURE is 0. The full-measure rest needed to repair the corrupted measure is then not added.

A possible solution to the issue could be
- changing line 1127 of undo.cpp from:
if (!e->isChordRest() && e->parent() && (e->parent()->type() == Element::SEGMENT)) {
to:
if (!(e->isChordRest() || (e->type() == Element::REPEAT_MEASURE)) && e->parent() && (e->parent()->type() == Element::SEGMENT)) {
- and adding a full measure value to fraction f at line 1317 of edit.cpp (for example by taking the duration of the parent of segment s1).

BUT

The REPEAT_MEASURE would be still treated as a ChordRest (and, even if it behaves like a rest, it is not exactly a chord or a rest).
A (much) better solution would be to add a particular case for this kind of element residing in a SegChordRest segment in the cmdDeleteSelection method.

I do not feel at ease by performing the change of the code requested by this last proposal for a pull-request, I am afraid of overlooking important pieces (I am not familiar with the undo/redo routines), which could give even greater problems.

Ciao,
ABL

Comments

• Jan 31, 2013 - 10:18

I forgot to add that I am using a debug compile and the crash comes from line 253 of measure.cpp [method Measure::remove(Segment* el)], from the instruction:
Q_ASSERT(el == _segments.first())
Since the segment (corresponding to "el") had already been deleted from the measure, the first segment of the measure does not coincide with the element being deleted.

Reply

• Feb 12, 2013 - 08:47

Sorry if I did not posted the link yesterday.
I opened a pull request:
https://github.com/musescore/MuseScore/pull/215
where I tried to fix this problem and two other bugs:
- the fact that by selecting a repeat measure sign alone (not the whole measure as for this bug report) and deleting it, a corrupted empty measure is generated (however, there is no crash);
- the fact that you can add a fermata to a repeat measure sign; this results into a crash.

For the last point I tried to prevent a priori the possibility of adding a fermata onto a repeat measure sign, since from a musical point of view it would look strange.

Reply

• Feb 22, 2013 - 10:59
Status (old) fixed ⇒ active

Thanks for the fix.
There is still a little bug left:
1- Create a new score.
2- Drag and drop the measure-repeat sign (%) from the Palette(->Repeats) onto a measure.
3- Select the MEASURE containing the measure-repeat sign.
4- Delete such measure, for example by hitting canc, or ctrl+X (depending on OS), or Edit->Cut.

Result:
Since the duration() of a repeat_measure is 0, a rest replacing the repeat_measure is not set at lines 1333-1346 of edit.cpp. An empty measure (a measure with no notes or rests) is thus created.

One possible solution could be to add:
if (cr->type() == Element::REPEAT_MEASURE)
f += cr->measure()->len();
after line 1329 of edit.cpp (i.e. removeChordRest(cr, true);), before f += cr->duration(); (which adds 0). This works also for irregular measures.

Or another possibility could be to define duration() for a repeat_measure equal to the measure length. Would it work also in the case of irregular measures?

Reply