[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Improve elbowed-hairpin (issue 324800043 by address@hidden)
From: |
thomasmorley65 |
Subject: |
Re: Improve elbowed-hairpin (issue 324800043 by address@hidden) |
Date: |
Mon, 24 Apr 2017 16:24:29 -0700 |
On 2017/04/24 14:42:18, david.nalesnik wrote:
LGTM.
I've pointed out two minor issues, but I don't believe they should
hold up the
review process.
By the way, applying
\override Hairpin.style = #'dashed-line to the regtest
input/regression/ferneyhough-hairpins.ly looks great!
https://codereview.appspot.com/324800043/diff/1/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/324800043/diff/1/scm/output-lib.scm#newcode1171
scm/output-lib.scm:1171: (if (not (pair? (cdr pts)))
Note that this will fail if pts is '(). Perhaps you should check the
length of
pts in elbowed-hairpin -- there must be at least two pairs in the pts
alist for
a viable line.
Yep.
I decided to let the condition for the inner 'connected-points' in
place, but to
emit a meaningful warning if it is called in 'make-connected-line' with
less than
two points. In this case an empty-stencil is returned.
Seems cleaner to do it early than later in 'elbowed-hairpin'.
https://codereview.appspot.com/324800043/diff/1/scm/output-lib.scm#newcode1172
scm/output-lib.scm:1172: (apply ly:stencil-add empty-stencil ls)
Why not use reduce here.
Done.
This warrants a new patchy-run because new functionality is added.
https://codereview.appspot.com/324800043/