https://codereview.appspot.com/256230045/

This patch adds support for setting the MIDI sequence name for MIDI output files using the value of the "midititle" field from a relevant \header block, and falling back to using the "title" field if "midititle" has not been defined. (This should be analogous to how "pdftitle" and "title" work for customizing PDF metadata.)

The purpose of the change is to improve the previous behavior where the name of every MIDI sequence created by LilyPond used to be shown by MIDI synthesizers as "control track" (previously, this string was hard-coded as the name of every initial track of MIDI files created by LilyPond by Control_track_performer).

The patch * extends every Performance instance with a reference to a \header block associated with the performance, adds Scheme library routines for getting and setting the associated \header (modeled after corresponding routines available for the Score class), and updates the Book::process_score function to initialize the header information of Performance objects attached to a score; * adds a "name" parameter to the Performance output routines, used for updating the track name in the performance's first Audio_staff (assumed to represent the control track) before outputting MIDI; and * changes the write-performances-midis function (in scm/midi.scm) to query the MIDI sequence name for a performance from the performance's \header block (adapted from the handle-metadata function in scm/framework-ps.scm).

For testing, I used the following LilyPond code:

\version "2.19.25"

\header { title = "Top-level title" }

% Book A without a title defined in a \header block \book { \bookOutputName "A"

% A: score without a title in Book A -- will get the top-level title \score { \new Staff { c1 } \midi { } }

% A-1: score with a title defined in a nested \header block (in Book A) -- % will get the title in this \header block \score { \new Staff { c1 } \header { title = "Score in Book A" } \midi { } }

% A-2: score with a title and a separate MIDI title defined in a nested % \header block (in Book A) -- will get the MIDI title in this \header block \score { \new Staff { c1 } \header { title = "Another score in Book A" midititle = "MIDI title" } \midi { } } }

% Book B with a title defined in a \header block \book { \bookOutputName "B" \header { title = "Book B" }

% B: score without a title in Book B -- will get the title of Book B \score { \new Staff { c1 } \midi { } }

% B-1: score with a \header block (and title) of its own (in Book B) -- will % get the title in this \header block \score { \new Staff { c1 } \header { title = "Score in Book B" } \midi { } } }

% Book C: book with a title defined in a \header block with book parts \book { \bookOutputName "C" \header { title = "Book C" }

% Book part in Book C with a title of its own \bookpart { \header { title = "Part 1 in Book C" }

% C: score without a title (in Part 1 of Book C) -- will get the title of
%    its enclosing book part
\score {
  \new Staff { c1 }
  \midi { }
}

% C-1: score with a \header (and a title) of its own (in Part 1 of Book C)
%      -- will get this title
\score {
  \new Staff { c1 }
  \header { title = "Score in Part 1 of Book C" }
  \midi { }
}

}

% C-2: score without a title outside any book part of Book C -- will get the % title of Book C \score { \new Staff { c1 } \midi { } }

% Book part in Book C with no \header block \bookpart {

% C-3: score without a \header block (in Part 2 of Book C) -- will get the
% title of Book C
\score {
  \new Staff { c1 }
  \midi { }
}

% C-4: score with an explicit title (in Part 2 of Book C) -- will get this
% title
\score {
  \new Staff { c1 }
  \header { title = "Score in Part 2 of Book C" }
  \midi { }
}

} }

edit in lily/performance-scheme.cc uses incorrect texinfo formatting and so breaks make (see reitveld).

Fixed texinfo formatting in patch set 2.

Passes make, make check and a full make doc.

Patch set 3 addresses some of the review comments.

And patch set 4 fixes an error in marking objects during garbage collection.

Passes make, make check and a full make doc.

Oh one thing to add, I am seeing a lot of this in the reg test:

--snip--

input/regression/midi/staff-map-instrument.midi

@@ -1,4 +1,4 @@ -track 0 ev (0, (255, 3, 'control track')) +track 0 ev (0, (255, 3, '')) ev (0, (255, 1, 'creator: ')) ev (0, (255, 88, '\x04\x02\x12\x08')) ev (0, (255, 81, '\x0fB@'))

--snip--

I assume that is expected?

On 2015/08/07 11:15:46, ht wrote:

On 2015/08/07 10:49:56, J_lowe wrote:

Oh one thing to add, I am seeing a lot of this in the reg test:

--snip--

input/regression/midi/staff-map-instrument.midi

@@ -1,4 +1,4 @@ -track 0 ev (0, (255, 3, 'control track')) +track 0 ev (0, (255, 3, '')) ev (0, (255, 1, 'creator: ')) ev (0, (255, 88, '\x04\x02\x12\x08')) ev (0, (255, 81, '\x0fB@'))

--snip--

I assume that is expected?

Yes, such differences are exactly what's expected as a result of this change: in place of the string "control track", there MIDI event should contain a string from the title field of a \header block in the input file (or an empty string if there's no \header block which could be associated with a \midi { } block in a LilyPond input file).

I guess I need to still update the patch with the changes to the regression test results – I'll try to do this today.

Hmm... Reading through the "Regression tests" section of the Contributor's guide, I'm starting to get the impression that the expected results of regression tests are not actually part of the source tree, so they can't be changed simply in a patchset. Is this correct?

What I could do, however, is to add the input file (in my first comment to this Rietveld issue) as a new regression test to the codebase.

Correct. There's no problem and no need to do anything provided the change is expected.

Why would you default to an empty string rather than "control track" here?

Why would you default to an empty string rather than "control track" here?

Since the purpose of this change is to give a name for the MIDI sequence using the information from a \header block, I just thought that the name "control track" would be a very odd default name for a MIDI sequence if no suitable \header block from which to query the name is found. If there's no name given for a MIDI sequence in the input file, I wouldn't like the application to invent one for me (especially a "technical" name such as "control track"). Would some other default than the empty string (such as "unnamed") work better?

I guess that the point of the request for this enhancement is that the string stored as the name of the first track in the MIDI file may get displayed in some form to the user by various MIDI synthesizers (and it wasn't possible to change the name from within the input file) – for example, timidity 2.13.2 outputs the following information when it starts to play a MIDI file created using LilyPond 2.18.2:

... Playing A.midi MIDI file: A.midi Format: 1 Tracks: 2 Divisions: 384 Sequence: control track Text: creator: Text: GNU LilyPond 2.18.2 ...

Note the "Sequence: control track" line, which displays the name of the MIDI sequence. (The "Text:" lines come from additional text events that get always stored into the first track of the file by the Control_track_performer.)

Another example that I've seen is a digital MIDI piano which, when given a disk with LilyPond-generated MIDI files in it, will display (in a small LCD panel) the contents of the disk for selection not as file names, but as the sequence names stored in the MIDI files on the disk. The result was a list of MIDI sequences all called "control track", making the selection of the file quite difficult...

I've now added also two regression tests, and updated the Changes document.

Passes make, make check and a full make doc.

Patch on countdown for August 14th

Patch counted down - please push.

As I don't have push permissions, I need to kindly ask someone else to handle this step for me. Please find the patch attached.

pushed to staging as commit ab6b5b6312019ac4d6ea8af57b2ba5aa4295b42f Author: Heikki Tauriainen Date: Sat Aug 15 21:33:31 2015 +0200

Set the sequence name in MIDI using title information from
 \header block

issue 4539

This patch adds support for setting the MIDI sequence name for MIDI output files
using the value of the "midititle" field from a relevant \header block, and
falling back to using the "title" field if "midititle"
has not been defined.  (This should be analogous to how "pdftitle" and "title"
work for customizing PDF metadata.)

The purpose of the change is to improve the previous behavior where the name of
every MIDI sequence created by LilyPond used to be shown by MIDI synthesizers as
"control track" (previously, this string was hard-coded as the name of every
initial track of MIDI files created by LilyPond by Control_track_performer).

The patch
  * extends every Performance instance with a reference to a \header block
associated with the performance, adds Scheme library routines for getting and
setting the associated \header (modeled after corresponding routines available
for the Score class), and updates the Book::process_score function to initialize
the header information of Performance objects attached to a score;
  * adds a "name" parameter to the Performance output routines, used for
updating the track name in the performance's first Audio_staff (assumed to
represent the control track) before outputting MIDI; and
  * changes the write-performances-midis function (in scm/midi.scm) to query the
MIDI sequence name for a performance from the performance's \header block
(adapted from the handle-metadata function in scm/framework-ps.scm).
  * adds two regtests

May I ask you for adding a meaningful commit-message next time. I've found nothing more than the title in the patch. You wrote some very interesting texts during review and I read them very carefully, many thanks for the insughts!! Though its much easier to use a commit message which I only have to c/p. For now I mainly used the text from comment #1

This was the first time I pushed a patch for someone else, hope I did it correctly.

Looks fine you just need to set status to 'fixed' and I like to cut and past the commit part so that bug squad can easily find it to verify it. I am only the owner of the tracker because Heikki doesn't seem to be listed and the scripts I use to announce the patch commit requires a contact in the Owner.

Oh and get rid of the 'patch-push' label ;)

author Heikki Tauriainen
Sat, 15 Aug 2015 19:33:31 +0000 (21:33 +0200) committer Thomas Morley
Sat, 15 Aug 2015 19:37:41 +0000 (21:37 +0200) commit ab6b5b6312019ac4d6ea8af57b2ba5aa4295b42f

May I ask you for adding a meaningful commit-message next time. I've found nothing more than the title in the patch.

I apologize for the extra work that this caused, I'd forgotten to update the actual commit message with the information I'd added (manually) on the Google Code / Rietveld issue pages. Thank you for your help in pushing the patch.

(I generally don't expect the core developers to fill in missing information to my commit messages on my behalf – if this happens again with patches that I might send in the future, please just bounce them back to me until I get their format right. Thanks again, I'll try to be more careful next time.)

Really no need for any apology! Just a hint. ;) I'm still learning how to do things myself.