Re: Issue 4622: create a <Part-combiner> class (issue 265260043 by address@hidden)

[dak]

On 2015/09/30 12:18:07, Dan Eble wrote:
> On 2015/09/30 11:23:11, dak wrote:
> >
> > I don't see where you want to be going with this patch.  What is it
supposed
> to
> > make easier for the programmer or the user?

> Well, for the typical user, nothing.  For people like me, it reduces
the amount
> of internal Lilypond code that must be duplicated to combine parts
with
> non-default settings (e.g. without solo passages).  With master, I
need to
> duplicate make-directed-part-combine-music.  After this patch, I will
be able to
> create my own <Part-combiner> and say just this:

> dfeVoiceCombine =
> #(define-music-function (part1 part2)
>     (ly:music? ly:music?)
>     (combine-two-parts-center dfe-voice-part-combiner part1 part2))

> Also the internal code is better organized (IMO).  The process (parts
-> event
> lists -> split list ...) is more obvious.  The dependence on specific
context
> names has been pushed outward a bit.

Well, my problem with non-default settings like "without solo passages"
is the standard one: changing such settings may well be desirable during
part combiner operation, so it seems like making any such settings
actual context properties would be the more flexible path.  And instead
of creating your own <Part-combiner> you'll be able to create your own
context mod with a particular set of initial settings.

Now make no mistake: the internals of the part combiner and its state
management are rather messy and non-modular.  If it would be two
additional lines to have another property like partCombineForced tracked
and extracted, that would make things nicer.  But that concerns more how
the part combiner is organized and controlled internally.  I don't think
that externally we have much to gain from having it fall apart into
different implementations rather than using different parameters for
driving a single partcombiner.

https://codereview.appspot.com/265260043/