lilypond-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]