[XML output format] Indentation of closing brackets is off

• Apr 7, 2020 - 04:35
Reported version
3.4
Type
Performance
Frequency
Once
Severity
S5 - Suggestion
Reproducibility
Always
Status
PR created
Regression
No
Workaround
No
Project

Looking at any .mscx or .musicxml files' content, i found that the formatting i'd typically expect is a bit off - very consistently for all nodes in the generated XML document.

Expected result (excerpt)

  <Chord>
    <dots>1</dots>
    <durationType>eighth</durationType>
    <Lyrics>
      <syllabic>begin</syllabic>
      <text>al</text>
    </Lyrics>
    <Note>
      <pitch>61</pitch>
      <tpc>9</tpc>
      <fret>3</fret>
      <string>2</string>
    </Note>
  </Chord>

Actual result (except)

  <Chord>
    <dots>1</dots>
    <durationType>eighth</durationType>
    <Lyrics>
      <syllabic>begin</syllabic>
      <text>al</text>
      </Lyrics>
    <Note>
      <pitch>61</pitch>
      <tpc>9</tpc>
      <fret>3</fret>
      <string>2</string>
      </Note>
    </Chord>

This makes the generated XML:

  1. harder to parse by the human eye
  2. larger than it needs to be

As this happens consistently with both .mscx or .musicxml, i assume this must be caused by one and the same file output component in the code - by accident or on purpose.

So first of all, kindly advise if this is a bug or a feature ;)

Encountered with Version 3.4.2 running under Manjaro Linux.


Comments

Amd just got discussed in the developers' chat, along with a possible solition, copied from there:

It's trivial to change.

In libmscore/xmlwriter.cpp, change the body of XmlWriter::etag() from this:

      putLevel();
      *this << "</" << stack.takeLast() << '>' << endl;

to this:

      auto const last { stack.takeLast() };
      putLevel();
      *this << "</" << last << '>' << endl;
Severity S4 - Minor S5 - Suggestion

So not a bug. Even if musicxml uses it differently, that just means they prefer a different style not that they mandate it.

In reply to by Jojo-Schmitz

Severity S5 - Suggestion S4 - Minor

Thanks for your reply, JoJo!

Coming from everything but a C++ background, i was about to try the simplest thing i assumed to work:

      QString tagToClose = stack.takeLast();
      putLevel();
      *this << "</" << tagToClose << '>' << endl;

But your approach seems more knowledgeable - will use your version with auto const last { stack.takeLast() }; (which frankly is over the top of my head) and submit that!

Severity S4 - Minor S5 - Suggestion

Not my code, but taken from the developers' chat on Telegram.
You would need to adjust all score files under mtest too, a huge task.

Status PR created active

And it surely will create a bunch of merge conflicts with pending PRs, if those also have mtest scores added or changed

In reply to by Philzen

> But your approach seems more knowledgeable - will use your version with auto const last { stack.takeLast() }; (which frankly is over the top of my head) and submit that!

That code snippet is from me, and it's functionally identical to yours. (The difference is purely stylistic.)

Maybe the mtests could get modified so that the diff command it uses ignores whitespace differences? Like adding the -b option?
It is in mtest/testutils.cpp, line 164