Skip to content

Commit

Permalink
Fix #317098: [MusicXML import] check code for white space issues
Browse files Browse the repository at this point in the history 
The MusicXML importer (incorrectly) uses QXmlStreamReader::readNext() to skip to the end of an element.
This works correctly only if the element is emtpy, which cannot always be guaranteed. The readNext() calls
are replaced by QXmlStreamReader::skipCurrentElement(), which will always work.
  • Loading branch information
@lvinken
lvinken committed Aug 11, 2021
1 parent 322c9ba commit ecd3d00
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 30 deletions.
2 changes: 1 addition & 1 deletion src/importexport/musicxml/internal/musicxml/importmxmlnoteduration.cpp
View file
Expand Up @@ -212,7 +212,7 @@ bool mxmlNoteDuration::readProperties(QXmlStreamReader& e)
//qDebug("tag %s", qPrintable(tag.toString()));
if (tag == "dot") {
_dots++;
e.readNext();
e.skipCurrentElement(); // skip but don't log
return true;
} else if (tag == "duration") {
duration(e);
Expand Down
10 changes: 5 additions & 5 deletions src/importexport/musicxml/internal/musicxml/importmxmlpass1.cpp
View file
Expand Up @@ -3221,15 +3221,15 @@ void MusicXMLParserPass1::note(const QString& partId,
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "chord") {
chord = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "cue") {
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "grace") {
grace = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "instrument") {
instrId = _e.attributes().value("id").toString();
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "lyric") {
const auto number = _e.attributes().value("number").toString();
_parts[partId].lyricNumberHandler().addNumber(number);
Expand Down Expand Up @@ -3348,12 +3348,12 @@ void MusicXMLParserPass1::notePrintSpacingNo(Fraction& dura)
while (_e.readNextStartElement()) {
if (_e.name() == "chord") {
chord = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "duration") {
duration(dura);
} else if (_e.name() == "grace") {
grace = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else {
_e.skipCurrentElement(); // skip but don't log
}
Expand Down
44 changes: 20 additions & 24 deletions src/importexport/musicxml/internal/musicxml/importmxmlpass2.cpp
View file
Expand Up @@ -4523,17 +4523,17 @@ Note* MusicXMLParserPass2::note(const QString& partId,
beam(bm);
} else if (_e.name() == "chord") {
chord = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "cue") {
cue = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "grace") {
grace = true;
graceSlash = _e.attributes().value("slash") == "yes";
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "instrument") {
instrumentId = _e.attributes().value("id").toString();
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "lyric") {
// lyrics on grace notes not (yet) supported by MuseScore
if (!grace) {
Expand Down Expand Up @@ -4887,12 +4887,12 @@ void MusicXMLParserPass2::notePrintSpacingNo(Fraction& dura)
while (_e.readNextStartElement()) {
if (_e.name() == "chord") {
chord = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "duration") {
duration(dura);
} else if (_e.name() == "grace") {
grace = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else {
_e.skipCurrentElement(); // skip but don't log
}
Expand Down Expand Up @@ -5479,7 +5479,7 @@ void MusicXMLParserLyric::parse()
} else if (_e.name() == "extend") {
hasExtend = true;
extendType = _e.attributes().value("type").toString();
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "syllabic") {
auto syll = _e.readElementText();
if (syll == "single") {
Expand Down Expand Up @@ -5552,7 +5552,7 @@ void MusicXMLParserNotations::slur()
_slurStop = true;
}

_e.readNext();
_e.skipCurrentElement(); // skip but don't log
}

//---------------------------------------------------------
Expand Down Expand Up @@ -5662,7 +5662,7 @@ void MusicXMLParserNotations::tied()
_logger->logError(QString("unknown tied type %1").arg(tiedType), &_e);
}

_e.readNext();
_e.skipCurrentElement(); // skip but don't log
}

//---------------------------------------------------------
Expand All @@ -5682,7 +5682,7 @@ void MusicXMLParserNotations::dynamics()
_dynamicsList.push_back(_e.readElementText());
} else {
_dynamicsList.push_back(_e.name().toString());
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
}
}
}
Expand All @@ -5706,15 +5706,14 @@ void MusicXMLParserNotations::articulations()
Notation artic = Notation::notationWithAttributes(_e.name().toString(),
_e.attributes(), "articulations", id);
_notations.push_back(artic);
_e.readNext();
continue;
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "breath-mark") {
_breath = SymId::breathMarkComma;
_e.readElementText();
// TODO: handle value read (note: encoding unknown, only "comma" found)
} else if (_e.name() == "caesura") {
_breath = SymId::caesura;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "doit"
|| _e.name() == "falloff"
|| _e.name() == "plop"
Expand All @@ -5723,7 +5722,7 @@ void MusicXMLParserNotations::articulations()
_e.attributes(), "articulations");
artic.setSubType(_e.name().toString());
_notations.push_back(artic);
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else {
skipLogCurrElem();
}
Expand All @@ -5748,11 +5747,10 @@ void MusicXMLParserNotations::ornaments()
Notation notation = Notation::notationWithAttributes(_e.name().toString(),
_e.attributes(), "articulations", id);
_notations.push_back(notation);
_e.readNext();
continue;
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "trill-mark") {
trillMark = true;
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "wavy-line") {
auto wavyLineTypeWasStart = (_wavyLineType == "start");
_wavyLineType = _e.attributes().value("type").toString();
Expand All @@ -5770,7 +5768,7 @@ void MusicXMLParserNotations::ornaments()
if (wavyLineTypeWasStart && _wavyLineType == "stop") {
_wavyLineType = "startstop";
}
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "tremolo") {
_tremoloType = _e.attributes().value("type").toString();
_tremoloNr = _e.readElementText().toInt();
Expand Down Expand Up @@ -5806,16 +5804,14 @@ void MusicXMLParserNotations::technical()
Notation notation = Notation::notationWithAttributes(_e.name().toString(),
_e.attributes(), "technical", id);
_notations.push_back(notation);
_e.readNext();
continue;
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "fingering" || _e.name() == "fret" || _e.name() == "pluck" || _e.name() == "string") {
Notation notation = Notation::notationWithAttributes(_e.name().toString(),
_e.attributes(), "technical");
notation.setText(_e.readElementText());
_notations.push_back(notation);
} else if (_e.name() == "harmonic") {
harmonic();
_e.readNext();
} else {
skipLogCurrElem();
}
Expand All @@ -5838,7 +5834,7 @@ void MusicXMLParserNotations::harmonic()
QString name = _e.name().toString();
if (name == "natural") {
notation.setSubType(name);
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else { // TODO: add artificial harmonic when supported by musescore
_logger->logError(QString("unsupported harmonic type/pitch '%1'").arg(name), &_e);
_e.skipCurrentElement();
Expand Down Expand Up @@ -6285,7 +6281,7 @@ void MusicXMLParserNotations::parse()
if (_arpeggioType == "") {
_arpeggioType = "none";
}
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "articulations") {
articulations();
} else if (_e.name() == "dynamics") {
Expand All @@ -6296,7 +6292,7 @@ void MusicXMLParserNotations::parse()
glissandoSlide();
} else if (_e.name() == "non-arpeggiate") {
_arpeggioType = "non-arpeggiate";
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
} else if (_e.name() == "ornaments") {
ornaments();
} else if (_e.name() == "slur") {
Expand Down

0 comments on commit ecd3d00

Please sign in to comment.