[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] part combiner flexibility
From: |
Han-Wen Nienhuys |
Subject: |
Re: [PATCH] part combiner flexibility |
Date: |
Sun, 7 Sep 2008 11:54:42 -0300 |
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