Re: Issue 1471: Invalidate alterations upon key change rather than forgetting them. (issue4384050)

[dak]

Reviewers: Graham Percival, Valentin Villenave, Keith,

Message:
On 2011/04/11 22:40:15, Valentin Villenave wrote:

> Doesn't this require a regtest?

The original bug report
<URL:http://code.google.com/p/lilypond/issues/detail?id=1471> has a test
file.  Adding the regtest together with the bug fix makes it impossible
to use make test-baseline in order to check the difference.  I have been
told that the policy documented in the contributor's guide regarding
regtests is different from the actual policy to be used, that this is
intentionally so and should not be changed (otherwise, I would already
have checked this file in).

See <URL:http://permalink.gmane.org/gmane.comp.gnu.lilypond.devel/35703>

This does not make any sense to me, so I decided to leave the regtest
business to the people making the policies.  It is just a matter of
checking in the file in the original bug report.  Whether it is checked
in simultaneously with the bug fix or afterwards makes no difference: it
will not be useful for seeing the difference using "make check" either
way.

Description:
Issue 1471: Invalidate alterations upon key change rather than
forgetting them.

See <URL:http://code.google.com/p/lilypond/issues/detail?id=1471>

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

Affected files:
  M lily/clef-engraver.cc
  M scm/music-functions.scm


Index: lily/clef-engraver.cc
diff --git a/lily/clef-engraver.cc b/lily/clef-engraver.cc
index  
5551cbc07c6eac64e15f22f47f7060aa96dced9e..5ddfea627e0869149ecc4e1c7d65522d437e1a2f  
100644
--- a/lily/clef-engraver.cc
+++ b/lily/clef-engraver.cc
@@ -138,6 +138,15 @@ Clef_engraver::process_music ()
   inspect_clef_properties ();
 }

+static void apply_on_children (Context *context, SCM fun)
+{
+  scm_call_1(fun, context->self_scm());
+  for (SCM s = context->children_contexts ();
+       scm_is_pair(s); s = scm_cdr (s))
+    apply_on_children(unsmob_context (scm_car(s)), fun);
+}
+
+
 void
 Clef_engraver::inspect_clef_properties ()
 {
@@ -152,9 +161,8 @@ Clef_engraver::inspect_clef_properties ()
       || scm_equal_p (octavation, prev_octavation_) == SCM_BOOL_F
       || to_boolean (force_clef))
     {
-      set_context_property_on_children (context (),
-                                       ly_symbol2scm ("localKeySignature"),
-                                       get_property ("keySignature"));
+      apply_on_children(context (),
+                       ly_lily_module_constant ("invalidate-alterations"));

       set_glyph ();
       if (prev_cpos_ != SCM_BOOL_F || to_boolean (get_property  
("firstClef")))
Index: scm/music-functions.scm
diff --git a/scm/music-functions.scm b/scm/music-functions.scm
index  
9790cbf1a3d145a8524b3e20b1b874c59c117de6..25b223bbe9826016cf692c5e3c4f14624aa72176  
100644
--- a/scm/music-functions.scm
+++ b/scm/music-functions.scm
@@ -1004,12 +1004,18 @@ then revert skipTypesetting."
       (equal? laziness #t)
       (<= bar-number (+ (cadr alteration-def) laziness))))

-(define (is-tied? alteration-def)
-  (let* ((def (if (pair? alteration-def)
-                (car alteration-def)
-                alteration-def)))
+(define (accidental-voided? alteration-def)
+  "Checks an alteration entry for being voided.
+
+Non-key alterations are voided when tying into the next bar or when
+there is a clef change, since neither repetition nor cancellation can
+be omitted when the same note occurs again.

-    (if (equal? def 'tied) #t #f)))
+Returns @code{#f} or the reason for the voiding, a symbol."
+  (let* ((def (if (pair? alteration-def)
+                 (car alteration-def)
+                 alteration-def)))
+    (and (symbol? def) def)))

 (define (extract-alteration alteration-def)
   (cond ((number? alteration-def)
@@ -1077,7 +1083,7 @@ specifies whether accidentals should be canceled in  
different octaves."
      (from-key-sig
       (set! previous-alteration from-key-sig)))

-    (if (is-tied? previous-alteration)
+    (if (accidental-voided? previous-alteration)
        (set! need-accidental #t)

        (let* ((prev-alt (extract-alteration previous-alteration))
@@ -1135,10 +1141,14 @@ immediately', that is, only look at key signature.   
@code{#t} is `forever'."
   (and (pair? (car entry)) (cdddr entry)))

 (define (key-entry-alteration entry)
-  "Return the alteration of an entry in localKeySignature."
-  (if (number? (car entry))
-      (cdr entry)
-      (cadr entry)))
+  "Return the alteration of an entry in localKeySignature.
+
+For convenience, returns @code{0} if entry is @code{#f}."
+  (if entry
+      (if (number? (car entry))
+         (cdr entry)
+         (cadr entry))
+      0))

 (define-public (find-pitch-entry keysig pitch accept-global accept-local)
   "Return the first entry in @var{keysig} that matches @var{pitch}.
@@ -1167,9 +1177,7 @@ look at bar lines nor different accidentals at the  
same note name."
     (if (not entry)
        (cons #f #f)
        (let* ((global-entry (find-pitch-entry keysig pitch #t #f))
-              (key-acc (if (equal? global-entry #f)
-                           0
-                           (key-entry-alteration global-entry)))
+              (key-acc (key-entry-alteration global-entry))
               (acc (ly:pitch-alteration pitch))
               (entrymp (key-entry-measure-position entry))
               (entrybn (key-entry-bar-number entry)))
@@ -1185,9 +1193,7 @@ on the same staff line."
     (if (not entry)
        (cons #f #f)
        (let* ((global-entry (find-pitch-entry keysig pitch #f #f))
-              (key-acc (if (equal? global-entry #f)
-                           0
-                           (key-entry-alteration global-entry)))
+              (key-acc (key-entry-alteration global-entry))
               (acc (ly:pitch-alteration pitch))
               (entrymp (key-entry-measure-position entry))
               (entrybn (key-entry-bar-number entry)))
@@ -1371,6 +1377,36 @@ as a context."
        (ly:warning (_ "unknown accidental style: ~S") style)
        (make-sequential-music '()))))))

+(define-public (invalidate-alterations context)
+  "Invalidate alterations in @var{context}.
+
+Elements of @code{'localKeySignature} corresponding to local
+alterations of the key signature have the form
address@hidden'((octave . notename) . (alter barnum . measurepos))}.
+Replace them with a version where @code{alter} is set to @code{'clef}
+to force a repetition of accidentals.
+
+Entries that conform with the current key signature are not invalidated."
+  (let* ((keysig (ly:context-property context 'keySignature)))
+    (set! (ly:context-property context 'localKeySignature)
+         (map-in-order
+          (lambda (entry)
+            (let* ((localalt (key-entry-alteration entry))
+                   (localoct (key-entry-octave entry)))
+              (if (or (accidental-voided? localalt)
+                      (not localoct)
+                      (= localalt
+                         (key-entry-alteration
+                          (find-pitch-entry
+                           keysig
+                           (ly:make-pitch localoct
+                                          (key-entry-notename entry)
+                                          0)
+                           #t #t))))
+                  entry
+                  (cons (car entry) (cons 'clef (cddr entry))))))
+          (ly:context-property context 'localKeySignature)))))
+               
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

 (define-public (skip-of-length mus)