Skip to content

Commit

Permalink
Fix #317099: [MusicXML import] replace assertions by proper error rep…
Browse files Browse the repository at this point in the history
…orting

The MusicXML parser contains several assertions checking start and end elements to verify the parser is still in sync.
For end users they have no effect (resulting in incomplete import without any error), for a debug build they crash
MuseScore when triggered.

These assertions have been replaced by a dialog providing meaningful information (expected element name and type versus actual)
to the end user and the developer.

Other Q_ASSERTS have been replaced by `if` statements.

Backport of musescore#8833, part 1
  • Loading branch information
lvinken authored and Jojo-Schmitz committed Mar 5, 2023
1 parent 50f96eb commit 2854cae
Show file tree
Hide file tree
Showing 12 changed files with 189 additions and 216 deletions.
6 changes: 4 additions & 2 deletions importexport/musicxml/exportxml.cpp
Expand Up @@ -5668,7 +5668,8 @@ static MeasureBase* lastMeasureBase(const System* const system)
MeasureBase* mb = nullptr;
if (system) {
const auto& measures = system->measures();
Q_ASSERT(!(measures.empty()));
if (measures.empty())
return nullptr;
mb = measures.back();
}
return mb;
Expand All @@ -5683,7 +5684,8 @@ static bool hasPageBreak(const System* const system)
const MeasureBase* mb = nullptr;
if (system) {
const auto& measures = system->measures();
Q_ASSERT(!(measures.empty()));
if (measures.empty())
return false;
mb = measures.back();
}

Expand Down
51 changes: 40 additions & 11 deletions importexport/musicxml/importmxml.cpp
Expand Up @@ -73,6 +73,30 @@ Score::FileError applyMusicXMLPVGStyles(Score* score, MxmlLogger* logger)
}


//---------------------------------------------------------
// musicXMLImportErrorDialog
//---------------------------------------------------------

/**
Show a dialog displaying the MusicXML import error(s).
*/

static int musicXMLImportErrorDialog(QString text, QString detailedText)
{
QMessageBox errorDialog;
errorDialog.setIcon(QMessageBox::Question);
errorDialog.setText(text);
errorDialog.setInformativeText(QObject::tr("Do you want to try to load this file anyway?"));
errorDialog.setDetailedText(detailedText);
errorDialog.setStandardButtons(QMessageBox::Yes | QMessageBox::No);
errorDialog.setDefaultButton(QMessageBox::No);
return errorDialog.exec();
}

//---------------------------------------------------------
// importMusicXMLfromBuffer
//---------------------------------------------------------

Score::FileError importMusicXMLfromBuffer(Score* score, const QString& /*name*/, QIODevice* dev)
{
//qDebug("importMusicXMLfromBuffer(score %p, name '%s', dev %p)",
Expand All @@ -92,21 +116,26 @@ Score::FileError importMusicXMLfromBuffer(Score* score, const QString& /*name*/,
dev->seek(0);
MusicXMLParserPass1 pass1(score, &logger);
Score::FileError res = pass1.parse(dev);
if (res != Score::FileError::FILE_NO_ERROR) {
logger.logError(QString("Error during MusicXML import pass 1. Returning."));
return res;
}
const auto pass1_errors = pass1.errors();

// pass 2
dev->seek(0);
MusicXMLParserPass2 pass2(score, pass1, &logger);
Score::FileError res2 = pass2.parse(dev);
if (res2 != Score::FileError::FILE_NO_ERROR) {
logger.logError(QString("Error during MusicXML import pass 2. Returning."));
return res2;
if (res == Score::FileError::FILE_NO_ERROR) {
dev->seek(0);
res = pass2.parse(dev);
}

// report result
const auto pass2_errors = pass2.errors();
if (!(pass1_errors.isEmpty() && pass2_errors.isEmpty())) {
if (!MScore::noGui) {
const QString text { QObject::tr("Error(s) found, import may be incomplete.") };
if (musicXMLImportErrorDialog(text, pass1.errors() + pass2.errors()) != QMessageBox::Yes)
res = Score::FileError::FILE_USER_ABORT;
}
}
return Score::FileError::FILE_NO_ERROR;

return res;
}

} // namespace Ms
2 changes: 0 additions & 2 deletions importexport/musicxml/importmxmlnoteduration.cpp
Expand Up @@ -187,7 +187,6 @@ QString mxmlNoteDuration::checkTiming(const QString& type, const bool rest, cons

void mxmlNoteDuration::duration(QXmlStreamReader& e)
{
Q_ASSERT(e.isStartElement() && e.name() == "duration");
_logger->logDebugTrace("MusicXMLParserPass1::duration", &e);

_specDura.set(0, 0); // invalid unless set correctly
Expand Down Expand Up @@ -244,7 +243,6 @@ bool mxmlNoteDuration::readProperties(QXmlStreamReader& e)

void mxmlNoteDuration::timeModification(QXmlStreamReader& e)
{
Q_ASSERT(e.isStartElement() && e.name() == "time-modification");
_logger->logDebugTrace("MusicXMLParserPass1::timeModification", &e);

int intActual = 0;
Expand Down
7 changes: 0 additions & 7 deletions importexport/musicxml/importmxmlnotepitch.cpp
Expand Up @@ -29,8 +29,6 @@ namespace Ms {

static Accidental* accidental(QXmlStreamReader& e, Score* score)
{
Q_ASSERT(e.isStartElement() && e.name() == "accidental");

bool cautionary = e.attributes().value("cautionary") == "yes";
bool editorial = e.attributes().value("editorial") == "yes";
bool parentheses = e.attributes().value("parentheses") == "yes";
Expand Down Expand Up @@ -61,9 +59,6 @@ static Accidental* accidental(QXmlStreamReader& e, Score* score)

void mxmlNotePitch::displayStepOctave(QXmlStreamReader& e)
{
Q_ASSERT(e.isStartElement()
&& (e.name() == "rest" || e.name() == "unpitched"));

while (e.readNextStartElement()) {
if (e.name() == "display-step") {
const auto step = e.readElementText();
Expand Down Expand Up @@ -99,8 +94,6 @@ void mxmlNotePitch::displayStepOctave(QXmlStreamReader& e)

void mxmlNotePitch::pitch(QXmlStreamReader& e)
{
Q_ASSERT(e.isStartElement() && e.name() == "pitch");

// defaults
_step = -1;
_alter = 0;
Expand Down

0 comments on commit 2854cae

Please sign in to comment.