Add new merge-function file for rests

Add new merge-function files for rests

Taken from Tracker issue 1228

Function to merge rests that occur, for example, in two voices at
the same moment so that only a single rest is printed.

This may not be the best way to implement this but I am hoping this is a start at least.

---

This issue has been closed by myself as it has been suggested that having a separate engraver is a better way to implement this 'hack'. I have updated Tracker 1288 with the latest revision of this issue in case someone else with more programmings skills that I can use what I have already created.

Thanks to Carl and Graham for their help.

[Carl, 2011-01-22 23:29:24]

Great start, James!

Getting this far is more than half the battle.

Are you up for the next round of changes now?

Thanks,

Carl

http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly
File ly/declarations-init.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly#newcode31
ly/declarations-init.ly:31: \include "merge-function.ly"
Once we move ly/merge-function.ly to scm/merge-rests.scm, this will need to move
to scm/lily.scm as part of the definition of init-scheme-files (see lines 393
and thereabouts.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly
File ly/merge-function.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode1
ly/merge-function.ly:1: %{
Once you have moved the commands out to ly/property-init.ly and
ly/engraver-init.ly, there's nothing but scheme code in this file, so it should
become a scheme file.

Let's call it scm/merge-rests.scm.

Put a LilyPond copyright statement at the top of it (use one from any other .scm
file), with Wilbert Berendsen and James Lowe as the copyright holders.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode118
ly/merge-function.ly:118: mergeRestsOn = {
You have properly defined mergeRestsOn and mergeRestsOff in ly/property-init.ly,
so they should be removed here.

mergeRests should be moved to ly/engraver-init.ly (and removed here).

http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly
File ly/property-init.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly#newcode282
ly/property-init.ly:282: mergeRests = \layout {
mergeRests should have the \layout{} block removed, so that it
can be included in anybody's layout block.

It should be in ly/engraver-init.ly.

[Graham Percival (old account), 2011-01-23 00:07:32]

A few nitpicks.  BTW, expect to produce 3-6 drafts.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly
File ly/merge-function.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode3
ly/merge-function.ly:3: 
Having an example is a nice idea, but don't do it here.  Make a separate file
Documentation/snippets/new/merging-rests.ly or something like that.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode8
ly/merge-function.ly:8: \include "merge-rests"
The whole point of this work is so that you _won't_ need to \include
merge-rests, so I'd omit this part.  :)

[Graham Percival (old account), 2011-01-23 00:14:42]

Correction.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly
File ly/merge-function.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode3
ly/merge-function.ly:3: 
On 2011/01/23 00:07:33, Graham Percival wrote:
> Having an example is a nice idea, but don't do it here.  Make a separate file
> Documentation/snippets/new/merging-rests.ly or something like that.

Wait, let me change that.  Forget about documentation at the moment, but write a
regtest.  Just take this example and put it in input/regression/  and add a
texidoc thing like all the other regtests have.

We'll do documentation as a separate patch; don't start thinking about that
until this one has been pushed.

[pkx166h, 2011-01-23 21:10:05]

I have done all the changes (except added a reg test) and uploaded them. However
this does not make cleanly. I am getting an error

current/scm/lily.scm:851:21: Unbound variable: merge-rests-on-positioning

Any help would be appreciated as I can't see any formatting errors.

http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly
File ly/declarations-init.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly#newcode31
ly/declarations-init.ly:31: \include "merge-function.ly"
On 2011/01/22 23:29:24, Carl wrote:
> Once we move ly/merge-function.ly to scm/merge-rests.scm, this will need to
move
> to scm/lily.scm as part of the definition of init-scheme-files (see lines 393
> and thereabouts.

Done.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly
File ly/merge-function.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode1
ly/merge-function.ly:1: %{
On 2011/01/22 23:29:24, Carl wrote:
> Once you have moved the commands out to ly/property-init.ly and
> ly/engraver-init.ly, there's nothing but scheme code in this file, so it
should
> become a scheme file.
> 
> Let's call it scm/merge-rests.scm.
> 
> Put a LilyPond copyright statement at the top of it (use one from any other
.scm
> file), with Wilbert Berendsen and James Lowe as the copyright holders.

Done.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode8
ly/merge-function.ly:8: \include "merge-rests"
On 2011/01/23 00:07:33, Graham Percival wrote:
> The whole point of this work is so that you _won't_ need to \include
> merge-rests, so I'd omit this part.  :)

Done.

http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode118
ly/merge-function.ly:118: mergeRestsOn = {
On 2011/01/22 23:29:24, Carl wrote:
> You have properly defined mergeRestsOn and mergeRestsOff in
ly/property-init.ly,
> so they should be removed here.
> 
> mergeRests should be moved to ly/engraver-init.ly (and removed here).

Done.

http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly
File ly/property-init.ly (right):

http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly#newcode282
ly/property-init.ly:282: mergeRests = \layout {
On 2011/01/22 23:29:24, Carl wrote:
> mergeRests should have the \layout{} block removed, so that it
> can be included in anybody's layout block.
> 
> It should be in ly/engraver-init.ly.
> 

Done.

[c_sorensen, 2011-01-23 21:14:22]

On 1/23/11 2:10 PM, "pkx166h@gmail.com" <pkx166h@gmail.com> wrote:

> Reviewers: carl.d.sorensen_gmail.com, Graham Percival,
> 
> Message:
> I have done all the changes (except added a reg test) and uploaded them.
> However this does not make cleanly. I am getting an error
> 
> current/scm/lily.scm:851:21: Unbound variable:
> merge-rests-on-positioning
> 
> Any help would be appreciated as I can't see any formatting errors.

You need to change (define (merge-rests-on-positioning ....))
to 
(define-public (merge-rests-on-positioning ....))

Do the same for the other function that is set as a property (I can't
remember exactly what it's called now).

HTH,

Carl

[Neil Puttock, 2011-01-23 23:14:13]

This is a nice hack, but the way it's currently implemented makes it unsuitable
to be merged.

The full-bar rests should be collected by an engraver (like the
Rest_collision_engraver), rather than using a hash table.

[pkx166h, 2011-01-24 16:14:49]

Patch v.3 is up. Still having compile issues even after Carl's suggestions - so
I wonder if someone can take a look and see if I have edited in the correct
place.

PS. I am not seeing any emails go to dev when I update or upload so I am not
sure if my gmail account is set up for dev properly. So I apologise if this has
already been seen

Thanks again for your help.

[c_sorensen, 2011-01-24 16:20:20]

On 1/24/11 9:14 AM, "pkx166h@gmail.com" <pkx166h@gmail.com> wrote:

> Patch v.3 is up. Still having compile issues even after Carl's
> suggestions - so I wonder if someone can take a look and see if I have
> edited in the correct place.
> 
> PS. I am not seeing any emails go to dev when I update or upload so I am
> not sure if my gmail account is set up for dev properly. So I apologise
> if this has already been seen

Emails are not automatically sent when you update or upload.  To get an
email sent, just click on Publish and Mail Comments (from the Rietveld issue
page) and announce a new patchset.

But -- Neil's comment indicated that this approach wrong for getting this
functionality into the LilyPond distribution.  He suggests that a new
engraver will be needed.

Therefore, putting this up as an LSR snippet in the original form is
probably the best thing to do right now, IMO.

Thanks,

Carl

[pkx166h, 2011-01-24 16:49:05]

On 2011/01/24 16:20:20, c_sorensen_byu.edu wrote:

snip...

> But -- Neil's comment indicated that this approach wrong for getting this
> functionality into the LilyPond distribution.  He suggests that a new
> engraver will be needed.
> 
> Therefore, putting this up as an LSR snippet in the original form is
> probably the best thing to do right now, IMO.
> 
> Thanks,
> 
> Carl
> 
> 

No problem. What would be the best thing to do with this 'code' would putting
the 'patch' I have on the tracker issue, with explanation and appropriate
comments from this Rietveld thread, be the right thing if only to save re-doing
more work for anyone who'd like to tackle this?

Seems a waste to just close this inasmuch I as a lot of the grunt work has been
done; or maybe not - it wasn't a waste for me in terms of learning though.

James

[c_sorensen, 2011-01-24 18:35:58]

On 1/24/11 9:49 AM, "pkx166h@gmail.com" <pkx166h@gmail.com> wrote:

> On 2011/01/24 16:20:20, c_sorensen_byu.edu wrote:
> 
> snip...
> 
>> But -- Neil's comment indicated that this approach wrong for getting
> this
>> functionality into the LilyPond distribution.  He suggests that a new
>> engraver will be needed.
> 
>> Therefore, putting this up as an LSR snippet in the original form is
>> probably the best thing to do right now, IMO.
> 
>> Thanks,
> 
>> Carl
> 
> 
> 
> No problem. What would be the best thing to do with this 'code' would
> putting the 'patch' I have on the tracker issue, with explanation and
> appropriate comments from this Rietveld thread, be the right thing if
> only to save re-doing more work for anyone who'd like to tackle this?

I think the right thing is to leave this Rietveld issue up and available,
and put a comment about its resolution, along with a link to the patch in
the bug issue thread.

Thanks,

Carl