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 - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
LilyPond Software Design
-- Code for Music Notation
http://www.lilypond-design.com
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
