Re: Fix mordents and pralltriller in articulate.ly (issue 5829043)

[botialoach]

Reviewers: Graham Percival,

Message:
On 2012/03/15 00:49:01, Graham Percival wrote:
> looks basically good, but two minor points.

One big issue -- the patch is reversed.
My bad in attempting to use git-cl.

I'm currently trying to work out how to delete this issue and create a
new one with the patch the right way around.
> Make that three minor points: please CC to -devel, not -devl.

> http://codereview.appspot.com/5829043/diff/1/ly/articulate.ly
> File ly/articulate.ly (right):


http://codereview.appspot.com/5829043/diff/1/ly/articulate.ly#newcode482
> ly/articulate.ly:482: (let ((pset (make-music 'PropertySet
> The indentation looks a bit off, but we don't have a tool for scheme
> indentation, nor do we even have any consistency in terms of spaces
vs. tabs in
> .scm files, so good enough for me.


http://codereview.appspot.com/5829043/diff/1/ly/articulate.ly#newcode625
> ly/articulate.ly:625: (else  music))
> extra space?


http://codereview.appspot.com/5829043/diff/1/ly/articulate.ly#newcode630
> ly/articulate.ly:630: % At last ... here's the music function that
aplies all
> the above to a
> this adds a typo?



Description:
Fix mordents and pralltriller in articulate.ly


    Mordents should be on-beat, not grace notes.  And pralltrillers
    (half-shakes) are always either 4 alternating notes, or an inverted
    mordent.

    There is of course a general problem here in that the way ornaments
    are realised has changed through the centuries.  Even Bach and
    Clementi disagree! I'm following CPE Bach's `True art of Keyboard
    Playing' in the interpretation here.  To do the job properly we'd
have
    two articulations: \prall and \inverted_mordent and treat them
    separately for MIDI, but typeset the same glyph.

    Reported-by: Christopher Maden <address@hidden>
    Signed-off-by: Peter Chubb <address@hidden>

Please review this at http://codereview.appspot.com/5829043/

Affected files:
  M ly/articulate.ly


Index: ly/articulate.ly
diff --git a/ly/articulate.ly b/ly/articulate.ly
index  
a345468ef71395bb745f1e867fc9d525a4d56526..d2d3b182ffd3dd835e607c53bf39f0f4cb3832f5  
100644
--- a/ly/articulate.ly
+++ b/ly/articulate.ly
@@ -341,18 +341,8 @@
 ;     (ac:accel trillMusic factor))
  )))

-%
-% Generate a tempoChangeEvent and its associated property setting.
-%
-#(define (ac:tempoChange tempo)
-  (make-sequential-music
-   (list (make-music 'TempoChangeEvent
-         'metronome-count
-         tempo
-         'tempo-unit
-         (ly:make-duration 0 0 1 1))
-    (context-spec-music
-    (make-property-set 'tempoWholesPerMinute  tempo) 'Score))))
+
+

 % If there's an articulation, use it.
 % If in a slur, use (1 . 1) instead.
@@ -424,14 +414,6 @@
             (string= t "rall."))
            (loop factor (cons e newelements) tail (cons 'rall actions)))
           ((or
-            (string= t "accelerando")
-            (string= t "accel")
-            (string= t "accel."))
-           (loop factor (cons e newelements) tail (cons 'accel actions)))
-          ((or
-            (string= t "poco accel."))
-           (loop factor (cons e newelements) tail (cons 'pocoAccel actions)))
-          ((or
             (string= t "poco rall.")
             (string= t "poco rit."))
            (loop factor (cons e newelements) tail (cons 'pocoRall actions)))
@@ -495,37 +477,25 @@
 	     (make-music 'RestEvent 'duration (ly:make-duration len dots newnum  
newdenom))))))
          music)))

-       ((accel)
-       (set! ac:lastTempo ac:currentTempo)
-       (set! ac:currentTempo (ly:moment-div ac:currentTempo ac:rallFactor))
-       (let ((pset (ac:tempoChange ac:currentTempo)))
-        (if (null? (cdr actions))
-         (make-sequential-music (list pset music))
-         (make-sequential-music
-          (list pset (loop (cdr actions)))))))
-
-       ((pocoAccel)
-       (set! ac:lastTempo ac:currentTempo)
-       (set! ac:currentTempo (ly:moment-div ac:currentTempo ac:pocoRallFactor))
-       (let ((pset (ac:tempoChange ac:currentTempo)))
-        (if (null? (cdr actions))
-         (make-sequential-music (list pset music))
-         (make-sequential-music
-          (list pset (loop (cdr actions)))))))
-
        ((rall)
-       (set! ac:lastTempo ac:currentTempo)
        (set! ac:currentTempo (ly:moment-mul ac:currentTempo ac:rallFactor))
-       (let ((pset (ac:tempoChange ac:currentTempo)))
+       (let ((pset (make-music 'PropertySet
+          'value
+          ac:currentTempo
+          'symbol
+          'tempoWholesPerMinute)))
         (if (null? (cdr actions))
          (make-sequential-music (list pset music))
          (make-sequential-music
           (list pset (loop (cdr actions)))))))

        ((pocoRall)
-       (set! ac:lastTempo ac:currentTempo)
        (set! ac:currentTempo (ly:moment-mul ac:currentTempo ac:pocoRallFactor))
-       (let ((pset (ac:tempoChange ac:currentTempo)))
+       (let ((pset (make-music 'PropertySet
+          'value
+          ac:currentTempo
+          'symbol
+          'tempoWholesPerMinute)))
         (if (null? (cdr actions))
          (make-sequential-music (list pset music))
          (make-sequential-music
@@ -533,8 +503,11 @@

        ((aTempo)
        (set! ac:currentTempo ac:lastTempo)
-       
-       (let ((pset (ac:tempoChange ac:currentTempo)))
+       (let ((pset (make-music 'PropertySet
+          'value
+          ac:currentTempo
+          'symbol
+          'tempoWholesPerMinute)))
         (if (null? (cdr actions))
          (make-sequential-music (list pset music))
          (make-sequential-music
@@ -649,12 +622,12 @@
      (ac:adjust-props (ly:music-property music 'symbol) music)
      music)

-    (else music))
+    (else  music))
  ))



-% At last ... here's the music function that applies all the above to a
+% At last ... here's the music function that aplies all the above to a
 % score.
 articulate = #(define-music-function (parser location music)
               (ly:music?)