Re: [PATCH] part combiner flexibility

[Han-Wen Nienhuys]

Some random comments:

+  // these contexts have a many-to-one relationship to handles_[]
+  struct Part_combine_outlets
+  {
+    Context *null;
+    Context *apart1;
+    Context *apart2;
+    Context *solo1;
+    Context *solo2;
+    Context *chords;
+    Context *rests;
+  } ctx_;
+
+  // unique context handles -- [0] is a Devnull context, others are Voice
+  Context_handle handles_[sizeof(Part_combine_outlets) / sizeof(Context*)];
+

Why don't you use a map<string, Context*> or perhaps a
Scheme_hash_table to setup the mapping?  If we go through with this
(which I doubt), the handles_ should be a vector<> so we get bounds
checking.

+      // After the first Voice context, find others below it to avoid creating
+      // more than one staff.

clever!

+  // Create the Voice contexts named in music properties.
+  ctx_.apart1 = construct_child ("apart-one-context-id");
+  ctx_.apart2 = construct_child ("apart-two-context-id");
+  ctx_.solo1  = construct_child ("solo-one-context-id");
+  ctx_.solo2  = construct_child ("solo-two-context-id");

+     (apart-one-context-id ,string?
+                          "Name of up-facing Voice for part combiner.")
+     (apart-two-context-id ,string?
+                          "Name of down-facing Voice for part combiner.")

1. why is this a music property?  Since it is all about contexts, I
think a context property would be better, at least from an abstract
perspective.  It may make initialization more tricky for the user.

2. you're adding a lot of symbols for analogous functions. How about
having a single alist?

  \set Staff.partCombineContextNames = #'((apart-one . One) (devnull .
null) ... )

This makes the override a little more complex for the user, but again,
I think it's preferable to polluting the namespace.

Include the testing .ly file in the patch, in input/regression/ ; it
should have a docstring detailing the test case. - I think your
current example is longish.


On Sun, Sep 7, 2008 at 10:50 AM, Dan Eble <address@hidden> wrote:
> I apologize if there's something amiss in this patch; this is the first time
> I've used git.  I was able to build Lilypond from a tarball, but I'm having
> problems running configure in my git-cloned directory, so I haven't built
> with this exact patch.

try running autogen.sh

> The attached example voicecombine.ly should make it obvious why these
> changes are useful (lyricsto and unison stems).
>
> There are pre-existing issues with combining rests which are too much for me
> to bite off right now, but I would like to fix those too, eventually.
>
> Regards,
> --
> Dan
>
>
> _______________________________________________
> lilypond-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/lilypond-devel
>
>



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