Reviewers: Valentin Villenave, carl.d.sorensen_gmail.com,
http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc File lily/part-combine-iterator.cc (right): http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode69 lily/part-combine-iterator.cc:69: Stream_event *unisono_event_; On 2010/11/26 23:05:51, Valentin Villenave wrote:
How about declaring these variables on the same line?
I don't think that's our coding style. http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode110 lily/part-combine-iterator.cc:110: void broadcast_new_state (Music_iterator *new_active, Stream_event *new_state_event); On 2010/11/26 23:05:51, Valentin Villenave wrote:
I'm not familiar with C++ coding style, but that looks like an awfully
long
line.
I can shorten the argument names, that would save a few characters... http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode133 lily/part-combine-iterator.cc:133: mmrest_event_ = 0; On 2010/11/26 23:05:51, Valentin Villenave wrote:
Since all these variables are used together a lot, another possibility
would be
to use an array?
I had thought about that, too. But there are only two places where we are looping through them (here and in derived_mark, which uses an array already). Everywhere else, just one of those is used. http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm File scm/define-music-types.scm (right): http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newcode37 scm/define-music-types.scm:37: (ApartEvent On 2010/11/26 23:05:51, Valentin Villenave wrote:
I find this name a bit hard to understand. Is ApartEvent some kind of
a
voice-agnostic solo-event, that can either be a solo-one- or
solo-two-event? No, it indicates that the two part-combined voices cannot be combined and should be displayed apart (i.e. with voiceOne/voiceTwo). Part-combining has five different states: -) Solo 1 -) Solo 2 -) Unisono (both play instruments play identical notes) and Unisilence (both are quiet) -) Chords (The two voices share the same rhythm/articulations and are not crossing, so they can be written as chords) -) Apart (It's not possible to combine the voices, so they have to be displayed as two separate voices)
Or maybe even make the iterator able to take any arbitrary number of
music
"threads", not just two... Ok, I'm dreaming here.)
Yes, that would be nice, but not that easy... http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newcode38 scm/define-music-types.scm:38: . ((description . "Indicate the part-combine status @q{apart}.") BTW, I can reformulate the description to make its purpose clearer. E.g. something like "Incidate that the part-combined voices cannot be combined and have to be displayed as separate voices." Description: PartCombine: Keep track of the state in the Part_combine_engraver -) Extend iterator to emit events also for apart and together states -) Add a clear-partcombine-event and broadcast it to unused voices to reset the partcombine state This can later on be used to make the engraver print out the texts depending on the previous state, and even for e.g. line breaks or so. Please review this at http://codereview.appspot.com/3334043/ Affected files: M lily/part-combine-engraver.cc M lily/part-combine-iterator.cc M scm/define-event-classes.scm M scm/define-music-types.scm _______________________________________________ lilypond-devel mailing list [email protected] http://lists.gnu.org/mailman/listinfo/lilypond-devel
