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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
