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.

Backport of musescore#8833, part 2
  • Loading branch information
@lvinken @Jojo-Schmitz
lvinken authored and Jojo-Schmitz committed Aug 30, 2021
1 parent 0594fcd commit 4d5e45d
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 30 deletions.
2 changes: 1 addition & 1 deletion importexport/musicxml/importmxmlnoteduration.cpp
View file
Expand Up @@ -219,7 +219,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") {
Expand Down
10 changes: 5 additions & 5 deletions importexport/musicxml/importmxmlpass1.cpp
View file
Expand Up @@ -3263,17 +3263,17 @@ void MusicXMLParserPass1::note(const QString& partId,
}
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();
Expand Down Expand Up @@ -3388,13 +3388,13 @@ 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 importexport/musicxml/importmxmlpass2.cpp
View file
Expand Up @@ -5471,20 +5471,20 @@ Note* MusicXMLParserPass2::note(const QString& partId,
beam(beamTypes);
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
Expand Down Expand Up @@ -5861,13 +5861,13 @@ 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 @@ -6485,7 +6485,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();
Expand Down Expand Up @@ -6565,7 +6565,7 @@ void MusicXMLParserNotations::slur()
_slurStart = true;
}

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

//---------------------------------------------------------
Expand Down Expand Up @@ -6676,7 +6676,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 @@ -6696,7 +6696,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 @@ -6720,8 +6720,7 @@ 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;
Expand All @@ -6730,7 +6729,7 @@ void MusicXMLParserNotations::articulations()
}
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"
Expand All @@ -6740,7 +6739,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 @@ -6766,12 +6765,11 @@ 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");
Expand All @@ -6788,7 +6786,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();
Expand Down Expand Up @@ -6827,8 +6825,7 @@ 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(),
Expand All @@ -6838,7 +6835,6 @@ void MusicXMLParserNotations::technical()
}
else if (_e.name() == "harmonic") {
harmonic();
_e.readNext();
}
else {
skipLogCurrElem();
Expand All @@ -6863,7 +6859,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);
Expand Down Expand Up @@ -7403,7 +7399,7 @@ void MusicXMLParserNotations::parse()
if (_e.name() == "arpeggiate") {
_arpeggioType = _e.attributes().value("direction").toString();
if (_arpeggioType == "") _arpeggioType = "none";
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
}
else if (_e.name() == "articulations") {
articulations();
Expand All @@ -7419,7 +7415,7 @@ void MusicXMLParserNotations::parse()
}
else if (_e.name() == "non-arpeggiate") {
_arpeggioType = "non-arpeggiate";
_e.readNext();
_e.skipCurrentElement(); // skip but don't log
}
else if (_e.name() == "ornaments") {
ornaments();
Expand Down

0 comments on commit 4d5e45d

Please sign in to comment.