Crash when using palette item upon a staff-text

• Feb 5, 2020 - 00:33
Reported version
3.4
Priority
P1 - High
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
MuseScore

Here are the exact steps:

1) Fresh score: add a few quarter notes
2) Place a staff-text upon one of the notes and type "(" character
3) Afterwards, open the master palette and go to "all symbols" section for example
4) With that staff-text highlighted, press one of the symbols to add "to it" like you can with symbols generally

Note: You can't actually get this to work anyways, but the user may attempt to do so. That is, as MuseScore is currently, you can't "tack" on another symbol to a staff text, or another staff text to a staff text for that matter after adding them to a palette. Actually, there are some reasonable use-cases for doing so (custom symbols from 'outside' fonts), but that's not the point here.

5) Notice that nothing happens and no problem occurs.
6) Now, drag the "(" staff text a little bit somewhere to alter the default position, keep it highlighted afterwards, and
7) Back in the master palette, click a symbol.

Result: I'm getting consistent crashing.

Again, this is strange behavior that probably won't happen often, but the code should be strickened to not allow a meltdown over something like this.

This is in 3.4.1, but this probably will also cause a crash to occur in older versions and isn't some sort of "regression".

P.S., Another weird thing here is that if you move the "(" staff text through the inspector by changing X/Y offset before attempting to apply a symbol in the Master Palette... there's no crash, nor is there one when using the keyboard arrows. It looks to be specifically having to do with offsetting through dragging the mouse, strangely enough. Maybe that will help to solve the problem.

Comments

• Feb 5, 2020 - 13:21
Severity S4 - Minor ⇒ S2 - Critical
Priority ⇒ P1 - High

I wouldn't call this minor, it is still a crash. But not a regression indeed.

Reply

• Feb 6, 2020 - 21:16

Problem can also be reproduced by dragging a symbol from the palette to the text. When at point 5) you'll see the text element isn't accepting the symbol. However if the text is moved, suddenly the element will accept the drop.
To solve this the text element should reject the drop when it is not in "edit" mode.
I'm still looking on how to do this.
By the way, if you look closer, in case of the crash, the TextCursor is completely crazy. The row and column values are rubbish (I guess: Not initialized!).

Reply

• Feb 6, 2020 - 21:27

TextBase::acceptDrop() already tries to reject drop if not in edit mode. It detects it by the presence of ElementEditData corresponding to this element. The issue is that Element::startDrag() adds an ElementEditData instance for this element to EditData, and it does not get cleaned up until some other element starts being dragged or edited. When a symbol is being dropped/added via palette to this text element, it finds that ElementEditData is in place and accepts drop. The crash happens because TextBase::drop expects TextEditData to be there instead of plain ElementEditData, that is why cursor values are bad in this case. So the issue should probably be resolved by proper EditData cleanup between dragging the text and trying to add a symbol to it. I am not sure when exactly this cleanup is better to happen.

Reply

• Feb 7, 2020 - 06:56

It seems to me the check in TextBase is ambiguous because is checks for the existence of a corresponding ElementEditData which can be a ElementEditData (don't accepts drops) or a TextEditData (do accept drops). However, since TextEditData a subclass of ElementEditData, it is hard to tell class of an instance..
So, instead of find a proper EditData cleanup (which can be a difficult if not an impossible task) it might be better to modify Textbase::acceptDrop() such it only accepts a drop if there is a corresponding TextEditData. This can be done using RTTI (do we this already somewhere in MuseScore) or by simply adding a private boolean textEditData in ElementEditData which is true for TextEditData only and a method isTestEditData() which can be used in TextBase::acceptDrop().

Reply

• Feb 8, 2020 - 14:38

Oops, I didn't notice that you'd been working on this @njvdberg. I'm sorry for not checking that. I came to the same conclusion as you, and decided to implement a more general type() function which can be overridden on every class derived from ElementEditData.

How far through were you with your fix for this? Would you rather I didn't submit mine and you did yours? Or can I submit my one instead? Sorry for the confusion on this, again.

Reply

• Feb 9, 2020 - 06:52

@TheOtherJThistle, don't worry, there are enough open issues for the both of us :-). I'm glad there is solution for this issue and I had had my fun finding my way in that part of the code too.
I had a look at your PR and it is almost 100% the same as I had but you took it a little wider by not restricting yourself to the ElementEditData and TextEditData only which makes is a better PR.

Reply

• Feb 10, 2020 - 08:52
Status PR created ⇒ fixed

Fixed in branch master, commit 48693311d4

_fix #300635: crash when dropping symbol on text that had just been moved

This happened because the acceptDrop check wasn't checking that the
editData it had for the text element was TextEditData. So, if the text
had been moved before, then it would have ElementEditData, and
acceptDrop would incorrectly return true.

This fixes the problem by adding a type() method to ElementEditData and
its derived classes so a proper check can be carried out in acceptDrop._

Reply

Fix version
3.5.0