Re: Add option to indicate frets by letters in tablature (issue164063)

[n . puttock]

Hi Trevor,

Here are a few comments as promised earlier.

Cheers,
Neil


http://codereview.appspot.com/164063/diff/5001/5002
File Documentation/changes.tely (right):

http://codereview.appspot.com/164063/diff/5001/5002#newcode73
Documentation/changes.tely:73: \new TabStaff
trailing space

http://codereview.appspot.com/164063/diff/5001/5002#newcode79
Documentation/changes.tely:79: \set fretLabels = #`(,(markup
#:with-color red "a")
trailing space

http://codereview.appspot.com/164063/diff/5001/5002#newcode80
Documentation/changes.tely:80: "b"
trailing space

http://codereview.appspot.com/164063/diff/5001/5003
File input/regression/tablature-letter.ly (right):

http://codereview.appspot.com/164063/diff/5001/5003#newcode23
input/regression/tablature-letter.ly:23: \set fretLabels = #`("α" "β"
"γ")
#'

http://codereview.appspot.com/164063/diff/5001/5005
File lily/grob.cc (right):

http://codereview.appspot.com/164063/diff/5001/5005#newcode160
lily/grob.cc:160: /* Call the scheme procedure stencil-whiteout in
scm/stencils.scm */
I'd use { } here if you want this comment placed before the code.

http://codereview.appspot.com/164063/diff/5001/5005#newcode162
lily/grob.cc:162: retval = *unsmob_stencil (scm_call_1 (
If you newline at the parenthesis, the following lines should be
indented accordingly, but this will make the line quite long so it's
probably better to start on the following line:

retval
  = *unsmob_stencil (scm_call_1 (ly_lily_module_constant
("stencil-whiteout"),
                                 retval.smobbed_copy()));

http://codereview.appspot.com/164063/diff/5001/5010
File scm/translation-functions.scm (right):

http://codereview.appspot.com/164063/diff/5001/5010#newcode398
scm/translation-functions.scm:398: (make-vcenter-markup
    (make-vcenter-markup
     (cond
      ((= 0 (length labels))
       (string (integer->char (+ fret (char->integer #\a)))))
      ((and (<= 0 fret) (< fret (length labels)))
       (list-ref labels fret))
      (else
       (ly:warning "No label for fret ~a (~a on string ~a);
only ~a fret labels provided"
                   fret pitch string-number (length labels))
       ".")))))

http://codereview.appspot.com/164063/diff/5001/5010#newcode414
scm/translation-functions.scm:414: (make-vcenter-markup
    (make-vcenter-markup
     (format
      "~a"
      (- (ly:pitch-semitones pitch)
         (list-ref tuning
                   ;; remove 1 because list index starts at 0
                   ;;and guitar string at 1.
                   (1- string-number))))))

http://codereview.appspot.com/164063/diff/5001/5010#newcode432
scm/translation-functions.scm:432: (make-vcenter-markup
    (make-vcenter-markup
     (let ((fret (- (ly:pitch-semitones pitch)
                    (list-ref tuning (1- string-number)))))
       (number->string (cond
                        ((and (> fret 0) (= string-number 5))
                         (+ fret 5))
                        (else fret)))))))

http://codereview.appspot.com/164063