Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by address@hidden)

[ht . lilypond . development]

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh
File lily/include/performance.hh (right):

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh#newcode44
lily/include/performance.hh:44: void write_output (string filename,
const string &name) const;
On 2015/08/06 02:30:36, Dan Eble wrote:
> It's not obvious what "name" is the name of.  A more descriptive name
or a
> comment would be helpful.

Changed name of argument to "performance_name".

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc
File lily/performance-scheme.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#newcode39
lily/performance-scheme.cc:39: Performance *p = unsmob<Performance>
(performance);
On 2015/08/06 02:30:37, Dan Eble wrote:
> LY_ASSERT_SMOB returns a pointer, doesn't it?  So this unsmob is
redundant.

I just adapted this code from score-scheme.cc (which makes similar
unsmob calls), so maybe there's a need for a more general clean-up if
this coding pattern must be optimized.  Being not that experienced with
how the smob classes work internally on the C++ level, I'd rather be
safe and follow the example than take responsibility that a direct
reinterpret_cast (which is what "unchecked_unsmob" appears to do) will
continue to work here.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41
lily/performance.cc:41: header_ (SCM_EOL)
On 2015/08/06 02:30:37, Dan Eble wrote:
> I don't work with Guile frequently enough to tell you whether or not
you need to
> do something to make sure that garbage collection works properly with
this SCM
> member.  David K. could probably tell you at a glance.

Thanks for this observation, I added the Performance::mark_smob member
function that should handle the new SCM member variable.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode81
lily/performance.cc:81: if (i == 0 && !s->audio_items_. empty())
On 2015/08/06 02:30:37, Dan Eble wrote:
> no space after the dot; space before ()

Done.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92
lily/performance.cc:92: text->text_string_ = name;
On 2015/08/06 02:30:37, Dan Eble wrote:
> On 2015/08/05 03:58:25, lemzwerg wrote:
> > This code allows overwriting `control track' only once.  I guess
this is
> > intended, however, it looks limiting.  Wouldn't it be better to
identify the
> > control track by another property, say
Audio_text::CONTROL_TRACK_NAME, instead
> > of a comparison with a string that the user might modify?

> In addition to that, recognizing the control track with (i == 0) seems
fragile.
> (Please forgive me for criticizing without offering an alternative.
I'm in a
> hurry.)

The original implementation rested on my interpretation of the
guidelines concerning the structure of a format 1 MIDI file (see my
comment in
<http://lists.gnu.org/archive/html/lilypond-user/2015-08/msg00048.html>
in the thread that led to this patch), and I trusted the existing
LilyPond implementation to follow these guidelines.

I agree that while the assumptions regarding the staff structure
probably rarely break in practice (provided that no
Control_track_performers are added to or removed from any contexts in a
LilyPond input file, which I think that no normal user will do
accidentally), the code just "looks" fragile...

I've now implemented the recognition of the control track staff here in
a more explicit way by defining a new Audio_staff subclass
(Audio_control_track_staff), and changing the Control_track_performer to
create an instance of this subclass, so the staff can then be any of the
staves in a performance (although it really should always be the first
one). I also changed the tests to verify the audio element structure of
the staff into assertions since the control track is expected to follow
the structure that's defined in Control_track_performer::initialize.

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