Make tab-note-heads and fretboards use common code.

Make tab-note-heads and fretboards use common code.
This combines the string and fret assigning code of the
tab-note-heads engraver and the fret-boards engraver.  Both
will use scheme procedures defined in scm/translation-functions.scm
and specified by context properties initialized in ly/engraver-init.ly

 * Modify the calling sequence of noteToFretFunction so that it
   takes a context, a note-list, a string-list, and an optional
   grob.  If grob is included, noteToFretFunction will add the
   fretboard to the grob.  If grob is not included, noteToFretFunction
   will return a list of (string fret finger) tuples.

 * Refactor the code in scm/translation-functions.scm to support
   this separation of function.

 * predefinedDiagramTable is now a context property for TabStaff as
   well as for FretBoards.  This means that if a chord is present in
   TabStaff, and predefinedDiagramTable for that TabStaff is not #f,
   and a predefined diagram for the chord exists, the TabStaff will
   display the notes of the predefined diagram.

 * Change tab-note-heads-engraver so that it calls noteToFretFunction
   instead of having the fret calculation code inside the engraver.

 * Change the noteToFretFunction calling sequence in fretboard-engraver
   to match the new definition

[Neil Puttock, 2010-01-23 16:42:33]

http://codereview.appspot.com/186268/diff/1/2
File lily/fretboard-engraver.cc (left):

http://codereview.appspot.com/186268/diff/1/2#oldcode100
lily/fretboard-engraver.cc:100: SCM changes = get_property("chordChanges");
get_property (

http://codereview.appspot.com/186268/diff/1/2#oldcode101
lily/fretboard-engraver.cc:101: if (to_boolean (changes) &&
scm_is_pair(last_fret_notes_)
scm_is_pair (

http://codereview.appspot.com/186268/diff/1/2
File lily/fretboard-engraver.cc (right):

http://codereview.appspot.com/186268/diff/1/2#newcode96
lily/fretboard-engraver.cc:96: ly_cxx_vector_to_list (tabstring_events_),
indent (hard tabs)

(and below in ADD_TRANSLATOR ())

http://codereview.appspot.com/186268/diff/1/3
File lily/tab-note-heads-engraver.cc (right):

http://codereview.appspot.com/186268/diff/1/3#newcode81
lily/tab-note-heads-engraver.cc:81: vector<Stream_event*> string_events;
Stream_event *

http://codereview.appspot.com/186268/diff/1/3#newcode106
lily/tab-note-heads-engraver.cc:106: a string_event is generated, so if there
was no string
string-number-event

http://codereview.appspot.com/186268/diff/1/3#newcode136
lily/tab-note-heads-engraver.cc:136: SCM pos_proc = get_property
("tablatureStaffPosition");
For normal staves, we have staffLineLayoutFunction, so something similar would
be better (though tabStaffLineLayoutFunction is a bit cumbersome).

http://codereview.appspot.com/186268/diff/1/3#newcode139
lily/tab-note-heads-engraver.cc:139: for (int i=0; i < scm_to_int (scm_length
(string_fret_finger)); i++)
(int i = 0; i < scm_ilength (string_fret_finger)); i++)

http://codereview.appspot.com/186268/diff/1/3#newcode149
lily/tab-note-heads-engraver.cc:149: context ()->self_scm ());
More a general comment, but we're lacking consistency in argument ordering for
translation functions: some start with the context, whereas others place it
last.

Since you're adding a rest arg to one of the functions, perhaps we should settle
for context first for all functions.

http://codereview.appspot.com/186268/diff/1/4
File ly/engraver-init.ly (right):

http://codereview.appspot.com/186268/diff/1/4#newcode621
ly/engraver-init.ly:621: %% One may change the string tunings as following :
follows:

http://codereview.appspot.com/186268/diff/1/6
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/186268/diff/1/6#newcode353
scm/define-context-properties.scm:353: tabstring events, a boolean defining
whether to make a fretboard,
I assume you removed the boolean arg in an earlier revision.

http://codereview.appspot.com/186268/diff/1/6#newcode460
scm/define-context-properties.scm:460: note head.  Called with two arguments:
string number and a fret number
Removing the context makes this less flexible.

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

http://codereview.appspot.com/186268/diff/1/7#newcode193
scm/translation-functions.scm:193: (acons 'string-count my-string-count '())
If details is null, then this line is equivalent to the following line.

http://codereview.appspot.com/186268/diff/1/7#newcode218
scm/translation-functions.scm:218: (map (lambda (x) (list 'mute  (1+ x)))
'mute (1+ x)

http://codereview.appspot.com/186268/diff/1/7#newcode236
scm/translation-functions.scm:236: "Convert @var{placement-list} to string-fret
list."
indentation

http://codereview.appspot.com/186268/diff/1/7#newcode284
scm/translation-functions.scm:284: (determine-frets-and-strings
indentation

(and following lines)

http://codereview.appspot.com/186268/diff/1/7#newcode287
scm/translation-functions.scm:287: (ensure-number
Default for ly:context-property instead?

http://codereview.appspot.com/186268/diff/1/7#newcode377
scm/translation-functions.scm:377: ;;; body of determined-frets-and-strings
determine

http://codereview.appspot.com/186268/diff/1/7#newcode419
scm/translation-functions.scm:419: ((= 0 (length labels))
Need to keep context to read fretLabels

http://codereview.appspot.com/186268/diff/1/7#newcode420
scm/translation-functions.scm:420: (string (integer->char (+ fret (char->integer
#\a)))))
fret-number

(same below)

http://codereview.appspot.com/186268/diff/1/7#newcode432
scm/translation-functions.scm:432: (format "~a" fret-number)))
indent

http://codereview.appspot.com/186268/diff/1/7#newcode442
scm/translation-functions.scm:442: (number->string (cond
indent

[Carl, 2010-01-24 01:36:35]

Neil,

Thanks for the careful review.

I think I've dealt with everything, but there is still an open question on
ly:context-property.  As far as I can see, there is not currently a means of
putting a default in the ly:context-property call.  I can see that it would be
good to have that.  I'm not sure I know how to do it, but I'll look into it.

Thanks again,

Carl

http://codereview.appspot.com/186268/diff/1/2
File lily/fretboard-engraver.cc (right):

http://codereview.appspot.com/186268/diff/1/2#newcode96
lily/fretboard-engraver.cc:96: ly_cxx_vector_to_list (tabstring_events_),
On 2010/01/23 16:42:33, Neil Puttock wrote:
> indent (hard tabs)
> 
> (and below in ADD_TRANSLATOR ())

Done.

http://codereview.appspot.com/186268/diff/1/3
File lily/tab-note-heads-engraver.cc (right):

http://codereview.appspot.com/186268/diff/1/3#newcode81
lily/tab-note-heads-engraver.cc:81: vector<Stream_event*> string_events;
On 2010/01/23 16:42:33, Neil Puttock wrote:
> Stream_event *

Done.

http://codereview.appspot.com/186268/diff/1/3#newcode106
lily/tab-note-heads-engraver.cc:106: a string_event is generated, so if there
was no string
On 2010/01/23 16:42:33, Neil Puttock wrote:
> string-number-event
> 
> 

Done.

http://codereview.appspot.com/186268/diff/1/3#newcode136
lily/tab-note-heads-engraver.cc:136: SCM pos_proc = get_property
("tablatureStaffPosition");
On 2010/01/23 16:42:33, Neil Puttock wrote:
> For normal staves, we have staffLineLayoutFunction, so something similar would
> be better (though tabStaffLineLayoutFunction is a bit cumbersome).

Done.

http://codereview.appspot.com/186268/diff/1/3#newcode139
lily/tab-note-heads-engraver.cc:139: for (int i=0; i < scm_to_int (scm_length
(string_fret_finger)); i++)
On 2010/01/23 16:42:33, Neil Puttock wrote:
> (int i = 0; i < scm_ilength (string_fret_finger)); i++)

Done.

http://codereview.appspot.com/186268/diff/1/3#newcode149
lily/tab-note-heads-engraver.cc:149: context ()->self_scm ());
On 2010/01/23 16:42:33, Neil Puttock wrote:
> More a general comment, but we're lacking consistency in argument ordering for
> translation functions: some start with the context, whereas others place it
> last.
> 
> Since you're adding a rest arg to one of the functions, perhaps we should
settle
> for context first for all functions.
> 

Done.

http://codereview.appspot.com/186268/diff/1/4
File ly/engraver-init.ly (right):

http://codereview.appspot.com/186268/diff/1/4#newcode621
ly/engraver-init.ly:621: %% One may change the string tunings as following :
On 2010/01/23 16:42:33, Neil Puttock wrote:
> follows:

Done.

http://codereview.appspot.com/186268/diff/1/6
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/186268/diff/1/6#newcode353
scm/define-context-properties.scm:353: tabstring events, a boolean defining
whether to make a fretboard,
On 2010/01/23 16:42:33, Neil Puttock wrote:
> I assume you removed the boolean arg in an earlier revision.

Oops -- yes I did.  I wanted a boolean, but there is not a scm_call_5, and
anytime the boolean was true I needed a grob, so I just used the grob as the
fretboard indicator.

Fixed.

http://codereview.appspot.com/186268/diff/1/6#newcode460
scm/define-context-properties.scm:460: note head.  Called with two arguments:
string number and a fret number
On 2010/01/23 16:42:33, Neil Puttock wrote:
> Removing the context makes this less flexible.

Done.

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

http://codereview.appspot.com/186268/diff/1/7#newcode193
scm/translation-functions.scm:193: (acons 'string-count my-string-count '())
On 2010/01/23 16:42:33, Neil Puttock wrote:
> If details is null, then this line is equivalent to the following line.

D'oh! This was old code that I just moved in the file.  Good catch!

Fixed

http://codereview.appspot.com/186268/diff/1/7#newcode218
scm/translation-functions.scm:218: (map (lambda (x) (list 'mute  (1+ x)))
On 2010/01/23 16:42:33, Neil Puttock wrote:
> 'mute (1+ x)

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode236
scm/translation-functions.scm:236: "Convert @var{placement-list} to string-fret
list."
On 2010/01/23 16:42:33, Neil Puttock wrote:
> indentation

What is the indentation issue here?

http://codereview.appspot.com/186268/diff/1/7#newcode287
scm/translation-functions.scm:287: (ensure-number
On 2010/01/23 16:42:33, Neil Puttock wrote:
> Default for ly:context-property instead?

Do you mean to modify ly:context-property so that it asks for a (probably
optional) default, and then returns the default if the property isn't found?

http://codereview.appspot.com/186268/diff/1/7#newcode377
scm/translation-functions.scm:377: ;;; body of determined-frets-and-strings
On 2010/01/23 16:42:33, Neil Puttock wrote:
> determine

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode419
scm/translation-functions.scm:419: ((= 0 (length labels))
On 2010/01/23 16:42:33, Neil Puttock wrote:
> Need to keep context to read fretLabels
> 
> 

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode420
scm/translation-functions.scm:420: (string (integer->char (+ fret (char->integer
#\a)))))
On 2010/01/23 16:42:33, Neil Puttock wrote:
> fret-number
> 
> (same below)
> 
> 

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode432
scm/translation-functions.scm:432: (format "~a" fret-number)))
On 2010/01/23 16:42:33, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode442
scm/translation-functions.scm:442: (number->string (cond
On 2010/01/23 16:42:33, Neil Puttock wrote:
> indent

Done.

[Neil Puttock, 2010-01-26 01:59:04]

On 2010/01/24 01:36:35, Carl wrote:

> I think I've dealt with everything, but there is still an open question on
> ly:context-property.  As far as I can see, there is not currently a means of
> putting a default in the ly:context-property call.  I can see that it would be
> good to have that.  I'm not sure I know how to do it, but I'll look into it.

Exactly.  Though it's not going to be as useful as defaults in ly:grob-property,
it'd be a nice enhancement.

> Oops -- yes I did.  I wanted a boolean, but there is not a scm_call_5, and
> anytime the boolean was true I needed a grob, so I just used the grob as the
> fretboard indicator.

Though it's undocumented, there is a scm_call_5 (I used it when fixing
\put-adjacent).  You could use scm_call_n instead of course.

> What is the indentation issue here?

The two-space indentation, e.g., below

(let ((string-frets

or

(ensure-number

Regards,
Neil

[c_sorensen, 2010-01-26 13:05:42]

On 1/25/10 6:59 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> On 2010/01/24 01:36:35, Carl wrote:
> 
>> I think I've dealt with everything, but there is still an open
> question on
>> ly:context-property.  As far as I can see, there is not currently a
> means of
>> putting a default in the ly:context-property call.  I can see that it
> would be
>> good to have that.  I'm not sure I know how to do it, but I'll look
> into it.
> 
> Exactly.  Though it's not going to be as useful as defaults in
> ly:grob-property, it'd be a nice enhancement.

Should this be a separate patch, probably applied before the current patch?

> 
>> Oops -- yes I did.  I wanted a boolean, but there is not a scm_call_5,
> and
>> anytime the boolean was true I needed a grob, so I just used the grob
> as the
>> fretboard indicator.
> 
> Though it's undocumented, there is a scm_call_5 (I used it when fixing
> \put-adjacent).  You could use scm_call_n instead of course.

I tried scm_call_5, and got a compile error.  Then I looked at the guile
page, and no scm_call_5, nor a scm_call_n.

git grep scm_call_5 returns nothing.

So does git grep scm_call_n.

How can I use scm_call_5 or scm_call_n?  (Although I think the solution I
got is actually just as good).


> 
>> What is the indentation issue here?
> 
> The two-space indentation, e.g., below
> 
> (let ((string-frets
> 
> or
> 
> (ensure-number

Yes, I found and fixed those.  This question was about lines 235-239

  (define placement-list->....
   "Convert .....
   (map (lambda (x) (...)
        (filter ((lambda (l) ...) placement-list))))

That indentation seems to me to be correct.

Thanks,

Carl

[Carl, 2010-01-28 04:42:34]

OK, so I added the optional default to ly:context-property, and fixed the
indentation.

make check works.

I think it's good to go now.

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

http://codereview.appspot.com/186268/diff/1/7#newcode236
scm/translation-functions.scm:236: "Convert @var{placement-list} to string-fret
list."
On 2010/01/24 01:36:35, Carl wrote:
> On 2010/01/23 16:42:33, Neil Puttock wrote:
> > indentation
> 
> What is the indentation issue here?

Done.

http://codereview.appspot.com/186268/diff/1/7#newcode287
scm/translation-functions.scm:287: (ensure-number
On 2010/01/24 01:36:35, Carl wrote:
> On 2010/01/23 16:42:33, Neil Puttock wrote:
> > Default for ly:context-property instead?
> 
> Do you mean to modify ly:context-property so that it asks for a (probably
> optional) default, and then returns the default if the property isn't found?
> 

Done.

[Carl, 2010-01-28 06:12:56]

I've found a bug in this code that showed up with the latest changes in the
regression tests.

I'll have a new patch set later; don't spend any time on this one right now.

THanks,

Carl

[Carl, 2010-01-28 06:13:27]

I've found a bug in this code that showed up with the latest changes in the
regression tests.

I'll have a new patch set later; don't spend any time on this one right now.

THanks,

Carl

[Carl, 2010-01-29 06:26:04]

The patch set now seems to be complete.  I've responded to Neil's comments, and
regression tests all pass.

Please review.

Thanks,

Carl

[Neil Puttock, 2010-01-30 18:44:15]

Hi Carl,

I've just tested this, and it breaks two regtests:

1) dead-notes.ly

The \deadNote command inside a chord is ignored (i.e., the tabs appear as
numbers rather than crosses).  Replacing \deadNote with \tweak also fails.

2) tablature-harmonic.ly

The harmonic brackets have disappeared.  Weirdly, if you move the \harmonic
command between the notes in the chord, both heads get harmonics.

Regards,
Neil

http://codereview.appspot.com/186268/diff/3011/3012
File lily/context-scheme.cc (right):

http://codereview.appspot.com/186268/diff/3011/3012#newcode97
lily/context-scheme.cc:97: " If @var{def} is given, and property value is
SCM_EOL,"
SCM_EOL will be unfamiliar to most users

[c_sorensen, 2010-01-30 18:52:58]

On 1/30/10 11:44 AM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> Hi Carl,
> 
> I've just tested this, and it breaks two regtests:
> 
> 1) dead-notes.ly
> 
> The \deadNote command inside a chord is ignored (i.e., the tabs appear
> as numbers rather than crosses).  Replacing \deadNote with \tweak also
> fails.

I saw this earlier, but my last regtest check didn't show it up.  I wonder
what I'm doing wrong with my regtest running.

I never fixed the deadNote error; when the regtest run came back clean I
forgot about it.

> 
> 2) tablature-harmonic.ly
> 
> The harmonic brackets have disappeared.  Weirdly, if you move the
> \harmonic command between the notes in the chord, both heads get
> harmonics.

I'll check it out.

> 
> Regards,
> Neil
> 
> 
> http://codereview.appspot.com/186268/diff/3011/3012
> File lily/context-scheme.cc (right):
> 
> http://codereview.appspot.com/186268/diff/3011/3012#newcode97
> lily/context-scheme.cc:97: " If @var{def} is given, and property value
> is SCM_EOL,"
> SCM_EOL will be unfamiliar to most users

Thanks, I'll fix it.

Carl