Place barres on fret diagrams if they can be inferred

Place barres on fret diagrams if they can be inferred

[thomasmorley651, 2018-05-03 22:28:26]

Hi Carl,

nice work.

A general thought:
As far as I understand the code will work automatically. Though, I foresee some
users wanting to switch it off (ofcourse not the majority, but there is always
somebody with different wishes).
Any chance to create an option for it?

Some inline remarks below.

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:258: (let* ((barres (make-array 0 5 4)) ; 5
fingers, 4 elements/barre
Afaik, this will be the first use of make-array in our source,
so I'd love some more explanations in the comments, especially about 5 and 4
bounds.
5 is obviously the amount of possible fingers, as you stated already.
About 4: this will be filled lateron with 
(barre-symbol 
 from-higher-string, p.e. 1
 to-lower-string, p.e. 6
 fret-number)

Why this order for second/third element?
I'm aware it makes no programmatical difference, but all NR examples do it the
other way round, something like:
(barre 5 1 3) and not (barre 1 5 3)

Probably exchange those to be consistent with the NR.
Here, while doing array-set! and in the filtering-condition later.

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:278: (not (= (list-ref l 3 ) 0))
Why not use (second/third/fourth l) instead of (list-ref l 1/2/3)?

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:289: (have-fingers (not (null? (filter (lambda
(sf)
Why (have-fingers (not ...))
and as predicate in line 306 as 
(not have-fingers)
Couldn't both (not ...) be deleted?

[Carl, 2018-05-04 04:21:53]

Harm,

Thanks for the great comments.

If the user doesn't want the barre to be displayed, they can avoid it by setting
fret-diagram-details.barre-type = #'none

Thanks,

Carl

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:258: (let* ((barres (make-array 0 5 4)) ; 5
fingers, 4 elements/barre
On 2018/05/03 22:28:25, thomasmorley651 wrote:
> Afaik, this will be the first use of make-array in our source,
> so I'd love some more explanations in the comments, especially about 5 and 4
> bounds.
> 5 is obviously the amount of possible fingers, as you stated already.
> About 4: this will be filled lateron with 
> (barre-symbol 
>  from-higher-string, p.e. 1
>  to-lower-string, p.e. 6
>  fret-number)
> 
> Why this order for second/third element?
> I'm aware it makes no programmatical difference, but all NR examples do it the
> other way round, something like:
> (barre 5 1 3) and not (barre 1 5 3)
> 
> Probably exchange those to be consistent with the NR.
> Here, while doing array-set! and in the filtering-condition later.

Hmm.  I just wanted to make a range from lower string number to higher string
number, because that's the way I think about intervals.  I can't see any problem
with making it go from higher string number to lower string number.  And I guess
that consistency with the NR is not a bad idea, even though I don't think it's
essential.

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:278: (not (= (list-ref l 3 ) 0))
On 2018/05/03 22:28:25, thomasmorley651 wrote:
> Why not use (second/third/fourth l) instead of (list-ref l 1/2/3)?

Because when I learned Scheme more than 30 years ago, first/second were
"syntactic sugar" and were frowned upon. 

But I can see that for this case, first and second would be easier for the
casual reader to understand.  Good suggestion.

https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:289: (have-fingers (not (null? (filter (lambda
(sf)
On 2018/05/03 22:28:26, thomasmorley651 wrote:
> Why (have-fingers (not ...))
> and as predicate in line 306 as 
> (not have-fingers)
> Couldn't both (not ...) be deleted?

Certainly, if we changed the name from have-fingers to no-fingers.  And getting
rid of the negative logic would certainly make it easier to understand.  Good
call!

[Carl, 2018-05-04 13:31:34]

Respond to Harm's comments

[thomasmorley651, 2018-05-04 20:09:36]

Hi Carl,

apart from one nit (see below)
LGTM

Probably not related to this patch:
Clicking "Delta from patch set" gives strange results. No clue what Rietveld
does...

https://codereview.appspot.com/294570043/diff/20001/scm/translation-functions...
File scm/translation-functions.scm (right):

https://codereview.appspot.com/294570043/diff/20001/scm/translation-functions...
scm/translation-functions.scm:305: (display barre-list)
Forgotten debugging-code?

[Carl, 2018-05-05 23:07:45]

Remove debug printing

[thomasmorley651, 2018-05-06 22:40:20]

On 2018/05/05 23:07:45, Carl wrote:
> Remove debug printing

Very nice! 

LGTM ofcourse

Thanks!