[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Create engravers for merging rests (issue 321930043 by address@hidde
From: |
david . nalesnik |
Subject: |
Re: Create engravers for merging rests (issue 321930043 by address@hidden) |
Date: |
Thu, 18 May 2017 07:15:23 -0700 |
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#newcode14
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#newcode23
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#newcode35
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#newcode39
scm/merge-rests-engraver.scm:39: (eq?
eqv?
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))
eqv?
https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode70
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#newcode81
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)
[...]
https://codereview.appspot.com/321930043/
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), pkx166h, 2017/05/18
- Re: Create engravers for merging rests (issue 321930043 by address@hidden),
david . nalesnik <=
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), thomasmorley65, 2017/05/18
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), horndude77, 2017/05/20
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), thomasmorley65, 2017/05/20
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), david . nalesnik, 2017/05/20
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), horndude77, 2017/05/21
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), thomasmorley65, 2017/05/21
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), thomasmorley65, 2017/05/21
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), tdanielsmusic, 2017/05/21
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), david . nalesnik, 2017/05/23
- Re: Create engravers for merging rests (issue 321930043 by address@hidden), horndude77, 2017/05/27