[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
MuseScore

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

• Apr 7, 2020 - 05:41

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;

Reply

• Apr 7, 2020 - 05:54
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.

Reply

• Apr 7, 2020 - 05:57
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!

Reply

• Apr 7, 2020 - 06:02
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.

Reply

• Apr 7, 2020 - 06:10
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

Reply

• Apr 7, 2020 - 06:23

> 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.)

Reply

• Apr 7, 2020 - 07:27

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

Reply