[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: LSR contribution
From: |
Thomas Morley |
Subject: |
Re: LSR contribution |
Date: |
Sun, 29 Dec 2019 12:11:52 +0100 |
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
- LSR contribution, Michael Käppler, 2019/12/15
- Re: LSR contribution, Thomas Morley, 2019/12/15
- Re: LSR contribution, Thomas Morley, 2019/12/15
- Re: LSR contribution, Michael Käppler, 2019/12/18
- Re: LSR contribution, Michael Käppler, 2019/12/20
- Re: LSR contribution, Thomas Morley, 2019/12/21
- Re: LSR contribution, Michael Käppler, 2019/12/27
- Re: LSR contribution,
Thomas Morley <=
- Re: LSR contribution, Thomas Morley, 2019/12/29
- Re: LSR contribution, Michael Käppler, 2019/12/20
Re: LSR contribution, Pierre Perol-Schneider, 2019/12/15