Issue 152600043: Additions in event-listener.ly (Closed)
|
Created:
10 years, 6 months ago by pkx166h Modified:
7 years, 8 months ago CC:
lilypond-devel_gnu.org, phil Hezaine Visibility:
Public. |
DescriptionIssue 4164
Additions in event-listener.ly
Changes applied to DrumVoice, drum-type...
Patch provided by Phil Hezaine
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebased to current master - comment by VV not yet addressed #Patch Set 3 : With Thomas' corrections - thanks Thomas. #
Total comments: 2
Patch Set 4 : Trivial typo fix (thanks Marc) #MessagesTotal messages: 12
Greetings Philippe and James, this looks good overall, I just have one minor comment. https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly File ly/event-listener.ly (right): https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode129 ly/event-listener.ly:129: (+ 60 (ly:pitch-semitones AFAICS, .ly files use spaces, not tabs. Check the indentation. https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode144 ly/event-listener.ly:144: "note" Strictly speaking, this isn't a "note". "Drumnote" may be too long, how about "stroke" or something else?
Sign in to reply to this message.
Rebased to current master - comment by VV not yet addressed
Sign in to reply to this message.
On 2014/10/17 06:40:24, Valentin Villenave wrote: > Greetings Philippe and James, > this looks good overall, I just have one minor comment. > > https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly > File ly/event-listener.ly (right): > > https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode129 > ly/event-listener.ly:129: (+ 60 (ly:pitch-semitones > AFAICS, .ly files use spaces, not tabs. Check the indentation. > > https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode144 > ly/event-listener.ly:144: "note" > Strictly speaking, this isn't a "note". "Drumnote" may be too long, how about > "stroke" or something else? If someone could advise me on the suggestion by Valentine as I doubt the author is going to reply to me and it will be doing the 'edit'.
Sign in to reply to this message.
https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly File ly/event-listener.ly (right): https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode69 ly/event-listener.ly:69: (eq? 0 (ly:moment-grace-numerator moment)) Why this change for the worse? Numbers need to be compared with eqv? rather than eq? and I don't see that anything was wrong with zero? here since ly:moment-grace-numerator certainly cannot return a non-number. https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode144 ly/event-listener.ly:144: "note" On 2014/10/17 06:40:24, Valentin Villenave wrote: > Strictly speaking, this isn't a "note". "Drumnote" may be too long, how about > "stroke" or something else? Well, it's called "drum-type" so what about just "type" here? https://codereview.appspot.com/152600043/diff/1/ly/event-listener.ly#newcode251 ly/event-listener.ly:251: (note-event . format-drumnote) I don't think that having separate engravers for \Voice and \DrumVoice makes a whole lot of sense. Instead, format-drumnote likely should get folded into format-note and the decision for the note format be made on the presence of pitch/drumtype fields.
Sign in to reply to this message.
On 2017/03/22 10:24:21, dak wrote: > ly/event-listener.ly:69: (eq? 0 (ly:moment-grace-numerator moment)) > Why this change for the worse? Numbers need to be compared with eqv? rather > than eq? and I don't see that anything was wrong with zero? here since > ly:moment-grace-numerator certainly cannot return a non-number. I suspect that this arose from "git rebase". You changed this in 2013 in 1c869295b643d256a99de90f67d32b442f6f0586; if the original patch was made from a branch that happened before that, it would of course have the previous (eq? 0 ...). [incidently, the (eq? 0 ... was part of the first version I wrote, in 2011.]
Sign in to reply to this message.
Thanks to David (and Graham). I understand David's comments but don't have the knowledge (or the time) to make the edits that are suggested here - at least from as I understand it there would need to be some kind of 'check/test' (if/then) construction in format-note to decide if it was \Voice or \DrumVoice. At least that is my reading of the comments. Oh well, it didn't hurt to try to resurrect someone's old patch. I'll have to set this back to patch-abandoned then. Thank you again all the same.
Sign in to reply to this message.
On 2017/03/25 10:34:02, pkx166h wrote:
> Thanks to David (and Graham).
>
> I understand David's comments but don't have the knowledge (or the time) to
make
> the edits that are suggested here - at least from as I understand it there
would
> need to be some kind of 'check/test' (if/then) construction in format-note to
> decide if it was \Voice or \DrumVoice.
>
> At least that is my reading of the comments.
>
> Oh well, it didn't hurt to try to resurrect someone's old patch. I'll have to
> set this back to patch-abandoned then.
>
> Thank you again all the same.
Hi James,
sorry for the late reply.
Maybe below will do the trick (diff is against master):
diff --git a/ly/event-listener.ly b/ly/event-listener.ly
index 4627aaa..a062472 100644
--- a/ly/event-listener.ly
+++ b/ly/event-listener.ly
@@ -122,12 +122,18 @@ as an engraver for convenience."
#(define (format-note engraver event)
(let* ((origin (ly:input-file-line-char-column
- (ly:event-property event 'origin))))
+ (ly:event-property event 'origin)))
+ (drum-type (ly:event-property event 'drum-type))
+ (pitch (ly:event-property event 'pitch)))
(print-line engraver
- "note"
- ;; get a MIDI pitch value.
- (+ 60 (ly:pitch-semitones
- (ly:event-property event 'pitch)))
+ (if (ly:pitch? pitch)
+ "note"
+ "type")
+ (if (ly:pitch? pitch)
+ ;; get a MIDI pitch value.
+ (+ 60 (ly:pitch-semitones
+ (ly:event-property event 'pitch)))
+ drum-type)
(ly:duration->string
(ly:event-property event 'duration))
(format-moment (ly:duration-length
@@ -206,23 +212,30 @@ as an engraver for convenience."
%%%% are notified about all notes and rests. We don't create any grobs or
%%%% change any settings.
+#(define event-listener-engraver
+ (make-engraver
+ (listeners
+ (tempo-change-event . format-tempo)
+ (rest-event . format-rest)
+ (note-event . format-note) ;; works for
+ (articulation-event . format-articulation)
+ (text-script-event . format-text)
+ (slur-event . format-slur)
+ (breathing-event . format-breathe)
+ (dynamic-event . format-dynamic)
+ (crescendo-event . format-cresc)
+ (decrescendo-event . format-decresc)
+ (text-span-event . format-textspan)
+ (glissando-event . format-glissando)
+ (tie-event . format-tie))))
+
\layout {
\context {
- \Voice
- \consists #(make-engraver
- (listeners
- (tempo-change-event . format-tempo)
- (rest-event . format-rest)
- (note-event . format-note)
- (articulation-event . format-articulation)
- (text-script-event . format-text)
- (slur-event . format-slur)
- (breathing-event . format-breathe)
- (dynamic-event . format-dynamic)
- (crescendo-event . format-cresc)
- (decrescendo-event . format-decresc)
- (text-span-event . format-textspan)
- (glissando-event . format-glissando)
- (tie-event . format-tie)))
+ \Voice
+ \consists #event-listener-engraver
+ }
+ \context {
+ \DrumVoice
+ \consists #event-listener-engraver
}
}
Sign in to reply to this message.
With Thomas' corrections - thanks Thomas.
Sign in to reply to this message.
Just a nitpick ... https://codereview.appspot.com/152600043/diff/40001/ly/event-listener.ly File ly/event-listener.ly (right): https://codereview.appspot.com/152600043/diff/40001/ly/event-listener.ly#newc... ly/event-listener.ly:220: (note-event . format-note) ;; works for The "works for" comment seems to be cut off. Perhaps "works for notes and drum notes"?
Sign in to reply to this message.
Trivial typo fix (thanks Marc)
Sign in to reply to this message.
https://codereview.appspot.com/152600043/diff/40001/ly/event-listener.ly File ly/event-listener.ly (right): https://codereview.appspot.com/152600043/diff/40001/ly/event-listener.ly#newc... ly/event-listener.ly:220: (note-event . format-note) ;; works for On 2017/08/14 20:36:07, marc wrote: > The "works for" comment seems to be cut off. > Perhaps "works for notes and drum notes"? done.
Sign in to reply to this message.
author Thomas Morley <thomasmorley65@gmail.com> Sat, 12 Aug 2017 10:34:42 +0100 (10:34 +0100) committer James Lowe <pkx166h@gmail.com> Tue, 22 Aug 2017 10:04:17 +0100 (10:04 +0100) commit f0781fa82140814e371275e84802b3648970477a
Sign in to reply to this message.
|
