LSR contribution

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

LSR contribution

Michael Käppler-2
Hi all,
a few days ago I submitted a snippet to the LSR (title "Coloring
successive intervals"). I can see it in the snippet database,
but not in the webpage. The "Contributing" section of LSR states, that:

"Once the snippet is in, it has to be reviewed and approved by one of
the LSR editors,
and then it must be digested by the search engine. Within a few days,
you should be able to see your snippet online."

Is this still valid in principle? Or maybe did I something wrong?

Cheers,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
Am So., 15. Dez. 2019 um 21:17 Uhr schrieb Michael Käppler <[hidden email]>:

>
> Hi all,
> a few days ago I submitted a snippet to the LSR (title "Coloring
> successive intervals"). I can see it in the snippet database,
> but not in the webpage. The "Contributing" section of LSR states, that:
>
> "Once the snippet is in, it has to be reviewed and approved by one of
> the LSR editors,
> and then it must be digested by the search engine. Within a few days,
> you should be able to see your snippet online."
>
> Is this still valid in principle? Or maybe did I something wrong?
>
> Cheers,
> Michael

In principle ... yes.
Though, I seem to be the only remaining regular active LSR editor, and
my time is limited.
Thus it may take some more time than the LSR "Contributing" section says.
Announcing it on the list helps, ofcourse.

I'll take a look soon.


Cheers,
  Harm

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Schneidy
In reply to this post by Michael Käppler-2
Hi Michael,
Cheers,
Pierre

Le dim. 15 déc. 2019 à 21:18, Michael Käppler <[hidden email]> a écrit :
Hi all,
a few days ago I submitted a snippet to the LSR (title "Coloring
successive intervals"). I can see it in the snippet database,
but not in the webpage. The "Contributing" section of LSR states, that:

"Once the snippet is in, it has to be reviewed and approved by one of
the LSR editors,
and then it must be digested by the search engine. Within a few days,
you should be able to see your snippet online."

Is this still valid in principle? Or maybe did I something wrong?

Cheers,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
In reply to this post by Thomas Morley-2
Am So., 15. Dez. 2019 um 21:47 Uhr schrieb Thomas Morley
<[hidden email]>:

>
> Am So., 15. Dez. 2019 um 21:17 Uhr schrieb Michael Käppler <[hidden email]>:
> >
> > Hi all,
> > a few days ago I submitted a snippet to the LSR (title "Coloring
> > successive intervals"). I can see it in the snippet database,
> > but not in the webpage. The "Contributing" section of LSR states, that:
> >
> > "Once the snippet is in, it has to be reviewed and approved by one of
> > the LSR editors,
> > and then it must be digested by the search engine. Within a few days,
> > you should be able to see your snippet online."
> >
> > Is this still valid in principle? Or maybe did I something wrong?
> >
> > Cheers,
> > Michael
>
> In principle ... yes.
> Though, I seem to be the only remaining regular active LSR editor, and
> my time is limited.
> Thus it may take some more time than the LSR "Contributing" section says.
> Announcing it on the list helps, ofcourse.
>
> I'll take a look soon.
As promised I had a look.

Many thanks for your snippet!

For now:

First I changed the LSR button "Large snippet" to "Standalone snippet".
It's a very rare case "Large snippet" is apppropriate, usualy for
snippets outputting multipe pages. In almost every other case it's
better to uncheck the buttons or to go for "Standalone snippet".
Otherwise compressed and (imho) ugly images are output by the LSR.

Please always observe a 80-characters line-width limit for your code.

Your snippet contains some advanced code. In such cases I often want
to discuss things a bit deeper. So:

What bugged me right from the first glance over it, is your need to
define several engravers, one for each case.
I think one engraver should do the work for _all_ those cases.

Attached you'll find my suggestion for this, along with better
indentation, 80-chars-line-width along with some minor adjustments.

Please have a look.

Nevertheless that can't be the final state, imho.
May I ask you to add inline code-comments what's done and why?
There are not so many examples of scheme-engravers around. One
thoroughly commented would be great.
Additionally, I'd go for more self-explaining variable-names.
P.e. "dt-st": for me it's an arbitrary collection of characters, with
a hyphen somewhere ;)

Thanks,
  Harm

atest-95.ly (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
Hi Harm,
many thanks for your comments and for the time you spent
with doing work that I should have been done.
I already thought about adjusting the engraver to take several intervals
into account,
but left it for simplicity (of the engraver, not of the examples ;)) But
you are absolutely right
that this would not be a good solution.

I'm currently working on it, taking a slightly different approach and
will send it (hopefully in
the next days) for another review.

Cheers,
Michael

Am 16.12.2019 um 01:50 schrieb Thomas Morley:

> Am So., 15. Dez. 2019 um 21:47 Uhr schrieb Thomas Morley
> <[hidden email]>:
>> Am So., 15. Dez. 2019 um 21:17 Uhr schrieb Michael Käppler <[hidden email]>:
>>> Hi all,
>>> a few days ago I submitted a snippet to the LSR (title "Coloring
>>> successive intervals"). I can see it in the snippet database,
>>> but not in the webpage. The "Contributing" section of LSR states, that:
>>>
>>> "Once the snippet is in, it has to be reviewed and approved by one of
>>> the LSR editors,
>>> and then it must be digested by the search engine. Within a few days,
>>> you should be able to see your snippet online."
>>>
>>> Is this still valid in principle? Or maybe did I something wrong?
>>>
>>> Cheers,
>>> Michael
>> In principle ... yes.
>> Though, I seem to be the only remaining regular active LSR editor, and
>> my time is limited.
>> Thus it may take some more time than the LSR "Contributing" section says.
>> Announcing it on the list helps, ofcourse.
>>
>> I'll take a look soon.
> As promised I had a look.
>
> Many thanks for your snippet!
>
> For now:
>
> First I changed the LSR button "Large snippet" to "Standalone snippet".
> It's a very rare case "Large snippet" is apppropriate, usualy for
> snippets outputting multipe pages. In almost every other case it's
> better to uncheck the buttons or to go for "Standalone snippet".
> Otherwise compressed and (imho) ugly images are output by the LSR.
>
> Please always observe a 80-characters line-width limit for your code.
>
> Your snippet contains some advanced code. In such cases I often want
> to discuss things a bit deeper. So:
>
> What bugged me right from the first glance over it, is your need to
> define several engravers, one for each case.
> I think one engraver should do the work for _all_ those cases.
>
> Attached you'll find my suggestion for this, along with better
> indentation, 80-chars-line-width along with some minor adjustments.
>
> Please have a look.
>
> Nevertheless that can't be the final state, imho.
> May I ask you to add inline code-comments what's done and why?
> There are not so many examples of scheme-engravers around. One
> thoroughly commented would be great.
> Additionally, I'd go for more self-explaining variable-names.
> P.e. "dt-st": for me it's an arbitrary collection of characters, with
> a hyphen somewhere ;)
>
> Thanks,
>    Harm


Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
In reply to this post by Thomas Morley-2
Hi Harm et al.,
attached is my updated version.
I decided to split up the validity checks from the actual engraver,
because otherwise in case of invalid parameters the engraver would
only be instantiated to do nothing and nevertheless have its
acknowledger called every time.
The code is much more verbose now, which makes the flow of information
and control
clearer I hope.

Please let me know what you think about it.

Cheers,
Michael

Am 16.12.2019 um 01:50 schrieb Thomas Morley:

> Am So., 15. Dez. 2019 um 21:47 Uhr schrieb Thomas Morley
> <[hidden email]>:
>> Am So., 15. Dez. 2019 um 21:17 Uhr schrieb Michael Käppler <[hidden email]>:
>>> Hi all,
>>> a few days ago I submitted a snippet to the LSR (title "Coloring
>>> successive intervals"). I can see it in the snippet database,
>>> but not in the webpage. The "Contributing" section of LSR states, that:
>>>
>>> "Once the snippet is in, it has to be reviewed and approved by one of
>>> the LSR editors,
>>> and then it must be digested by the search engine. Within a few days,
>>> you should be able to see your snippet online."
>>>
>>> Is this still valid in principle? Or maybe did I something wrong?
>>>
>>> Cheers,
>>> Michael
>> In principle ... yes.
>> Though, I seem to be the only remaining regular active LSR editor, and
>> my time is limited.
>> Thus it may take some more time than the LSR "Contributing" section says.
>> Announcing it on the list helps, ofcourse.
>>
>> I'll take a look soon.
> As promised I had a look.
>
> Many thanks for your snippet!
>
> For now:
>
> First I changed the LSR button "Large snippet" to "Standalone snippet".
> It's a very rare case "Large snippet" is apppropriate, usualy for
> snippets outputting multipe pages. In almost every other case it's
> better to uncheck the buttons or to go for "Standalone snippet".
> Otherwise compressed and (imho) ugly images are output by the LSR.
>
> Please always observe a 80-characters line-width limit for your code.
>
> Your snippet contains some advanced code. In such cases I often want
> to discuss things a bit deeper. So:
>
> What bugged me right from the first glance over it, is your need to
> define several engravers, one for each case.
> I think one engraver should do the work for _all_ those cases.
>
> Attached you'll find my suggestion for this, along with better
> indentation, 80-chars-line-width along with some minor adjustments.
>
> Please have a look.
>
> Nevertheless that can't be the final state, imho.
> May I ask you to add inline code-comments what's done and why?
> There are not so many examples of scheme-engravers around. One
> thoroughly commented would be great.
> Additionally, I'd go for more self-explaining variable-names.
> P.e. "dt-st": for me it's an arbitrary collection of characters, with
> a hyphen somewhere ;)
>
> Thanks,
>    Harm


interval-color-engraver-lsr.ly (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
In reply to this post by Thomas Morley-2
Btw. what I do not understand is why (process-acknowledged) is called
multiple times
after one grob has been acknowledged, it is even called >before< any
grob has been
acknowledged.
You can notice this clearly in
input/regression/scheme-engraver.ly

Cheers,
Michael

Am 16.12.2019 um 01:50 schrieb Thomas Morley:

> Am So., 15. Dez. 2019 um 21:47 Uhr schrieb Thomas Morley
> <[hidden email]>:
>> Am So., 15. Dez. 2019 um 21:17 Uhr schrieb Michael Käppler <[hidden email]>:
>>> Hi all,
>>> a few days ago I submitted a snippet to the LSR (title "Coloring
>>> successive intervals"). I can see it in the snippet database,
>>> but not in the webpage. The "Contributing" section of LSR states, that:
>>>
>>> "Once the snippet is in, it has to be reviewed and approved by one of
>>> the LSR editors,
>>> and then it must be digested by the search engine. Within a few days,
>>> you should be able to see your snippet online."
>>>
>>> Is this still valid in principle? Or maybe did I something wrong?
>>>
>>> Cheers,
>>> Michael
>> In principle ... yes.
>> Though, I seem to be the only remaining regular active LSR editor, and
>> my time is limited.
>> Thus it may take some more time than the LSR "Contributing" section says.
>> Announcing it on the list helps, ofcourse.
>>
>> I'll take a look soon.
> As promised I had a look.
>
> Many thanks for your snippet!
>
> For now:
>
> First I changed the LSR button "Large snippet" to "Standalone snippet".
> It's a very rare case "Large snippet" is apppropriate, usualy for
> snippets outputting multipe pages. In almost every other case it's
> better to uncheck the buttons or to go for "Standalone snippet".
> Otherwise compressed and (imho) ugly images are output by the LSR.
>
> Please always observe a 80-characters line-width limit for your code.
>
> Your snippet contains some advanced code. In such cases I often want
> to discuss things a bit deeper. So:
>
> What bugged me right from the first glance over it, is your need to
> define several engravers, one for each case.
> I think one engraver should do the work for _all_ those cases.
>
> Attached you'll find my suggestion for this, along with better
> indentation, 80-chars-line-width along with some minor adjustments.
>
> Please have a look.
>
> Nevertheless that can't be the final state, imho.
> May I ask you to add inline code-comments what's done and why?
> There are not so many examples of scheme-engravers around. One
> thoroughly commented would be great.
> Additionally, I'd go for more self-explaining variable-names.
> P.e. "dt-st": for me it's an arbitrary collection of characters, with
> a hyphen somewhere ;)
>
> Thanks,
>    Harm


Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
In reply to this post by Michael Käppler-2
Am Sa., 21. Dez. 2019 um 00:35 Uhr schrieb Michael Käppler <[hidden email]>:

>
> Hi Harm et al.,
> attached is my updated version.
> I decided to split up the validity checks from the actual engraver,
> because otherwise in case of invalid parameters the engraver would
> only be instantiated to do nothing and nevertheless have its
> acknowledger called every time.
> The code is much more verbose now, which makes the flow of information
> and control
> clearer I hope.
>
> Please let me know what you think about it.
>
> Cheers,
> Michael

Hi Michael,

thanks again for your updated snippet.

I've taken a closer look.

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.
Along with it, I added a basic check for the user provided list (about
equal length of each sublist)
There was an undefined variable `gen-warntext`, which is now gone as well.

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

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.

Cheers,
  Harm

atest-95.ly (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
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 (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
Hi Michael,

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

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
Am So., 29. Dez. 2019 um 12:11 Uhr schrieb Thomas Morley
<[hidden email]>:

> Some other remarks:
[...]

And a general one.

If a note is last in previous interval and first in a new one, then
the color from the new one is done, leaving the first of the previous
interval with the for it set color:

\score {
  <<
    \new Staff { b c' b' }
    \new Staff { <b c' b'> }
  >>
  \layout {
    \context {
      \Voice
      \consists
        \color_interval_engraver #intervaldefs
          #`(("2-" 0 #t ,green)
             ("7+" 0 #t ,red)
          )
    }
  }
}

No clue how to improve this situation.


Cheers,
  Harm

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
In reply to this post by Thomas Morley-2
Hi Harm,
sorry that I did not respond for over one month... Many other things had
to be done.
Now I would like to finish this snippet to be free for new interesting
things. ;)

I did substantially rework some parts of the engraver, namely switching
to a list-processing-way of dealing with current/last-grobs/pitches/...
Not sure if you like this change. I like it for less code duplication...

> Hi Michael,
>
> Am Fr., 27. Dez. 2019 um 16:12 Uhr schrieb Michael Käppler <[hidden email]>:
>> Hi Harm,
>> thanks for your comments and this inspiring discussion!
> me too!
>
> Iiuc, you want to keep the possibility the user defines the intervaldefs like
> #(define intervaldefs
>     '(("my-prime++" . (0 . 1))
>       ...)
Exactly.

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

>
> 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
Done. The reason was that I thought of 'n' as the "running" variable
(n=1,2,3 etc.) and 'N' as the last value,
that 'n' finally reaches.
But maybe this was rather misleading.

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

>
>
>>> 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) '()))
Done.
>
> 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.
Done.

>
> 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.
Not at all. It's interesting to learn from your style!

> And a general one.
>
> If a note is last in previous interval and first in a new one, then
> the color from the new one is done, leaving the first of the previous
> interval with the for it set color:
>
> \score {
>    <<
>      \new Staff { b c' b' }
>      \new Staff { <b c' b'> }
>    >>
>    \layout {
>      \context {
>        \Voice
>        \consists
>          \color_interval_engraver #intervaldefs
>            #`(("2-" 0 #t ,green)
>               ("7+" 0 #t ,red)
>            )
>      }
>    }
> }
>
> No clue how to improve this situation.
You're absolutely right. I think this is unavoidable with this design
(coloring both notes).
I added a warning for this case and, while I was at it, some more debug
output.
Some neat solution would be to draw Horizontal_brackets instead. Thus we
could
correctly represent overlapping intervals, too.
I do not have the time to implement this, however.

Cheers,
Michael



interval-color-engraver-lsr.ly (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
Hi Michael,

Am Mi., 5. Feb. 2020 um 14:54 Uhr schrieb Michael Käppler <[hidden email]>:
>
> Hi Harm,
> sorry that I did not respond for over one month... Many other things had
> to be done.

Same here...

> Now I would like to finish this snippet to be free for new interesting
> things. ;)
>
> I did substantially rework some parts of the engraver, namely switching
> to a list-processing-way of dealing with current/last-grobs/pitches/...
> Not sure if you like this change. I like it for less code duplication...

Fine with me, I already updated the LSR-code, for now unapproved.
Please have a look whether all is ok, snippet-description and the like.

Probably one suggestion:
How about gving the user some hint or example-message what can be
expected if warnings are triggered or debug-mode is enabled?

> > If a note is last in previous interval and first in a new one, then
> > the color from the new one is done, leaving the first of the previous
> > interval with the for it set color:
> >
> > \score {
> >    <<
> >      \new Staff { b c' b' }
> >      \new Staff { <b c' b'> }
> >    >>
> >    \layout {
> >      \context {
> >        \Voice
> >        \consists
> >          \color_interval_engraver #intervaldefs
> >            #`(("2-" 0 #t ,green)
> >               ("7+" 0 #t ,red)
> >            )
> >      }
> >    }
> > }
> >
> > No clue how to improve this situation.

> You're absolutely right. I think this is unavoidable with this design
> (coloring both notes).
> I added a warning for this case and, while I was at it, some more debug
> output.
> Some neat solution would be to draw Horizontal_brackets instead. Thus we
> could
> correctly represent overlapping intervals, too.
> I do not have the time to implement this, however.

Same here ...

> Cheers,
> Michael


Thanks,
  Harm

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2

Hi Harm,

> Fine with me, I already updated the LSR-code, for now unapproved.
> Please have a look whether all is ok, snippet-description and the like.
Thank you for updating!
Will update the description this evening.
Do you have any idea how to preserve indentation within <code> blocks?
Inserting &nbsp; or the like seems hacky, inline-css-styling too...
>
> Probably one suggestion:
> How about gving the user some hint or example-message what can be
> expected if warnings are triggered or debug-mode is enabled?
Do you mean inside the description or as snippet output?
Btw, do you know a way to dump warning-messages to the graphical output?

Cheers,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
Am 10.02.2020 um 10:10 schrieb Michael Käppler:
>
>
> Do you have any idea how to preserve indentation within <code> blocks?
> Inserting &nbsp; or the like seems hacky, inline-css-styling too...
Ok, found out that the combination of <pre> and <code> tags does work
well for that purpose.

>>
>> Probably one suggestion:
>> How about gving the user some hint or example-message what can be
>> expected if warnings are triggered or debug-mode is enabled?
> Do you mean inside the description or as snippet output?
> Btw, do you know a way to dump warning-messages to the graphical output?
Added some examples to the description.
The LSR seems to be down at the moment, though.
What bothers me a little bit is the unpredictable order of warnings on
the console.
Do you know a way to fix this?

Cheers,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
In reply to this post by Michael Käppler-2
Am Mo., 10. Feb. 2020 um 10:11 Uhr schrieb Michael Käppler <[hidden email]>:

>
>
> Hi Harm,
>
> > Fine with me, I already updated the LSR-code, for now unapproved.
> > Please have a look whether all is ok, snippet-description and the like.
> Thank you for updating!
> Will update the description this evening.
> Do you have any idea how to preserve indentation within <code> blocks?
> Inserting &nbsp; or the like seems hacky, inline-css-styling too...
> >
> > Probably one suggestion:
> > How about gving the user some hint or example-message what can be
> > expected if warnings are triggered or debug-mode is enabled?
> Do you mean inside the description or as snippet output?

In the description or probably as a comment in the code.

> Btw, do you know a way to dump warning-messages to the graphical output?

Nope. My experiments with 'with-output-to-string' weren't successful,
at least for ly:warning and similar.
Ofcourse one could avoid ly:warning and go for some other printing
function, but I think it would lead to more confusion instead of
helping the user.

Best,
  Harm

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
In reply to this post by Michael Käppler-2
Am Mo., 10. Feb. 2020 um 20:43 Uhr schrieb Michael Käppler <[hidden email]>:
>
> Am 10.02.2020 um 10:10 schrieb Michael Käppler:
> >
> >
> > Do you have any idea how to preserve indentation within <code> blocks?
> > Inserting &nbsp; or the like seems hacky, inline-css-styling too...
> Ok, found out that the combination of <pre> and <code> tags does work
> well for that purpose.

:)

> >> Probably one suggestion:
> >> How about gving the user some hint or example-message what can be
> >> expected if warnings are triggered or debug-mode is enabled?
> > Do you mean inside the description or as snippet output?
> > Btw, do you know a way to dump warning-messages to the graphical output?
> Added some examples to the description.
> The LSR seems to be down at the moment, though.
> What bothers me a little bit is the unpredictable order of warnings on
> the console.
> Do you know a way to fix this?

Sorry, no clue if it's even possible ...

Cheers,
  Harm

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Michael Käppler-2
In reply to this post by Thomas Morley-2
Am 10.02.2020 um 20:51 schrieb Thomas Morley:
> Am Mo., 10. Feb. 2020 um 10:11 Uhr schrieb Michael Käppler <[hidden email]>:
>>
>> Hi Harm,
>>
>>> Fine with me, I already updated the LSR-code, for now unapproved.
>>> Please have a look whether all is ok, snippet-description and the like.
Everything ok now, please approve if you're fine with it, too.

Cheers,
Michael

Reply | Threaded
Open this post in threaded view
|

Re: LSR contribution

Thomas Morley-2
Am Mo., 10. Feb. 2020 um 22:15 Uhr schrieb Michael Käppler <[hidden email]>:

>
> Am 10.02.2020 um 20:51 schrieb Thomas Morley:
> > Am Mo., 10. Feb. 2020 um 10:11 Uhr schrieb Michael Käppler <[hidden email]>:
> >>
> >> Hi Harm,
> >>
> >>> Fine with me, I already updated the LSR-code, for now unapproved.
> >>> Please have a look whether all is ok, snippet-description and the like.
> Everything ok now, please approve if you're fine with it, too.
>
> Cheers,
> Michael

Approves as
http://lsr.di.unimi.it/LSR/Item?id=1100

Many thanks,
  Harm