Re: LSR contribution

[Michael Käppler]

Hi Harm,
thanks for your comments and this inspiring discussion!
I would like to discuss a couple of things further.
My current version is attached.

> You try to give the user always meaningful warning-messages.
> That's great, putting out really helpful messages is hard work...
> Alas, speaking only for me, I don't like those multiple nested `if`.
> Thus I defined `type-check-intervals-given` and use it to filter the
> user-given interval-list.
> `filter` will return false for the first occurrence of failed
> `type-check-intervals-given`. Thus it can be used to deal with
> user-errors step-by-step.
Nice solution!
> Along with it, I added a basic check for the user provided list (about
> equal length of each sublist)

A good addition, but it did not work as intended, because the dissection
of the list with (car) (second) etc. took place regardless of whether
the interval was well-formed. See the comment in the code below.
I would not use ly:error here, because that will
terminate the compilation process, right?  It may be that
some intervals are well-formed and some are not. I think we should
still go on and process the well-formed intervals.
Otherwise we should raise an error for the other fault conditions
(Direction, enharmonic, color, missing interval in definitions) too,
what I do not think is appropriate.

There was an undefined variable `gen-warntext`, which is now gone as well.

Sorry for that mistake, which reminds me of always thoroughly testing
my code... :(
> Furthermore, I changed the basic `intervaldefs` to take only pairs of
> the interval-string and the semi-tonoc steps. The diatonic steps are
> calculated relying on the interval-string.
I have to admit that I'm not happy with this change.
I think the user should be able to use custom
interval denotations like e.g.
https://en.wikipedia.org/wiki/Interval_(music)#Alternative_interval_naming_conventions
rather than having to rely on a hardcoded system.
> I found no need to do work in `process-acknowledged`.
> Thus all work is done in 'note-head-interface of `acknowledgers`
> Probably more efficient, but I have not really checked.
I think it is definitily more efficient, since process-acknowledged is 
called multiple times after
one grob has been acknowledged by the engraver. The question is to which 
extent
the "educational" idea of showing the various hooks in action justifies 
this overhead.

> A plethora of minor changes in code and comments... ;)
>
> WDYT?
>
> Btw, there is one case, where I don't know how to deal with:
> 2.18.2 can't cope with an empty engraver, see:
>
> \score {
>    \new Staff \relative c' { c4 d }
>    \layout {
>      \context {
>        \Voice
>        \consists \color_interval_engraver #intervaldefs #`(("30-" 0 #t ,green))
>      }
>    }
> }
>
> No problem for 2.19.83, though.
Oh no, further insufficient testing of mine. The following minimal 
"void" engraver
works for me with both 2.18.2 and 2.19.80:
`((initialize . ,(lambda (translator) #t)))

I'm commenting now directly in your code, mentioning only thoughts
that I did not mention before. Btw, your code had pretty much
lines with trailing whitespace which I removed, because I work here
on a local git repo and the diffs become cluttered otherwise...

> #(use-modules (ice-9 pretty-print))
Removed, since unused.
> color_interval_engraver =
> #(define-scheme-function (parser location intervaldefs debug? 
> intervals-given)
>    (list? (boolean?) list?) ;; debug? is optional, defaults to #f
>
>   (define (string-diatonic-semi-tonic-list string-semi-tonic-list)
>    (map
>      (lambda (e)
>        (let* ((interval-string
>                 (string-trim-both
>                   (car e)
>                   (lambda (c) (or (eqv? c #\+) (eqv? c #\-)))))
>               (interval-diatonic
>                 (string->number interval-string)))
>          (cons (car e) (cons (1- interval-diatonic) (cdr e)))))
>      string-semi-tonic-list))
>
>   (define (type-check-intervals-given msg-header)
Is there a reason for not defining this as a binding
in the following (let* ...)?
No need to explicitly pass msg-header, then.
>     (lambda (interval)
>       ;; basic check for amount of args
>       (if (= 4 (length interval))
>           #t
>           (begin
>             (ly:error
>               "~a Interval ~a must have 4 entries" msg-header interval)
>             #f))
Here is a bug - if the check does not succeed,
the function will not return with #f but instead
go on with the (let) construct.
>       ;; check every entry for type, additonally the first entry 
> whether it's
>       ;; a key in intervaldefs
>       (let ((name (car interval))
>             (dir (second interval))
>             (enh? (third interval))
>             (color (fourth interval)))
>         (and
>           ;; check first entry for string? and whether it's in 
> intervaldefs
>           (if (and (string? name) (assoc-get name intervaldefs))
>               #t
>               (begin
>                 (ly:warning
>                   "~a In interval ~a, ~a not found in interval 
> definitions"
>                   msg-header
>                   interval
>                   (car interval))
>                 #f))
>           ;; check second entry for ly:dir?
>           (if (ly:dir? dir)
>               #t
>               (begin
>                 (ly:warning
>           "~a In interval ~a, wrong type argument: ~a, needs to be a 
> direction."
>                   msg-header
>                   interval
>                   dir)
>                 #f))
>           ;; check third entry for boolean?
>           (if (boolean? enh?)
>               #t
>               (begin
>                 (ly:warning
>             "~a In interval ~a, wrong type argument: ~a, needs to be a 
> boolean."
>                   msg-header
>                   interval
>                   enh?)
>                 #f))
>           ;; check fourth entry for color?
>           (if (color? color)
>               #t
>               (begin
>                 (ly:warning
>               "~a In interval ~a, wrong type argument: ~a, needs to be 
> a color."
>                   msg-header
>                   interval
>                   color)
>                 #f))))))
>
>    (let* ((msg-header "Color_interval_engraver:")
>           (interval-defs-list (string-diatonic-semi-tonic-list 
> intervaldefs))
>           (cleaned-intervals-given
>             (filter (type-check-intervals-given msg-header) 
> intervals-given))
>           (search-intervals
>             ;; mmh, not sure if `reverse` is really needed
It is not needed, because the order of checking the intervals does not 
matter.
(It would only matter if two conflicting interval colors are given, like
\consists \color_interval_engraver #intervaldefs
        #`(("3--" 0 #f ,green)
           ("3--" 0 #f ,yellow))

>             (reverse
>               (map
>                 (lambda (interval)
>                   (let ((diatonic-semitonic-pair
>                           (assoc-get (car interval) interval-defs-list)))
>                     (cons diatonic-semitonic-pair (cdr interval))))
>                 cleaned-intervals-given))))
[Rest skipped]

Cheers,
Michael

interval-color-engraver-lsr.ly
Description: Text Data