Re: Issue 1228: 5-month-old PATCH

[Carl Sorensen]

On 1/22/11 12:40 PM, "James Lowe" <address@hidden> wrote:
> OK. I'll have a go.
> 
> Can I just clarify one thing? This particular file/edit uses the *.ily
> 'suffix'. Is this 'special' or just an older/another syntax as I cannot
> find any *ily files in my current source.

.ily was proposed as a .ly file that was intended only to be included.  I
don't know that it ever got traction.

The way to get it into the distribution is probably not as a .ily file.

Probably, we want to create a new file scm/measure-combine.scm that will
contain the scheme functions used.  The scheme functions that should be
callable by other modules (e.g. from LilyPond code) need to be defined using
define-public, instead of define.  That means merge-rests-on-positioning and
merge-multimeasure-rests-on-Y-offset.

While you're at it, I'd recommend a change to the name
merge-multimeasure-rests-on-Y-offset.  Perhaps something like
merge-multimeasure-rests-and-return-Y-offset  I think the name is a bit
clearer.

The procedure rest-score is written in more of a procedural style than a
functional style.  Scheme uses functional programming as its basic idiom.  I
haven't worked it out exactly, but I think that all of the score
calculations could be done in the let block if it were changed to a let*
block.  Generally, we try to avoid using set! when writing Scheme code.

merge-rests-on-positioning has an artificial limit of two rests to merge.
It would seem that another function that takes two grobs and tries to
combine them could be called in a recursive manner, which would allow
combining rests from more than two voices.  But I don't necessarily think
that holding up the patch for this functionality is necessary.

The commands mergeRestsOn and mergeRestsOff should be put into some existing
.ly file.   I'm not sure exactly where yet.  My first thought was
ly/music-functions-init.ly, but they're not written as music functions.
After thinking about it some more, I think they belong in
ly/property-init.ly.

The mergeRests command should probably be adjusted so that it goes into a
layout block, rather than creating a layout block.  It would then become
part of ly/engraver-init.ly (Look at the very end for the definitions of
RemoveEmptyStaffContext).

Finally, you need to create a regtest that demonstrates how it works.

I'll be happy to provide any advice I can about making this work.

Thanks,

Carl