Fix Issue 1035 -- Add context property for negative frets

Fix Issue 1035 -- Add context property for negative frets
Add handleNegativeFrets, with possibilities of 'ignore,
  'recalculate, 'include

Reorder functions in scm/translation-functions.scm to put
  context in scope of calculate-frets-and-strings

Add regression test.

[Carl, 2011-01-17 23:10:12]

I've posted a patch for fixing issue 1035, by giving the user control over
what to do with negative fret numbers demanded by an assigned string.

The default behavior is to recalculate the note and put it in the tablature or
fretboard as if it had not had a string assigned.  It can also be set to ignore
the note (leaving it out of the tablature or fretboard) or to include the note
(giving a negative fret number in the tablature or fretboard.

Please review.

Thanks,

Carl

[marc, 2011-01-18 19:42:41]

Hi Carl,

LGTM - I don't think that many users will ever
change handleNegativeFrets, but your solution
is encompassing and leaves nothing to be desired ;-)

Regards,

Marc

http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm#ne...
scm/translation-functions.scm:394: ((eq? handle-negative 'recalculate)
In case of a resulting negative fret there should be
at least a warning that lilypond recalculates the
fret on another string IMHO.

[Carl, 2011-01-18 21:24:54]

Thanks, Marc.  Good suggestion.

http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm#ne...
scm/translation-functions.scm:394: ((eq? handle-negative 'recalculate)
On 2011/01/18 19:42:41, marc wrote:
> In case of a resulting negative fret there should be
> at least a warning that lilypond recalculates the
> fret on another string IMHO.

Thanks for the idea.  I agree.  Done.

[Neil Puttock, 2011-01-23 17:54:23]

Hi Carl,

Is moving `determine-frets-and-strings' required for the patch to work?  It
makes reviewing the changes difficult.

Cheers,
Neil

[c_sorensen, 2011-01-23 19:11:46]

On Jan 23, 2011, at 10:54 AM, "n.puttock@gmail.com"  
<n.puttock@gmail.com> wrote:

> Hi Carl,
>
> Is moving `determine-frets-and-strings' required for the patch to  
> work?
> It makes reviewing the changes difficult.
>

If I don't move it, I will need to add an argument for either the  
context or the property value.  I think it's much cleaner to move  
determine-frets-and-strings inside the scope where context is defined.

It probably should have been there in the first place.

I could make a separate patch that moves it, and then compare against  
that patch, if you think it's worth it

Thanks,

Carl


> http://codereview.appspot.com/4056041/

[Neil Puttock, 2011-01-23 22:25:45]

On 2011/01/23 19:11:46, c_sorensen_byu.edu wrote:

> I could make a separate patch that moves it, and then compare against  
> that patch, if you think it's worth it

Thanks, but don't worry about it.  I'll try that myself once I've applied the
patch.

Cheers,
Neil

[Graham Percival (old account), 2011-02-01 23:04:33]

LGTM, and passes regtests.

[Neil Puttock, 2011-02-02 23:55:59]

LGTM.

Just needs rebasing (I assume you don't want to delete
tablature-dot-placement.ly)

[Carl, 2011-02-04 04:21:11]

On 2011/02/02 23:55:59, Neil Puttock wrote:
> LGTM.
> 
> Just needs rebasing (I assume you don't want to delete
> tablature-dot-placement.ly)

Done