Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)

[ht . lilypond . development]

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 11:22:27, dak wrote:

On 2015/08/06 11:05:56, ht wrote:
> I added the Performance::mark_smob member function
> that should handle the new SCM member variable.



That's wrong.  mark_smob is not a virtual member function of

Music_output so

there is no reason Performance::mark_smob will ever be called.


Ok, so I overlooked the significance of Music_output and used an
incorrect smob class (Score) as a model for extending Performance when
I'd been better off with using (for example) Paper_book instead.  Thanks
for the correction, I hope to have got it right this time.

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)

[dak]

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 11:05:56, ht wrote:

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.


That's wrong.  mark_smob is not a virtual member function of
Music_output so there is no reason Performance::mark_smob will ever be
called.

Music_output provides the derived_mark virtual member function for the
purpose of adding your own marking to classes derived from Music_output.

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

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)

[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);
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

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/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel