Create engravers for merging rests

Create engravers for merging rests

This commit includes engravers for merging rests and multimeasure rests
among multiple voices.

[Dan Eble, 2017-05-14 19:26:57]

I'm currently trying to give other things priority over Lilypond, so I'm not
going to review this change, but I came to say this: any part of this feature
that you want to remain working for more than a week needs a regression test. 
Here are some categories of tests you might choose to implement:

basic features; things that everyone expects to work

difficult cases; cases that required significant debugging during development;
cases you can foresee others trying (including accidentally) which might cause
problems for your algorithm

known issues; undesired behavior that is important but that you're not currently
inclined to fix (these should receive tickets in the issue tracker after the
feature is merged)

arbitrary decisions: when there isn't a single right way to handle the input,
but you want to draw attention if a future developer changes the results

[horndude77, 2017-05-16 05:58:21]

Create engravers for merging rests

[horndude77, 2017-05-16 06:04:34]

On 2017/05/14 19:26:57, Dan Eble wrote:
> I'm currently trying to give other things priority over Lilypond, so I'm not
> going to review this change, but I came to say this: any part of this feature
> that you want to remain working for more than a week needs a regression test. 
> Here are some categories of tests you might choose to implement:
> 
> basic features; things that everyone expects to work
> 
> difficult cases; cases that required significant debugging during development;
> cases you can foresee others trying (including accidentally) which might cause
> problems for your algorithm
> 
> known issues; undesired behavior that is important but that you're not
currently
> inclined to fix (these should receive tickets in the issue tracker after the
> feature is merged)
> 
> arbitrary decisions: when there isn't a single right way to handle the input,
> but you want to draw attention if a future developer changes the results

I added a regression test. Is it enough to just put the lilypond file in
/input/regression/ for it to be included in the regression test or does it need
to be specified anywhere else?

There is one arbitrary decision around the positioning of multimeasure rests
that are one measure long. It always sets the same vertical staff position. If
the staff were a single line (e.g. percussion) and two voices shared a full bar
rest then they would be combined and positioned incorrectly. It's called out in
a comment should there be a desire to fix that at some point.

Also sorry about my previous message. Figuring out git-cl for updating an issue.

Thanks for the help.

[horndude77, 2017-05-17 06:24:16]

Rebased on master

[thomasmorley651, 2017-05-17 22:01:42]

On 2017/05/17 06:24:16, horndude77 wrote:
> Rebased on master

Hi Jay,

this one will not get a proper review unless
https://sourceforge.net/p/testlilyissues/issues/1228/
is updated. If you have write-access now, you can do it manually:
link to the patch here
switch "Status" -> started
       "Patch" -> new
       "Owner" -> yourself

Better ofcourse you have git-cl properly configurated.
Speaking of it, all messages here should be cc'ed to the devel-list.

Doing 
git config -e
should return (among other data):
[rietveld]
        server = codereview.appspot.com
        cc = lilypond-devel@gnu.org

You can edit this manually as well.

[horndude77, 2017-05-18 05:14:49]

Rebase, set tracker issue

[pkx166h, 2017-05-18 11:54:08]

Passes make, make check and a full make doc.

[david.nalesnik, 2017-05-18 14:15:24]

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly
File ly/init.ly (right):

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36
ly/init.ly:36: #(use-modules (scm merge-rests-engraver))
I'm not sure why you are defining a separate module.  The usual procedure would
be to add your functionality to an existing file in the load list or add your
new file to the load list (in scm/lily.scm -- see init-scheme-files-body).

The Span_stem_engraver, for example, puts the bulk of its code in
scm/music-functions-init.scm.  (There is also some code in
scm.scheme-engravers.scm -- I'm not sure if you ought to add something there.)

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:14: (eq?
Here (and elsewhere) use eqv? to compare numbers.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
Here (and below) use the scheme-engraver macro for consistency.  As far as I'm
aware, all Scheme engravers in the code base use this now.  See
scm/scheme-engravers.scm or input/regression/scheme-text-spanner.ly for
examples.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:35: (if (eq? 'Rest (assoc-ref (ly:grob-property
grob 'meta) 'name))
(See comment about recognizing grobs below.)

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:39: (eq?
eqv?

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:44: (eq? (ly:grob-property mmrest 'measure-count)
1))
eqv?

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:70: (let* ((curr-rests '())
let* not needed -- use let

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:81: (if (eq? 'MultiMeasureRest (assoc-ref
(ly:grob-property grob 'meta) 'name))
You could use grob::name here.  Ordinarily, though, objects are recognized by
their interfaces.  So, here you should try

(if (grob::has-interface grob 'multi-measure-rest-interface)
  [...]

[thomasmorley651, 2017-05-18 20:54:41]

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:10: (define (rest-length rest)
This definition is unused later and wouldn't work because of here undefined
'rest-a'.
Maybe change it to something iterating over a list, comparing their elements
looking at their 'duration-log for Rests and 'measure-count for
MultiMeasureRests.
And use it for checking equal Rests/MMRs.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
Two general questions:
(1)
Is it possible to merge both engravers or is there a use case to have them
separated?
(2)
What do you think about introducing a property to switch rest-merging on/off.
Could be a grob-property, both support rest-interface. Or probably a
context-property because the engraver(s) are in Staff.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:72: `((start-translation-timestep . ,(lambda
(trans)
The order:
start-translation-timestep
stop-translation-timestep
finalize
acknowledgers
feels irritating and does not correspondend to the time they are called.
Any reason I for it, I'm not aware?

[horndude77, 2017-05-19 15:34:57]

Address review comments

[horndude77, 2017-05-20 05:18:38]

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly
File ly/init.ly (right):

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36
ly/init.ly:36: #(use-modules (scm merge-rests-engraver))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> I'm not sure why you are defining a separate module.  The usual procedure
would
> be to add your functionality to an existing file in the load list or add your
> new file to the load list (in scm/lily.scm -- see init-scheme-files-body).
> 
> The Span_stem_engraver, for example, puts the bulk of its code in
> scm/music-functions-init.scm.  (There is also some code in
> scm.scheme-engravers.scm -- I'm not sure if you ought to add something there.)

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:10: (define (rest-length rest)
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> This definition is unused later and wouldn't work because of here undefined
> 'rest-a'.
> Maybe change it to something iterating over a list, comparing their elements
> looking at their 'duration-log for Rests and 'measure-count for
> MultiMeasureRests.
> And use it for checking equal Rests/MMRs.

I removed the method, though you're right I should investigate removing that
duplication.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:14: (eq?
On 2017/05/18 14:15:24, david.nalesnik wrote:
> Here (and elsewhere) use eqv? to compare numbers.

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
On 2017/05/18 14:15:24, david.nalesnik wrote:
> Here (and below) use the scheme-engraver macro for consistency.  As far as I'm
> aware, all Scheme engravers in the code base use this now.  See
> scm/scheme-engravers.scm or input/regression/scheme-text-spanner.ly for
> examples.

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> Two general questions:
> (1)
> Is it possible to merge both engravers or is there a use case to have them
> separated?

Done. They're not combined.

> (2)
> What do you think about introducing a property to switch rest-merging on/off.
> Could be a grob-property, both support rest-interface. Or probably a
> context-property because the engraver(s) are in Staff.

Very good point. I haven't done this yet, but it should be part of this. Will
investigate.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:35: (if (eq? 'Rest (assoc-ref (ly:grob-property
grob 'meta) 'name))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> (See comment about recognizing grobs below.)

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:39: (eq?
On 2017/05/18 14:15:24, david.nalesnik wrote:
> eqv?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:44: (eq? (ly:grob-property mmrest 'measure-count)
1))
On 2017/05/18 14:15:24, david.nalesnik wrote:
> eqv?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:70: (let* ((curr-rests '())
On 2017/05/18 14:15:24, david.nalesnik wrote:
> let* not needed -- use let

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:72: `((start-translation-timestep . ,(lambda
(trans)
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> The order:
> start-translation-timestep
> stop-translation-timestep
> finalize
> acknowledgers
> feels irritating and does not correspondend to the time they are called.
> Any reason I for it, I'm not aware?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver....
scm/merge-rests-engraver.scm:81: (if (eq? 'MultiMeasureRest (assoc-ref
(ly:grob-property grob 'meta) 'name))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> You could use grob::name here.  Ordinarily, though, objects are recognized by
> their interfaces.  So, here you should try
> 
> (if (grob::has-interface grob 'multi-measure-rest-interface)
>   [...]

Since both multimeasure rests and rests have the rest-interface further checking
is required. Take a look at the latest change and let me know if I should check
for the multi-measure-rest-interface instead.

[horndude77, 2017-05-20 06:04:39]

Add suspendRestMerging context property

[thomasmorley651, 2017-05-20 12:18:34]

Much better now, though:

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
The current patch-set fails with:
"Variable used before given a value: rest-eqv"
because subsequent usage of 'define ...' is equivalent to let not let*

Moving (define (rest-eqv rest-len-prop) before the engraver-definiton starts
will work or something at the lines of

...
  (let* ((rest-eqv
           (lambda (rest-len-prop)
             (define (rest-len rest) (ly:grob-property rest rest-len-prop))
             (lambda (rest-a rest-b)
               (eqv? (rest-len rest-a) (rest-len rest-b)))))
         (mmrest-same-length (rest-eqv 'measure-count))
         (rest-same-length (rest-eqv 'duration-log)))

  (define (merge-mmrests rests)
...

[david.nalesnik, 2017-05-20 14:33:15]

Looks much better -- see comments below.

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
Just moving one rest on top of the other might cause offsets with some printers.
 This happened at one time with flags on chords: see
http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html

Would it work to set 'stencil of all of the cdr to point-stencil?

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:193: ((eq? 'MultiMeasureRest (grob::name grob))
The only two grobs you could get are Rest and MultiMeasureRest, so you could
just write

(else
  (set! [...]


[I think looking at the grob name is fine.]

[horndude77, 2017-05-21 00:25:37]

Make overlapped grobs invisible, update regression test for attached text, fix
error

[horndude77, 2017-05-21 04:27:33]

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
On 2017/05/20 12:18:33, thomasmorley651 wrote:
> The current patch-set fails with:
> "Variable used before given a value: rest-eqv"
> because subsequent usage of 'define ...' is equivalent to let not let*
> 
> Moving (define (rest-eqv rest-len-prop) before the engraver-definiton starts
> will work or something at the lines of
> 
> ...
>   (let* ((rest-eqv
>            (lambda (rest-len-prop)
>              (define (rest-len rest) (ly:grob-property rest rest-len-prop))
>              (lambda (rest-a rest-b)
>                (eqv? (rest-len rest-a) (rest-len rest-b)))))
>          (mmrest-same-length (rest-eqv 'measure-count))
>          (rest-same-length (rest-eqv 'duration-log)))
> 
>   (define (merge-mmrests rests)
> ...
> 

Interesting. I didn't run into that error. I'm using the latest lilydev which is
using guile 2. That could be the difference. In any case I've removed the
methods so this shouldn't happen.

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
On 2017/05/20 14:33:15, david.nalesnik wrote:
> Just moving one rest on top of the other might cause offsets with some
printers.
>  This happened at one time with flags on chords: see
> http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html
> 
> Would it work to set 'stencil of all of the cdr to point-stencil?

Using point-stencil causes odd alignment issues with text connected to
multimeasure rests. Making them invisible doesn't have this issue. Would that
solve the above potential problem?

Related to this, is there a way to re-parent grobs onto the surviving rest? If
so we could then use point-stencil (or possibly fully destroy) the other rests.

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:193: ((eq? 'MultiMeasureRest (grob::name grob))
On 2017/05/20 14:33:15, david.nalesnik wrote:
> The only two grobs you could get are Rest and MultiMeasureRest, so you could
> just write
> 
> (else
>   (set! [...]
> 
> 
> [I think looking at the grob name is fine.]

It was simple enough to switch over and use (else ...) as well.

[thomasmorley651, 2017-05-21 10:38:27]

On 2017/05/21 04:27:33, horndude77 wrote:
> https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
> File scm/scheme-engravers.scm (right):
> 
>
https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
> scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
> On 2017/05/20 12:18:33, thomasmorley651 wrote:
> > The current patch-set fails with:
> > "Variable used before given a value: rest-eqv"
> > because subsequent usage of 'define ...' is equivalent to let not let*
> > 
> > Moving (define (rest-eqv rest-len-prop) before the engraver-definiton starts
> > will work or something at the lines of
> > 
> > ...
> >   (let* ((rest-eqv
> >            (lambda (rest-len-prop)
> >              (define (rest-len rest) (ly:grob-property rest rest-len-prop))
> >              (lambda (rest-a rest-b)
> >                (eqv? (rest-len rest-a) (rest-len rest-b)))))
> >          (mmrest-same-length (rest-eqv 'measure-count))
> >          (rest-same-length (rest-eqv 'duration-log)))
> > 
> >   (define (merge-mmrests rests)
> > ...
> > 
> 
> Interesting. I didn't run into that error. I'm using the latest lilydev which
is
> using guile 2. That could be the difference. In any case I've removed the
> methods so this shouldn't happen.

Yep, I can confirm it worked with guilev2, and your recent code works for
guilev1 as well.

> 
>
https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
> scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
> 'Y-offset (rest-offset rest)))
> On 2017/05/20 14:33:15, david.nalesnik wrote:
> > Just moving one rest on top of the other might cause offsets with some
> printers.
> >  This happened at one time with flags on chords: see
> > http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html
> > 
> > Would it work to set 'stencil of all of the cdr to point-stencil?
> 
> Using point-stencil causes odd alignment issues with text connected to
> multimeasure rests. Making them invisible doesn't have this issue. Would that
> solve the above potential problem?
> 
> Related to this, is there a way to re-parent grobs onto the surviving rest? If
> so we could then use point-stencil (or possibly fully destroy) the other
rests.
> 

We have ly:grob-set-parent!
My first attempts to use it were only partly successfull.
I'll attach my attempt at the issue tracker.
https://sourceforge.net/p/testlilyissues/issues/1228/?limit=25&page=1#d443
It's nothing more than a sketch and only tries to care about
MutiMeasureRestText, not MultiMeasureRestNumber.
Something weird happens at line-break.
Anyway, maybe of some use, though...

[thomasmorley651, 2017-05-21 17:12:26]

I'd like to mention another point:
What to do with pitched rests and rests with user-set staff-position, merge them
automatically to the zero-position?

I'd say using suspendRestMerging-property is sufficient to cover this case, but
this is only me. Other opinions?

[Trevor Daniels, 2017-05-21 19:04:46]

On 2017/05/21 17:12:26, thomasmorley651 wrote:
> I'd like to mention another point:
> What to do with pitched rests and rests with user-set staff-position, merge
them
> automatically to the zero-position?

If a user has explicitly set the position of a rest this should be honoured
by default, I think ...
 
> I'd say using suspendRestMerging-property is sufficient to cover this case,
but
> this is only me. Other opinions?

... unless some property (mergePitchedRests?) is set true.

[david.nalesnik, 2017-05-23 18:51:51]

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
On 2017/05/21 04:27:33, horndude77 wrote:
> On 2017/05/20 14:33:15, david.nalesnik wrote:
> > Just moving one rest on top of the other might cause offsets with some
> printers.
> >  This happened at one time with flags on chords: see
> > http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html
> > 
> > Would it work to set 'stencil of all of the cdr to point-stencil?
> 
> Using point-stencil causes odd alignment issues with text connected to
> multimeasure rests. Making them invisible doesn't have this issue. Would that
> solve the above potential problem?

I'm not so sure now that I identified an actual problem, but this an improvement
b/c it will cut down on PDF file size.
> 
> Related to this, is there a way to re-parent grobs onto the surviving rest? If
> so we could then use point-stencil (or possibly fully destroy) the other
rests.

I'd have to look into this, but as a first observation I think it would help if
MultiMeasureRest stored pointers to associated MultiMeasureRestText and
MultiMeasureRestNumber grobs.  (This would be done in the
Multi_measure_rest_engraver.)

I don't think that this is something that needs to be tackled in this patch.

[horndude77, 2017-05-27 19:50:59]

Don't merge pitched rests

[horndude77, 2017-05-27 20:03:43]

> On 2017/05/21 17:12:26, thomasmorley651 wrote:
> > I'd like to mention another point:
> > What to do with pitched rests and rests with user-set staff-position, merge
> them
> > automatically to the zero-position?
> 
> If a user has explicitly set the position of a rest this should be honoured
> by default, I think ...

Done. If the staff-position property is set it won't attempt to merge rests at
that position. Seems to work well.

> > I'd say using suspendRestMerging-property is sufficient to cover this case,
> but
> > this is only me. Other opinions?
> 
> ... unless some property (mergePitchedRests?) is set true.

I didn't create another property for overriding this behavior. Merging pitched
rests could behave unexpectedly for some users - should one of the merged rests
pitches be respected or should they be at the single voice position? Also, I'd
hate to add a property like this which I'd expect to be rarely used. It can be
worked around with tags (i.e. pitched rest for the single part, normal rest for
combined part). If in the future there's a need it could be added when the use
cases are better understood.

[dak, 2017-05-27 20:39:34]

On 2017/05/27 20:03:43, horndude77 wrote:
> > On 2017/05/21 17:12:26, thomasmorley651 wrote:
> > > I'd like to mention another point:
> > > What to do with pitched rests and rests with user-set staff-position,
merge
> > them
> > > automatically to the zero-position?
> > 
> > If a user has explicitly set the position of a rest this should be honoured
> > by default, I think ...
> 
> Done. If the staff-position property is set it won't attempt to merge rests at
> that position. Seems to work well.
> 
> > > I'd say using suspendRestMerging-property is sufficient to cover this
case,
> > but
> > > this is only me. Other opinions?
> > 
> > ... unless some property (mergePitchedRests?) is set true.
> 
> I didn't create another property for overriding this behavior. Merging pitched
> rests could behave unexpectedly for some users - should one of the merged
rests
> pitches be respected or should they be at the single voice position? Also, I'd
> hate to add a property like this which I'd expect to be rarely used. It can be
> worked around with tags (i.e. pitched rest for the single part, normal rest
for
> combined part). If in the future there's a need it could be added when the use
> cases are better understood.

We have a similar situation regarding the directions of slurs.  In
this case, explicit different directions (in the SlurEvent expression,
not in the generated Slur grob) lead to both slurs being retained.

I think it reasonable to retain all unique pitched rests, and when
there are none, a single unpitched rest.  Is this useful?  I don't
really know.

[thomasmorley651, 2017-05-29 23:13:30]

I think this one warrants an entry in changes.

https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm...
scm/scheme-engravers.scm:152: (define (rests-all-unpitched rests)
This will catch not only pitched rests, but also rests where the user has set
Rest.staff-position.
I tend to agree with it, although rests where the user changed 'Y-offset or
'extra-offset are not taken into account. We can't foresee any of those
possibilities, anyway.

Though, I would insert a comment about it for future developers working on it.

In general, why do you use a recursion?
(every (lambda (r) (null? (ly:grob-property r 'staff-position))) rests)
looks at least shorter

[horndude77, 2017-06-12 05:18:30]

Rebase, simplify scheme with every as suggested

[thomasmorley651, 2017-06-12 07:40:42]

One nit. See below. 
No need for a new patch-set, imho. You could change it right before pushing.

Otherwise LGTM

https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/s...
File Documentation/notation/simultaneous.itely (right):

https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/s...
Documentation/notation/simultaneous.itely:917: parts. This can be accomplished
using the merge rests engraver.
I'd use the name, i.e. "Merge_rests_engraver"

[dak, 2017-06-12 08:19:34]

https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/s...
File Documentation/notation/simultaneous.itely (right):

https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/s...
Documentation/notation/simultaneous.itely:917: parts. This can be accomplished
using the merge rests engraver.
On 2017/06/12 07:40:42, thomasmorley651 wrote:
> I'd use the name, i.e. "Merge_rests_engraver"

In that case I'd write

    using @code{Merge_rests_engraver}

namely omitting "the".

[horndude77, 2017-06-14 05:03:38]

Minor edit to docs