Additions in event-listener.ly

Additions in event-listener.ly

Changes applied to DrumVoice, drum-type...

[phil Hezaine, 2013-03-29 18:40:42]

Changes for DrumVoice, drum-type in event-listener.ly

[janek, 2013-03-29 19:21:31]

Hi Phil,

thanks for uploading your patch! There are some minor style improvements needed.

Janek

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly
File ly/event-listener.ly (right):

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly#newcode69
ly/event-listener.ly:69: (eq? 0 (ly:moment-grace-numerator moment))
Out of curiosity (my knowledge about Scheme is poor): what's the difference
between 'zero?' and 'eq? 0' ?
Is 'eq? 0'? better in some way?

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly#newcode140
ly/event-listener.ly:140: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% ADDED
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
please remove %%%%%%%%%ADDED%%%%%%%%%%% lines, here and below.

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly#newcode148
ly/event-listener.ly:148: ;; but with a DrumVoice I get:
I understand that the lines below are about some problem that you have with the
code.  In that case, please mark it clearly with 'TODO'.

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly#newcode295
ly/event-listener.ly:295: ; Added: chord-event
please remove these '; Added: ...' comments as well.

[dak, 2013-03-29 19:23:49]

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly
File ly/event-listener.ly (right):

https://codereview.appspot.com/8165043/diff/2001/ly/event-listener.ly#newcode69
ly/event-listener.ly:69: (eq? 0 (ly:moment-grace-numerator moment))
On 2013/03/29 19:21:31, janek wrote:
> Out of curiosity (my knowledge about Scheme is poor): what's the difference
> between 'zero?' and 'eq? 0' ?
> Is 'eq? 0'? better in some way?

eq? 0 will not complain if the compared value is not a number.  It also is
undefined code since (eq? 0 0) is not guaranteed by the Scheme standard.  Better
use zero? here.

[phil Hezaine, 2013-03-29 21:30:33]

Style improvements needed.

[Graham Percival, 2013-03-29 23:30:19]

https://codereview.appspot.com/8165043/diff/1002/ly/event-listener.ly
File ly/event-listener.ly (right):

https://codereview.appspot.com/8165043/diff/1002/ly/event-listener.ly#newcode140
ly/event-listener.ly:140: #(define (format-drumnote engraver event)
Why is this a separate function?  can't it be done by the normal (format-note
...) function?

https://codereview.appspot.com/8165043/diff/1002/ly/event-listener.ly#newcode246
ly/event-listener.ly:246: #(define (format-dynamic engraver event-chord)
This looks identical to the above (format-dynamic...) ?

[phil Hezaine, 2013-04-01 19:35:04]

Ameliorations in the code.

[phil Hezaine, 2013-04-01 19:58:34]

261 (dynamic-event-chord . format-dynamic)
Ah! Zut! I forget to delete this line...

281 (dynamic-event-chord . format-dynamic)
... and also this line!

[Graham Percival, 2013-04-01 20:58:40]

https://codereview.appspot.com/8165043/diff/13001/ly/event-listener.ly
File ly/event-listener.ly (right):

https://codereview.appspot.com/8165043/diff/13001/ly/event-listener.ly#newcod...
ly/event-listener.ly:127: (ly:event-property event-chord 'pitch)))
Are you sure that event-chord is defined?

https://codereview.appspot.com/8165043/diff/13001/ly/event-listener.ly#newcod...
ly/event-listener.ly:139: % Then I don't find how to put together
format-drumnote and
I think you'll need to figure out if an event contains a 'drum-type or not.  If
it does, then extract it; if not, then extract the 'pitch.

As for (expecting Pitch): I suggest adding a
  (display event)
to the top of this function, then look at the output you get when you run a
simple test case containing 1 note or 1 drumnote.  That should help to narrow
down the problem.

https://codereview.appspot.com/8165043/diff/13001/ly/event-listener.ly#newcod...
ly/event-listener.ly:219: (ly:event-property event-chord 'text))))
ditto, where is event-chord defined?

[phil Hezaine, 2013-04-09 19:08:56]

only add drum-note and drum-name

[pkx166h, 2014-10-15 14:08:51]

On 2013/04/09 19:08:56, phil Hezaine wrote:
> only add drum-note and drum-name

This has now been moved to https://codereview.appspot.com/152600043 for tracker
issue http://code.google.com/p/lilypond/issues/detail?id=4164

someone asked for this on the user lists, and I saw that it had never been
implemented in the main code, so have downloaded the patch and it still applies
to current master.

It is undergoing testing now.

Thanks

Jameds