Re: Make tab-note-heads and fretboards use common code. (issue186268)
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
http://codereview.appspot.com/186268/show
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
On 1/30/10 11:44 AM, n.putt...@gmail.com n.putt...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
The patch set now seems to be complete. I've responded to Neil's comments, and regression tests all pass. Please review. Thanks, Carl http://codereview.appspot.com/186268/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
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.
http://codereview.appspot.com/186268/show
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
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 http://codereview.appspot.com/186268/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
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 http://codereview.appspot.com/186268/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
2010/1/26 Carl Sorensen c_soren...@byu.edu: Should this be a separate patch, probably applied before the current patch? Definitely a separate patch, though I think it can wait until this patch has been sorted. 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). Sorry, I must've been completely befuddled last night; I was thinking of scm_list_5/scm_list_n, which are obviously no use in this situation. Yes, I found and fixed those. This question was about lines 235-239 Oops. :) (define placement-list- Convert . (map (lambda (x) (...) (filter ((lambda (l) ...) placement-list That indentation seems to me to be correct. The docstring and body are out by one space, (same for the docstring in get-predefined-fretboard). Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
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 http://codereview.appspot.com/186268/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make tab-note-heads and fretboards use common code. (issue186268)
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: vectorStream_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
