[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Fix #687: Include MIDI swing script in default distribution (issue 5
From: |
v . villenave |
Subject: |
Re: Fix #687: Include MIDI swing script in default distribution (issue 572520044 by address@hidden) |
Date: |
Sun, 10 Mar 2019 15:58:05 -0700 |
Reviewers: thomasmorley651,
Message:
Thanks! I actually hadn’t reviewed Johannes’ code in detail, but you
make good points.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly
File ly/swing.ly (right):
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode99
ly/swing.ly:99: (fold (lambda (x y) (and x y)) #t (map rational? lst))))
On 2019/03/10 20:02:06, thomasmorley651 wrote:
Why not
(every rational? lst)
?
Sure.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode102
ly/swing.ly:102: (if (< n 1) '() (append (integers-from-1-to (1- n))
(list n))))
On 2019/03/10 20:02:06, thomasmorley651 wrote:
Why not
(iota n 1 1)
?
Since this function is only called once, and with (- n 1), we don’t even
need anything other than iota.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode112
ly/swing.ly:112: (ly:duration? (ly:music-property music 'duration))))
On 2019/03/10 20:02:06, thomasmorley651 wrote:
Mmh, strange check
Indeed. But I can’t seem to make it work without it.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode119
ly/swing.ly:119: (factr (ly:make-moment (+ 1 (/ delta dur)))))
On 2019/03/10 20:02:06, thomasmorley651 wrote:
Why not
(1+ (/ delta dur))
?
Done.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode163
ly/swing.ly:163: (#t
On 2019/03/10 20:02:06, thomasmorley651 wrote:
(else
Done.
https://codereview.appspot.com/572520044/diff/574520044/ly/swing.ly#newcode229
ly/swing.ly:229: (integers-from-1-to (- n 1))))
On 2019/03/10 20:02:06, thomasmorley651 wrote:
(1- n)
No longer needed if using (iota n 1), anyway.
Description:
Fix #687: Include MIDI swing script in default distribution
This commit is based on Johannes Rohrer’s contribution,
which in turn stemmed in part from other contributor’s work:
https://sourceforge.net/p/testlilyissues/issues/687/?page=1&limit=25#c2cf
(I haven’t contacted Johannes to ask for authorization; I assume
we can safely distribute it under GPL like the rest of LilyPond.
Full credit is given in the source code, and his many comments
have all been preserved.)
Documentation and regtest added, largely inspired by Peter Chubb’s
articulate.ly script.
Please review this at https://codereview.appspot.com/572520044/
Affected files (+609, -7 lines):
M Documentation/notation/input.itely
A input/regression/swing-test.ly
A ly/swing.ly
- Re: Fix #687: Include MIDI swing script in default distribution (issue 572520044 by address@hidden),
v . villenave <=