Re: implementation plan for music streams

[Han-Wen Nienhuys]

Erik Sandberg wrote:
> Some known issues:
> - scm/define-event-classes.scm contains rather unsorted functions which are 

i'm missing that file.

> - The Stream_event class duplicates its 'context property with a context_ 
> member; this was originally intended to give speedups, but it is broken in 
> this version and requires some modifications to Context in order to work. 
> I'll probably remove the context_ member altogether in the next revision.

yes please do.

> /*
> Event dispatching:
> - Collect a list of listeners for each relevant class
> - Send the event to each of these listeners, in increasing priority order.
>   This is done by keeping a bubble-sorted temporary list of listener lists,
>   and iteratively send the event to the lowest-priority listener.
> - An event is never sent twice to listeners with equal priority.
> */
> IMPLEMENT_LISTENER (Dispatcher, dispatch) (Stream_event *ev)
> {
>   SCM class_symbol = ev->get_property ("class");
>   if (!scm_symbol_p (class_symbol))
>     {
>       warning (_f ("Unknown event class %s", ly_symbol2string 
> (class_symbol).c_str ()));
>       return;
>     }
>
>   SCM class_list = scm_primitive_eval (class_symbol);

ugh. WTF is this? Where does this come from, in what module should it be 
defined. Why does this do an eval() for every dispatch() call?


>   bool sent = false;
>
>   // TODO: fix this loop.
>   int num_classes = 0;
>   for (SCM cl = class_list; cl != SCM_EOL; cl = scm_cdr (cl))
>     num_classes++;


scm_ilength


>   // Collect all listener lists.
>   struct { int prio; SCM list; } lists[num_classes+1];
>   int i = 0;
>   for (SCM cl = class_list; cl != SCM_EOL; cl = scm_cdr (cl))
>     {
>       SCM list = scm_hashq_ref (listeners_, scm_car (cl), SCM_EOL);
>       if (list == SCM_EOL)
>         num_classes--;
>       else 
> 	{
>           // bubblesort.
>           int prio = scm_to_int (scm_caar (list));
>           int j;
>           for (j = i; j > 0 && lists[j-1].prio > prio; j--)
>             lists[j] = lists[j-1];
>           lists[j].prio = prio;
>           lists[j].list = list;
>           i++;
>         }
>     }
>   lists[num_classes].prio = INT_MAX;

can you use a Scheme sort routine to do this?

>   // Never send an event to two listeners with equal priority.
>   int last_priority = -1;
>   // Iteratively process all event classes, in increasing priority.
>   while (num_classes)
>     {
>       // Send the event, if we haven't already sent it to this target.
>       if (lists[0].prio != last_priority)
>         {
>           // process the listener
>           assert (lists[0].prio > last_priority);
>           last_priority = lists[0].prio;
>
>           Listener *l = unsmob_listener (scm_cdar (lists[0].list));
>           l->listen (ev);
>           sent = true;
>         }
>       // go to the next listener; bubble-sort the class list.
>       SCM next = scm_cdr (lists[0].list);
>       if (next == SCM_EOL)
>         num_classes--;
>       int prio = (next == SCM_EOL) ? INT_MAX : scm_to_int (scm_caar (next));
>       for (i = 0; prio > lists[i+1].prio; i++)
>         lists[i] = lists[i+1];
>       lists[i].prio = prio;
>       lists[i].list = next;
>     }
>
>   if (!sent)
>     warning (_f ("Junking event: %s", ly_symbol2string (class_symbol).c_str 
> ()));
> }

do I understand correctly that for every time step, we get multiple 
bubble sorts? That doesn't look very clean?

> #if 0
> /*
>  New listeners are appended to the end of the list.
>  This way, listeners will listen to an event in the order they were added.
> */

why if 0 ?


>   // We just remove the listener once.
>   bool first = true;
>
>   SCM dummy = scm_cons (SCM_EOL, list);
>   SCM e = dummy;
>   while (scm_cdr (e) != SCM_EOL)
>     if (*unsmob_listener (scm_cdadr (e)) == l && first)
>       {
>         scm_set_cdr_x (e, scm_cddr(e));
>         first = false;
>         break;
>       }
>     else
>       e = scm_cdr (e);
>   list = scm_cdr (dummy);

try to use scm_delq or similar, if not possible, devise an appropriate 
del() routine yoursefl.


> #if 0 // obsolete appandable-list code

remove or fix.


> #ifndef NDEBUG
> //  assert (SCM_EOL == scm_hashq_ref (listeners_, ly_symbol2scm 
> ("StreamEvent"), SCM_EOL));
> #endif


idem.


> LY_DEFINE (ly_make_dispatcher, "ly:make-dispatcher",

as a matter of style, this should be in dispatcher-scheme.cc



> /*
>   listener-scheme.cc -- Connect listeners to Scheme through Scm_listener
>
>   source file of the GNU LilyPond music typesetter
>
>   (c) 2005-2006 Erik Sandberg  <address@hidden>
> */
>
> #include "listener.hh"
> #include "ly-smobs.icc"
> #include "stream-event.hh"
>
> class Scm_listener

this should be in scm-listener.cc

> {
> public:
>   Scm_listener (SCM callback);
>   DECLARE_LISTENER (listener);
> protected:
>   DECLARE_SMOBS (Scm_listener,);
> private:
>   SCM callback_;
> };
>
> IMPLEMENT_LISTENER (Scm_listener, listener) (Stream_event *ev)


Please change the def of this macro so we  can have

IMPLEMENT_LISTENER (Scm_listener, listener);
Scm_listener::real_declaration (Stream_event *)

otherwise tools like TAGS get very confused.



> LY_DEFINE (ly_make_listener, "ly:make-listener",
scm-listener-scheme.cc


> // ES todo: Add stuff to lily-proto.hh: Stream_event and its subclasses, 
> Stream_creator, etc.

yes. in any case, I'm missing a patch.


> SCM
> Stream_event::internal_get_property (SCM sym) const
> {
>   SCM s = scm_sloppy_assq (sym, property_alist_);
>   if (s != SCM_BOOL_F)
>     return scm_cdr (s);
>   return SCM_EOL;
> }


you might want to consider basing these objects on Prob; see prob.cc


> LY_DEFINE (ly_make_stream_event, "ly:make-stream-event",

see above.

> /*TODO: to lily-proto*/
> class Stream_event;
>
> typedef struct {
>   void (*listen_callback) (void *, Stream_event *);
>   void (*mark_callback) (void *);
> } listener_vtable;

Listener_vtable

(Listener_function_table would be better still)

> #define SEND_EVENT_TO_CONTEXT(ctx, cl, ...)                             \
>   {                                                                     \
>     Stream_event *_e_ = new Stream_event (ctx, ly_symbol2scm (cl));     \
>     __VA_ARGS__;                                                        \
>     ctx->event_source ()->distribute (_e_);                               \
>     scm_gc_unprotect_object (_e_->self_scm ());                              \
>   }
>   
> #define EVENT_PROPERTY(prop, val)	\
>   (_e_->set_property (prop, val))

what's this? Is it ever used?  It looks fishy anyway.


In general, it seems that Dispatcher is not connected to Stream_event at 
all. Why not make

  Listener::listen (SCM)

iso.

  Listener::listen (Stream_event*)

then the code will generalize to other  classes more easily.


--

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

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