Re: Create engravers for merging rests (issue 321930043 by address@hidden)

[horndude77]

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#newcode10
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#newcode14
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#newcode23
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#newcode23
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#newcode35
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#newcode39
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#newcode44
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#newcode70
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#newcode72
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#newcode81
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.

https://codereview.appspot.com/321930043/