Re: [PATCH] Flexible accidentals code from /dev/rune

[Han-Wen Nienhuys]

On Wed, Aug 20, 2008 at 7:17 PM, Joe Neeman <address@hidden> wrote:
>> > No thanks, I just pushed the second patch and I've done a much smaller
>> > and cleaner version of the first one.
>>
>> This works great! (there was just a glitch in a doc string, which I
>> have taken the liberty to correct myself)
>>
>> Do you think it's clean enough to be submitted to Han-Wen now, or does
>> it still need some polishing?
>
> I think it's ok as-is. There is one TODO (regarding moving a function
> from C++ to scheme) but I don't think it's crucial.

I looked at these commits; generally looks OK.

commit dbe77ae36c3da410adcfdec68553b700c699901a
Author: Valentin Villenave <address@hidden>

    Compile fix

commit bbe968dbe625e5e710f8bc92613ca01be30cfac3
Author: Valentin Villenave <address@hidden>

    Adding a regtest for flexible contemporary accidentals

commit f077f41c3c80e4988ebdd2f9c08110912a8c20e4
Author: Joe Neeman <address@hidden>

    Merge accidental callbacks from dev/rune.

commit bd2e397e9713886346310fbc2a80f8d7e9da8f40
Author: Joe Neeman <address@hidden>

    Cleanups for pitches and scales.



        "Checks the need for an accidental and a 'restore' accidental against a"
+          " key signature.  The laziness is the number of bars for
which reminder"
+          " accidentals are used (ie. if laziness is zero, we only
cancel accidentals"
+          " in the same bar; if laziness is three, we cancel
accidentals up to three"
+          " bars after they first appear.  Octaveness is either
'same-octave or"
+          " 'any-octave and it specifies whether accidentals should
be canceled in"
+          " different octaves.")

isn't this supposed to be texi ?

+  LY_ASSERT_TYPE (unsmob_pitch, pp, 2);

use understandable name for argument pp


+      Duration *dur = unsmob_duration (note->get_property ("duration"));
+      Rational dur_length = dur ? dur->get_length () : Rational (0);
+      Moment mp = robust_scm2moment (get_property
("measurePosition"), Moment (0));
+
+      Moment end_mp = mp.grace_part_ < Rational(0)
+       ? Moment(mp.main_part_, mp.grace_part_ + dur_length)
+       : Moment(mp.main_part_ + dur_length, 0);
+

I have the feeling we must have code for this in a central place. If
not, we should.

+  (if (number? (car entry))
+      #f
+      (caddr entry)))

I think you can write (and CONDITION VALUE) - or in this case (and
(not cond) value)

+      (let* ((entry (car keysig))
+            (entryoct (key-entry-octave entry))
+            (entrynn (key-entry-notename entry))

indentation seems off.  Tabs?

   (c) 2006--2007 Han-Wen Nienhuys <address@hidden>
-
+      2007--2008 Rune Zedeler
+      2008       Joe Neeman <address@hidden>
 */

We should really get started on transfering (c) to the FSF or some
entity, (I have the vague feeling that we already came to consensus
about this, but not sure), and short headers without this individual
author name nonsense.

+          "The argument is a vector of natural numbers, each of which "
+          "represents the number of tones of a pitch above the tonic.")

rational numbers, actually.


+
+\layout { ragged-right = ##t }
+
+\relative c'' {
+  #(set-accidental-style 'dodecaphonic)
+  gis4 a g gisis
+  #(set-accidental-style 'neo-modern)
+  gis8 a gis gis g' gis gis,, a'
+  #(set-accidental-style 'neo-modern-cautionary)
+  eis fis eis eis g2
+}
+

it would be nice if you could find a way to check the results against
a set of 'golden' accidentals, so it is easier to notice if anything
is wrong.  Some of the beam regression tests do this for example.

-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen