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

[Joe Neeman]

On Thu, 2008-08-21 at 00:57 -0300, Han-Wen Nienhuys wrote:
> 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.

Those are all the commits in question.

> 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 ?

I'm not sure what this means...

> +      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.

I couldn't find it, so I moved this to context.cc.

> +  (if (number? (car entry))
> +      #f
> +      (caddr entry)))
> 
> I think you can write (and CONDITION VALUE) - or in this case (and
> (not cond) value)

Is it ok if I leave this as-is? Maybe it's because I'm not really a
schemer, but I find this much clearer.

> +      (let* ((entry (car keysig))
> +            (entryoct (key-entry-octave entry))
> +            (entrynn (key-entry-notename entry))
> 
> indentation seems off.  Tabs?

As David pointed out, this is due to the '+' in the diff.

>    (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.

I vaguely remember discussing this too (or maybe I'm thinking about the
GPL v3 discussion). Anyway, it's ok with me. I'll put resolving this
copyright stuff on my todo list.

Joe