Re: LSR contribution

[Thomas Morley]

Hi Michael,

Am Fr., 27. Dez. 2019 um 16:12 Uhr schrieb Michael Käppler <address@hidden>:
>
> Hi Harm,
> thanks for your comments and this inspiring discussion!

me too!

> I would like to discuss a couple of things further.
> My current version is attached.

> > 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.

ok

> > 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.

Iiuc, you want to keep the possibility the user defines the intervaldefs like
#(define intervaldefs
   '(("my-prime++" . (0 . 1))
     ...)

Fine with me, this should be mentioned in the description/comments,
probably like

%% Interval definitions alist
%% Key:
%% number-string determines the interval type, 1=prime, 2=second, 3=third ...
%% plus and minus signs determine variant, no sign=perfect interval, +=major,
%% ++=augmented, -=minor, --=diminished

%% Other strings for the interval-type are possible as well, if given
to the engraver in the same way.
(Bad wording, you may get the point, though ... hopefully)

Btw, I'd change

%% intervals-given: list of the form
%%   #`((interval1 ,dir1 enh1 ,color1)
%%      (interval2 ,dir2 enh2 ,color2)
%%      ...
%%      (intervalN ,dirN enhN ,colorN))
%% with
%% intervaln: string - specifying the interval to search after
%% dirn: integer - UP (=1) DOWN (=-1) or 0 (up and down)
%% enhn: boolean - search for enharmonically equivalent intervals, too?
%% colorn: lilypond color value, see NR A.7.

to always use capital N for
intervalN etc

> > 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.

I think we should go for the current code.
Other hooks should be thoroughly demonstrated, _if_ they are needed to
get the desired result.
In other words, demonstrating process-acknowledged should be left to
another LSR snippet.


> > 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)))

Nice, I'd add a comment about different behaviour of 2.18.2 vs 2.19.x
accepting an empty list as engraver.
You go back to the list-syntax, also possible would be:
(make-engraver ((initialize translator) '()))
I'm undecided here ... you decision ;)

> 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...

Sorry for that, I usually don't care about trailing whitespace.
Well, ... unless I use a git repo myself ...

> > 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* ...)?

Nope

> 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

Indeed, your current code is superior

> 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))
>

Ok

> >             (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

Some other remarks:

the type-check uses ly:dir?, ofcourse it's my own suggestion to use
ly:dir?, though probably worth a comment, because the allowed 0 here
means UP _and_ DOWN as opposed to the usual CENTER.

Some comments exceed the 80-characters line-width.

For some strings you circumvent it by doing (string-append "long
string " "other long string")
I'll not object to do so. Though, I've no problem to simply put those
long strings at line-begin or to decrease indentation.
Even if that means to violate usual indentation-rules.


All things mentioned above are micro-issues, imho, I hope you'll not
tired of me being such a nitpicker.


Thanks,
  Harm