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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel