Hi Michael, Am Fr., 27. Dez. 2019 um 16:12 Uhr schrieb Michael Käppler <[email protected]>: > > 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
