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

[Trevor Daniels]

Thanks Neil

Sorry about the trailing spaces.  I was used to my Windows editor 
removing
them automatically; gedit can't, AFAICS.  However, I've just 
discovered that
git rebase --warning=fix will remove them, so as long as I remember 
to do
that they should be gone.

I think I've corrected the formatting errors as you suggested. 
Could you please
explain when I should use a 1-space indent and when 2-space, please, 
otherwise
this seems just arbitrary.  I do find a 1-space indent makes it more 
difficult to
see the alignments when the lines are some distance apart.

So is http://codereview.appspot.com/164063 now ready to go?

I'll amend the docs after pushing.

Trevor

----- Original Message ----- 
From: <address@hidden>
To: <address@hidden>; <address@hidden>
Cc: <address@hidden>; <address@hidden>
Sent: Sunday, December 13, 2009 7:54 PM
Subject: Re: Add option to indicate frets by letters in tablature 
(issue164063)


> 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


--------------------------------------------------------------------------------


> _______________________________________________
> lilypond-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/lilypond-devel