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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
