Re: Merge_rests_engraver: fix vertical rest positions (issue 334740043 by address@hidden)

[thomasmorley65]

Hi Malte,

your patch fixes the "magnifyStaff"-problem.

Though, this functionality needs testing, imho.
I'd extend the reg-test merge-rests-engraver.ly or add a new one.

Some other thoughts:


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

https://codereview.appspot.com/334740043/diff/1/scm/scheme-engravers.scm#newcode140
scm/scheme-engravers.scm:140: (define (rest-eqv rest-len-prop)
While you're on it, I'd prefer rest-eqv? for a predicate returning a
boolean. Here and at several other instances.

https://codereview.appspot.com/334740043/diff/1/scm/scheme-engravers.scm#newcode214
scm/scheme-engravers.scm:214: (merge-rests rests (lambda (r) 0))
'merge-rests' as defined above expects a procedure as second argument,
so there is the need to put in a procedure here as well.
But wouldn't it be better to delete the 'position-function'-argument
from the definition and derive the local 'staff-pos'-variable from
(if (is-single-bar-rest? (car rests)) 2 0)
directly?
With the advantage one could then delete the entire 'mmrest-position',
at least after correcting the other code-parts expecting
position-function.
Then here only
(merge-rests rests)
remains.

https://codereview.appspot.com/334740043/