immediately dropping break onto vertical/text frame doesn't always get applied

• Dec 11, 2018 - 21:37
Reported version
3.0
Priority
P1 - High
Type
Graphical (UI)
Frequency
Few
Severity
S4 - Minor
Reproducibility
Randomly
Status
PR created
Regression
No
Workaround
No
Project

Example reproduction step:
1. Start with initial Untitled score
2. Drag & Drop a page or system break onto the initial title vertical frame

Result: break is not applied, but should be.

Bug also occurs if immediately drop a vertical/text frame and then immediately drop a page/system break ontop that frame. But for some reason, clicking around and doing other things in between may sometimes cause this bug to not happen. Also clicking the frame and then double-clicking a break element from inspector will often result in the correct desired behavior (and then even undoing that will then result in subsequent drops actually working).

Verified on master OS: Windows 10 (10.0), Arch.: x86_64, MuseScore version (64-bit): 3.0.0., revision: 3543170


Comments

investigating. I'm noticing that in Box::drop() when droping a page break onto that initial title box, for some strange reason, the box's flag for line break is set...which causes the "if (pageBreak() || lineBreak())" block to occur and then break out & return false in that block, when what should rather happen is to evalute that if statement to be false so the code performs the subsequent operation of adding the break.

So I need to determine why that flag was set in the first place.

Ok...well that flag is set to true in VBox::VBox()...and it makes sense that vertical boxes have a line break after them. So maybe that drop code is wrong...in that it shouldn't be looking for the flag to be set, but should rather look for if there is an actual break element in the box.

looking at git blame, and I don't see any major changes to Box::drop() in the last couple years...although who knows maybe the problem started before 2 years ago but wasn't noticed.

Turns out what I mentioned in description is very indicative:

"Also clicking the frame and then double-clicking a break element from inspector will often result in the correct desired behavior (and then even undoing that will then result in subsequent drops actually working)."

Turn out the on subsequent drops, the line break flag is NOT set, which is why the rest of the code proceeds correctly. So it seems probably that initialization of the flag to true is what is causing the problem. I'm guessing it is not supposed to be initialized to true...

I guessing the only usefulness of having that flag set is to prevent users from adding unnecessary line break elements to vboxes. But even still, that code wouldn't function on undo...so it wasn't fully correct.

In the code during that time, the MeasureBase class had a _lineBreak which controlled that property (instead of using Element flags in current code), and werner had a comment "///< Forced line break"...so I'm guessing he's setting it to force a line break. But setting it seems unecessary in the current code...

Looking at the current code, the only other place where setLineBreak(true) is called is in MeasureBase::add(), inside of a "if (e->isLayoutBreak())" block. But that code is actually adding a line break element. So it seems the consistent interpretation of the line break flag being true is that that means that there is an actual line break element. Not simply an implicit line break (as naturally occurs at the end of a vbox).

Well the only problem with removing that setLineBreak is that it does indeed allow user to place line break elements on vboxes, even though they are unencessary. But then upon delete of that unenceesary line break, I can actually produce a crash:

MuseScore3.exe!abort() Line 77  C++
MuseScore3.exe!Ms::mscoreMessageHandler(QtMsgType type, const QMessageLogContext &amp; context, const QString &amp; msg) Line 3566  C++
[External Code] 
MuseScore3.exe!Ms::toMeasure(Ms::ScoreElement * e) Line 506 C++
MuseScore3.exe!Ms::Measure::mmRestLast() Line 3032  C++

> MuseScore3.exe!Ms::Score::deleteItem(Ms::Element * el) Line 1714 C++
MuseScore3.exe!Ms::Score::cmdDeleteSelection() Line 2182 C++
MuseScore3.exe!Ms::Score::cmd::__l2::() Line 3655 C++
[External Code]
MuseScore3.exe!Ms::Score::cmd(const QAction * a, Ms::EditData & ed) Line 3682 C++
MuseScore3.exe!Ms::ScoreView::cmd(const char * s) Line 2313 C++
MuseScore3.exe!Ms::ScoreView::cmd(const QAction * a) Line 1761 C++
MuseScore3.exe!Ms::MuseScore::cmd(QAction * a, const QString & cmd) Line 6086 C++
MuseScore3.exe!Ms::MuseScore::cmd(QAction * a) Line 5577 C++
MuseScore3.exe!Ms::MuseScore::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 850 C++
[External Code]
MuseScore3.exe!Ms::ScoreTab::actionTriggered(QAction * _t1) Line 226 C++
MuseScore3.exe!Ms::ScoreTab::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 110 C++
[External Code]
MuseScore3.exe!main(int argc, char * * av) Line 7455 C++
[External Code]

Note with the current master code, I can't actually produce a crash by deleting an unnecessary line break element (after performing the work around of using undo to get line break flag to be set false).

No crashes found with my PR so far...

However I should add...there is the additional question to ask: should MuseScore deliberately prohibit Line Break and Don'tBreak elements from being dropped (or added via double-clicking from palette) onto VBoxes? They don't actually do anything. So their presence is unnecessary. The current Box::acceptDrop() code does indeed return true for any LAYOUT_BREAK element (including line break elements) for any Box (which includes VBoxes), so that code would have to be changed. Anyway, although these line breaks are unnecessary, they also don't matter, so I guess their presence is not a problem.