Re: Yet another 2 patches for music streams

[Han-Wen Nienhuys]

Erik Sandberg schreef:
> Hi,
>
> I've finished 2 more patches for music streams. One of them is considerably 
> larger than the other, but the two should be independent.

Some comments.

Also, in general,  I think it should be possible to split patches up 
further, eg. the tremolo stuff seems independent of the 
partcombine/lyriccombine stuff.

> +         dir_ = RIGHT;
> +       }
> +      if (dir == STOP)
> +       {

This looks a bit confusing. Maybe we can rename things to more clear?

>+  // number of beams for short tremolos
>+  int expected_beaming_;

please use expected_beam_count_ . In general, if you need these 1-line 
comments to explain what a member does, you have to rename that member.

>
> +      SCM tremolo_symbol = ly_symbol2scm ("TremoloSpanEvent");
> +      SCM start_event_scm = scm_call_2 (ly_lily_module_constant 
> ("make-span-event"), tremolo_symbol, scm_from_int (START));
> +      unsmob_music (start_event_scm)->set_spot (*origin);
> +      SCM stop_event_scm = scm_call_2 (ly_lily_module_constant 
> ("make-span-event"), tremolo_symbol, scm_from_int (STOP));
> +
> +      Music *start_event = unsmob_music (start_event_scm);
> +      Music *stop_event = unsmob_music (stop_event_scm);

I think you're leaking memory here, some unprotects are necessary.

>+      SCM start_chord = scm_call_1 (ly_lily_module_constant 
("chordify-event"), start_event_scm);
>+      SCM stop_chord = scm_call_1 (ly_lily_module_constant 
>("chordify-event"), stop_event_scm);
>+
>+      child_list_ = scm_list_3 (start_chord, body->self_scm (), 
>stop_chord);+

I think it should be possible to do without the chord_event, and simply 
insert the events themselves. Possibly you will need to make an 
Event_iterator, which is similar to Event_chord_iterator.

> +      child_list_ = scm_list_3 (start_chord, body->self_scm (), stop_chord);+  
>   }
> +
> +  Sequential_iterator::construct_children ();

> +void
> +Chord_tremolo_iterator::derived_mark () const
> {
> -  return child_iter_->try_music (m);
> +  scm_gc_mark (child_list_);

It's better style to construct the child_list in get_music_list, and not 
store it all, as it's already stored in the base class in the cursor_ 
variable.  Then you also don't need a derived_mark() member.

I notice that you've done this with the other iterators too. Can you 
change this, also in time-scaled-music-iterator and percent-repeat-iterator?


> Questions:
> - It seems that the OutputPropertySetMusic music type is unused. Should I 
> remove it?

yes.

> - I have not yet done anything about old-lyric-combine-music-iterator. Should 
> I spend time on making it work, or can we junk oldaddlyrics for the next 
> release? (there's no regression test for oldaddlyrics, btw)

let's junk it.


> - What's the Right Way to pass warnings and errors from Scheme code? I want 
> some warning messages to make use of Input objects.

 (ly:message INPUT-SMOB "format string ~a" argument .. )



> +          // Wait for a Create_context event, to catch implicitly created 
> voices before it's too late.
> +          t->events_below ()->add_listener (GET_LISTENER (check_new_context), 
> ly_symbol2scm ("CreateContext"));
> +          listening_ = true;
> +        }

why does this happen in find_voice() ?

Isn't it more natural to set the listener in construct_children?


> +#if 0
>    bool b = get_outlet ()->try_music ((Music *)m); // ugh
>    Music_iterator *it = b ? (Music_iterator *) this : 0;        // ugh
>    if (!it)

can you strip the #if 0 sections from patches?

> +      // UGH. Only swallow the output property event in the context
> +      // it was intended for. This is inelegant but not inefficient.
> +      if (context ()->context_name_symbol () == m->get_property 
> ("context-type"))
> +        {
> +          props_.push_back (m);
> +          return true;
> +        }

this doesn't work with \alias. Can you use context::is_alias() ?

s_name ()));
> +      // Send the event to a bottom context. The context-type property
> +      // will later be used to apply the event in this context

Minor nit: can you use /* */ for multiline comments, with indents like

 /* bla bla blah
    bla bla bla
 */


> +      get_music ()->set_property ("context-type",
> +                                 get_outlet ()->context_name_symbol ());

You're modifying the input; that's not allowed.

Can you write a convert-ly rule to change

  \context Foo \applyOutput XXX

->

  \applyOutput #'Foo XXXX

and change the \applyOutput to set context-type.


> +IMPLEMENT_LISTENER (Part_combine_iterator, set_busy);
> +void
> +Part_combine_iterator::set_busy (SCM se)
> +{
> +  Stream_event *e = unsmob_stream_event (se);
> +  SCM mus = e->get_property ("music");
> +  Music *m = unsmob_music (mus);
> +  assert (m);
> +
> +  if (m->is_mus_type ("note-event") || m->is_mus_type ("cluster-note-event"))
> +    busy_ = true;
> +}
> +
> +/*
> +* Processes a moment in an iterator, and returns whether any new music was 
> reported.
> +*/
> +bool
> +Part_combine_iterator::try_process (Music_iterator *i, Moment m)
> +{
> +  Dispatcher *disp = i->get_outlet ()->event_source ();
> +  
> +  disp->add_listener (GET_LISTENER (set_busy), ly_symbol2scm ("MusicEvent"));
> +  busy_ = false;
> +  
> +  i->process (m);
> +  
> +  disp->remove_listener (GET_LISTENER (set_busy), ly_symbol2scm ("MusicEvent"));
> +  return busy_;
> +}

I don't understand this. Why are you adding and removing listeners all 
the time? Why don't you signal the state of the Part_combine_iterator to 
set_busy through another boolean member?  ie.

  try_process ()
  {
    notice_busy_ = true
    i->process (m);
    notice_busy_ = false
  }

> +  if (start_moment_.main_part_.is_infinity () && start_moment_ < 0)
> +    start_moment_ = now;
> +

I don't understand. What is this for?


>  void
> +Translator_group::connect_to_context (Context *c)
> +{
> +  if (context_)
> +    programming_error ("already connected to a context");
> +  context_ = c;
> +  c->event_source ()->add_listener (GET_LISTENER (eat_event), ly_symbol2scm 
> ("MusicEvent"));
> +//   c->event_source ()->add_listener (GET_LISTENER (create_child), ly_symbol2scm 
> ("CreateContext"));

the same as for "#if 0"

> +/*
> +  if (!try_music (m))
> +    // TODO: this is for testing and will be removed
> +    m->origin ()->warning ("Music not swallowed by engraver");
> +*/
> +}

and here too.

> -     (tremolo-type ,integer? "")
> +     (tremolo-type ,integer? "speed of tremolo, e.g. 16 for c4:16")

hmm. That sucks, we should store the negative log (-4 for 16) rather 
than 16.

>  - Don't use lily module, create a new module instead.
>  - delay application of the function
>  */
> +#define LOWLEVEL_MAKE_SYNTAX(proc, args)       \
> +  scm_apply_0 (proc, args)
> +/* Syntactic Sugar. */
>  #define MAKE_SYNTAX(name, location, ...)       \
> -  scm_apply_0 (ly_lily_module_constant (name), scm_list_n (make_input 
> (location), __VA_ARGS__, SCM_UNDEFINED));
> +  LOWLEVEL_MAKE_SYNTAX (ly_lily_module_constant (name), scm_list_n (make_input 
> (location), __VA_ARGS__, SCM_UNDEFINED));
>  

I don't see the point of LOWLEVEL_MAKE_SYNTAX


> -       | ONCE music_property_def {
> -               Music *m = unsmob_music ($2);
> -               SCM e = m->get_property ("element");
> -                unsmob_music (e)->set_property ("once", SCM_BOOL_T);
> -               $$ = $2;

I'm not sure if I argee with this. We're getting a warning, but to me 
it's a real syntax error. I'm worried that people might expect \once to 
work in places where it doesn't.

> +         (ly:error _ ("Music head function must return Music object"))

Also, if this happens, you have to make sure that the error is also 
signaled to the parser, that is: the exit status of Lily should be 
nonzero, and the toplevel expression containing the faulty expression 
should not be interpreted.

You might have to make a scheme binding for the parser object to achieve 
this.

>  (define-ly-syntax-loc (repeat type num body alts)
>    (make-repeat type num body alts))
> +
> +(define-ly-syntax-loc (context-specification type id mus ops create-new?)

I believe that according to Scheme coding standards, the ? is only used 
on procedures, so this should just be create-new.

--

Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen

LilyPond Software Design
 -- Code for Music Notation
http://www.lilypond-design.com