Re: Make tab-note-heads and fretboards use common code. (issue186268)

[Carl . D . Sorensen]

Reviewers: Neil Puttock,

Message:
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.

Description:
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

Please review this at http://codereview.appspot.com/186268/show

Affected files:
  M lily/fretboard-engraver.cc
  M lily/tab-note-heads-engraver.cc
  M ly/engraver-init.ly
  M ly/property-init.ly
  M scm/define-context-properties.scm
  M scm/translation-functions.scm