Modify chord-name-engraver to call capo-handler

Modify chord-name-engraver to call capo-handler


Add capo properties to define-context-properties


Add capo-handler function for guitar chords

[Janek Warchol, 2011-07-25 21:31:22]

New patch from Wol uploaded.  According to Carl, it addresses previous concerns;
we only have a problem with Scheme syntax left.
(don't run regtests on this yet)

[Janek Warchol, 2011-07-25 22:04:56]

Uploaded Carl's fix, everything works now!
I'm making from scratch and will run regtests.

[Carl, 2011-07-25 22:21:23]

Wol,

Please fix the indentation, and make a new patch so that you get credit for the
commit.

Thanks,

Carl

http://codereview.appspot.com/4800051/diff/3001/scm/chord-name.scm
File scm/chord-name.scm (right):

http://codereview.appspot.com/4800051/diff/3001/scm/chord-name.scm#newcode180
scm/chord-name.scm:180: (map (lambda (p)
Indent one space

http://codereview.appspot.com/4800051/diff/3001/scm/chord-name.scm#newcode182
scm/chord-name.scm:182: pitches))
This will need to be indented to match up with (lambda

http://codereview.appspot.com/4800051/diff/3001/scm/chord-name.scm#newcode195
scm/chord-name.scm:195: (capo-vertical (ly:context-property context
'capoVertical #f)))
Indent one space

[Carl, 2011-07-25 22:21:23]

Wol,

Please fix the indentation, and make a new patch so that you get credit for the
commit.

Thanks,

Carl

[antlists_youngman.org.uk, 2011-07-29 22:02:14]

On 25/07/11 23:21, Carl.D.Sorensen@gmail.com wrote:
> Wol,

Hi Carl
> 
> Please fix the indentation, and make a new patch so that you get credit
> for the commit.
> 
> Thanks,
> 
> Carl
> 
New patch attached. I've taken your patch, applied it, fixed the
indentation (I hope :-) and created the new one.

Sorry I haven't actually tested it on my system, but make is crashing at
the moment - there seems to be a snafu in the documentation somewhere
:-( That said, all I've done is fix indenting so if it worked for you it
shouldn't break now ...

I'll post an updated docu patch as soon as I can get my build to work -
basically taking my original snippet, and making it print two lots of
chords, the first set horizontal, the second vertical.

Cheers,
Wol

[Janek Warchol, 2011-07-30 17:23:16]

New patch from Wol uploaded.

[Carl, 2011-07-30 20:30:24]

LGTM, but we need a regtest before pushing.

[pkx166h, 2011-07-30 21:49:54]

Doesn't pass a make check.

I get an error on

regression/fret-diagrams-string-thickness.ly

log file shows:

Renaming input to:
`/home/jlowe/lilypond-git/input/regression/fret-diagrams-string-thickness.ly'
Interpreting music... ERROR: In procedure ly:pitch-transpose:
ERROR: Wrong type argument in position 2 (expecting Pitch): ()

[antlists_youngman.org.uk, 2011-07-30 22:52:48]

On 30/07/11 22:49, pkx166h@gmail.com wrote:
> Doesn't pass a make check.
> 
> I get an error on
> 
> regression/fret-diagrams-string-thickness.ly
> 
> log file shows:
> 
> Renaming input to:
> `/home/jlowe/lilypond-git/input/regression/fret-diagrams-string-thickness.ly'
> 
> Interpreting music... ERROR: In procedure ly:pitch-transpose:
> ERROR: Wrong type argument in position 2 (expecting Pitch): ()
> 
> http://codereview.appspot.com/4800051/
> 
I've just modified my sample code to print straight chords at the start,
and it now fails. So it looks like the problem is in the trap that says
"no transpose, just fall straight through to the formatter like it did
before".

Bad error to miss ... if anybody can help me with the necessary scheme...

Cheers,
Wol

[dak, 2011-07-30 23:01:32]

http://codereview.appspot.com/4800051/diff/10001/scm/chord-name.scm
File scm/chord-name.scm (right):

http://codereview.appspot.com/4800051/diff/10001/scm/chord-name.scm#newcode177
scm/chord-name.scm:177: (if (not capo-pitch)
Seems like a Lispism: '() evaluates as true.  You probably should rather write
#f.

[antlists_youngman.org.uk, 2011-07-30 23:15:30]

On 30/07/11 23:52, Wols Lists wrote:
> On 30/07/11 22:49, pkx166h@gmail.com wrote:
>> Doesn't pass a make check.
>>
>> I get an error on
>>
>> regression/fret-diagrams-string-thickness.ly
>>
>> log file shows:
>>
>> Renaming input to:
>> `/home/jlowe/lilypond-git/input/regression/fret-diagrams-string-thickness.ly'
>>
>> Interpreting music... ERROR: In procedure ly:pitch-transpose:
>> ERROR: Wrong type argument in position 2 (expecting Pitch): ()
>>
>> http://codereview.appspot.com/4800051/
>>
> I've just modified my sample code to print straight chords at the start,
> and it now fails. So it looks like the problem is in the trap that says
> "no transpose, just fall straight through to the formatter like it did
> before".
> 
> Bad error to miss ... if anybody can help me with the necessary scheme...
> 
Following up to my own post ... the relevant snippet of code is

+        (capo-pitch (ly:context-property context 'capoPitch '())))
+    (if (not capo-pitch)
+        (chord-function pitches bass inversion context)  ;; call the
chordNameFunction as of old
+        (let* ((new-pitches   ;; else transpose the pitches and do the
chord twice

What does ly:context-property return if capoPitch hasn't been set? If
I've got it right, I've told it to return '() as the default, and then
I'm testing for #f. Oops. How do I fix that? Either get context-property
to return #f (I've tried just swapping '() for #f, and it doesn't seem
to work) or change the if to test for '()?

Cheers,
Wol

[c_sorensen, 2011-07-30 23:22:47]

On 7/30/11 5:15 PM, "Wols Lists" <antlists@youngman.org.uk> wrote:

> On 30/07/11 23:52, Wols Lists wrote:
>> On 30/07/11 22:49, pkx166h@gmail.com wrote:
>>> Doesn't pass a make check.
>>> 
>>> I get an error on
>>> 
>>> regression/fret-diagrams-string-thickness.ly
>>> 
>>> log file shows:
>>> 
>>> Renaming input to:
>>> 
`/home/jlowe/lilypond-git/input/regression/fret-diagrams-string-thickness.ly>>>
'
>>> 
>>> Interpreting music... ERROR: In procedure ly:pitch-transpose:
>>> ERROR: Wrong type argument in position 2 (expecting Pitch): ()
>>> 
>>> http://codereview.appspot.com/4800051/
>>> 
>> I've just modified my sample code to print straight chords at the start,
>> and it now fails. So it looks like the problem is in the trap that says
>> "no transpose, just fall straight through to the formatter like it did
>> before".
>> 
>> Bad error to miss ... if anybody can help me with the necessary scheme...
>> 
> Following up to my own post ... the relevant snippet of code is
> 
> +        (capo-pitch (ly:context-property context 'capoPitch '())))
> +    (if (not capo-pitch)
> +        (chord-function pitches bass inversion context)  ;; call the
> chordNameFunction as of old
> +        (let* ((new-pitches   ;; else transpose the pitches and do the
> chord twice
> 
> What does ly:context-property return if capoPitch hasn't been set? If
> I've got it right, I've told it to return '() as the default, and then
> I'm testing for #f. Oops. How do I fix that? Either get context-property
> to return #f (I've tried just swapping '() for #f, and it doesn't seem
> to work) or change the if to test for '()?

It returns '() (because you've told it to do so).

Change the if to test for

if (null? capoPitch)

HTH,

Carl

[antlists_youngman.org.uk, 2011-07-30 23:35:41]

On 31/07/11 00:22, Carl Sorensen wrote:
> 
> 
> 
> On 7/30/11 5:15 PM, "Wols Lists" <antlists@youngman.org.uk> wrote:
> 
>> On 30/07/11 23:52, Wols Lists wrote:
>>> On 30/07/11 22:49, pkx166h@gmail.com wrote:
>>>> Doesn't pass a make check.
>>>>
>>>> I get an error on
>>>>
>>>> regression/fret-diagrams-string-thickness.ly
>>>>
>>>> log file shows:
>>>>
>>>> Renaming input to:
>>>>
>
`/home/jlowe/lilypond-git/input/regression/fret-diagrams-string-thickness.ly>>>
> '
>>>>
>>>> Interpreting music... ERROR: In procedure ly:pitch-transpose:
>>>> ERROR: Wrong type argument in position 2 (expecting Pitch): ()
>>>>
>>>> http://codereview.appspot.com/4800051/
>>>>
>>> I've just modified my sample code to print straight chords at the start,
>>> and it now fails. So it looks like the problem is in the trap that says
>>> "no transpose, just fall straight through to the formatter like it did
>>> before".
>>>
>>> Bad error to miss ... if anybody can help me with the necessary scheme...
>>>
>> Following up to my own post ... the relevant snippet of code is
>>
>> +        (capo-pitch (ly:context-property context 'capoPitch '())))
>> +    (if (not capo-pitch)
>> +        (chord-function pitches bass inversion context)  ;; call the
>> chordNameFunction as of old
>> +        (let* ((new-pitches   ;; else transpose the pitches and do the
>> chord twice
>>
>> What does ly:context-property return if capoPitch hasn't been set? If
>> I've got it right, I've told it to return '() as the default, and then
>> I'm testing for #f. Oops. How do I fix that? Either get context-property
>> to return #f (I've tried just swapping '() for #f, and it doesn't seem
>> to work) or change the if to test for '()?
> 
> It returns '() (because you've told it to do so).
> 
> Change the if to test for
> 
> if (null? capoPitch)
> 
> HTH,
> 
It helps a lot :-)

Revised patch attached ...

Cheers,
Wol

[Janek Warchol, 2011-07-31 10:35:46]

New patch uploaded. Passes regtests made from scratch.

[Janek Warchol, 2011-08-01 05:17:37]

New patch set uploaded (adding a regtest).

2011/8/1 Wols Lists <antlists@youngman.org.uk>:
> Regression test attached. I've looked at the other regression tests and
> tried to make it similar. 

Great!

http://codereview.appspot.com/4800051/diff/14003/input/regression/chord-capo.ly
File input/regression/chord-capo.ly (right):

http://codereview.appspot.com/4800051/diff/14003/input/regression/chord-capo....
input/regression/chord-capo.ly:4: texidoc="Properties capoPitch, capoVertical:
display chordnames, suitably
trailing whitespace here

[pkx166h, 2011-08-01 07:34:25]

Passes Make and reg tests

[Janek Warchol, 2011-08-02 19:59:06]

http://codereview.appspot.com/4800051/diff/12002/input/regression/chord-capo.ly
File input/regression/chord-capo.ly (right):

http://codereview.appspot.com/4800051/diff/12002/input/regression/chord-capo....
input/regression/chord-capo.ly:12: g1
I would change all g major chords into a minor chords.

[Neil Puttock, 2011-08-02 20:24:15]

http://codereview.appspot.com/4800051/diff/12002/Documentation/notation/chord...
File Documentation/notation/chords.itely (right):

http://codereview.appspot.com/4800051/diff/12002/Documentation/notation/chord...
Documentation/notation/chords.itely:516: In make-pitch, leave the first argument
at 0, the second argument is the
A bit of syntactic sugar would OK here.  Since it's now possible to pass pitches
as arguments to music functions, it would be nice to have a \capo function which
doesn't require users to wrestle with ly:make-pitch.

http://codereview.appspot.com/4800051/diff/12002/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/4800051/diff/12002/lily/chord-name-engraver.cc#...
lily/chord-name-engraver.cc:127: SCM capo_proc = ly_lily_module_constant
("capo-handler");
While this gets round the duplication in formatter functions, it hides the
actual context property which does the formatting (and you won't find another
instance of ly_lily_module_constant () used in this way).  I still think the
minor duplication is preferable.

[antlists_youngman.org.uk, 2011-08-02 21:02:25]

On 02/08/11 21:24, n.puttock@gmail.com wrote:
> 
>
http://codereview.appspot.com/4800051/diff/12002/Documentation/notation/chord...
> 
> File Documentation/notation/chords.itely (right):
> 
>
http://codereview.appspot.com/4800051/diff/12002/Documentation/notation/chord...
> 
> Documentation/notation/chords.itely:516: In make-pitch, leave the first
> argument at 0, the second argument is the
> A bit of syntactic sugar would OK here.  Since it's now possible to pass
> pitches as arguments to music functions, it would be nice to have a
> \capo function which doesn't require users to wrestle with
> ly:make-pitch.
> 
That's already been posted to lily-frogs for review, if you would care
to take a look. It contains a bit too much pseudo-code and "how do I do
this" comments for my liking, which is why I didn't want to post it
anywhere else ... :-)

Basically, the idea is you put "\capoKey A" after "\key C \major" or
whatever, and it does the rest, working out the transposition, the fret,
and printing the appropriate text, ie "Capo 3 (A)" at the start of the
score.

> http://codereview.appspot.com/4800051/diff/12002/lily/chord-name-engraver.cc
> 
> File lily/chord-name-engraver.cc (right):
> 
>
http://codereview.appspot.com/4800051/diff/12002/lily/chord-name-engraver.cc#...
> 
> lily/chord-name-engraver.cc:127: SCM capo_proc = ly_lily_module_constant
> ("capo-handler");
> While this gets round the duplication in formatter functions, it hides
> the actual context property which does the formatting (and you won't
> find another instance of ly_lily_module_constant () used in this way).
> I still think the minor duplication is preferable.

:-(

And if somebody creates a new chord-formatter and forgets it, some poor
user is going to get bitten by the mysterious failure of the capo
function :-(
> 
> http://codereview.appspot.com/4800051/
> 
Cheers,
Wol

[janek, 2011-08-22 15:20:23]

Hi,

after disappearing for some time (sorry for that) i'm reading my inbox
and found that guitar capo transposer isn't finished!
Wol, how can i help you?  Would you like me to investigate the scheme
function about which you wrote on frogs list ("After capo handler
comes capo key")?
Maybe it would be good to move that discussion to -devel - not many
people watch Frogs list.

cheers,
Janek