Re: Allow left-handed fret-markups (issue 339270043 by address@hidden)

[thomasmorley65]

On 2018/01/22 23:52:29, Carl wrote:
> Looks good to me, but I have one suggestion.

> Thanks,

> Carl

> https://codereview.appspot.com/339270043/diff/1/scm/fret-diagrams.scm
> File scm/fret-diagrams.scm (right):


https://codereview.appspot.com/339270043/diff/1/scm/fret-diagrams.scm#newcode365
> scm/fret-diagrams.scm:365: ((and (eq? orientation 'landscape)
left-handed)
> I would tend to write this as
>   ((eq? orientation 'landscape)
>      (cons fret-coordinate (if left-handed (- (1- string-count)
> string-coordinate)
>                                                                    (-
> string-coordinate (1- string-count)))
>    (eq? orientation 'opposing-landscape)
>       (cons (- fret-coordinate) (if left-handed (- string-coordinate
(1-
> string-count))

  (- (1-
> string-count) string-coordinate)))
>    (else (cons (if left-handed (- string-coordinate) string-coordinate)
(-
> fret-coordinate)))))

> I think it shows the structure better (i.e. it shows three different
> orientations, and it explicitly shows where the left-handed changes
things (y
> coordinate for landscape, x coordinate for regular).  But I don't
insist on this
> by any means.

Yep, the difference between left- and right-handed are certain negative
instead of a positive values.
I'd love to use a multiplier retrieved from left-handed.
Though, how to code properly and user-friendly.
One could do
(1) separate conditions (that's what I've done). Disadvantage: code is
more complex. No multiplier.
(2) Let 'left-handed accept RIGHT/LEFT. Disadvantage is the need to
input it via an quasiquoted list
(3) Let 'left-handed accept -1/1. Disadvantage: it may be not that
obvious to the user what -1/1 actually means
(4) Let 'left-handed accept symbols left/right. Disadvantage: need to
transform these symbols into -1/1.
Something like (if (eq? left-handed 'left) -1 1) needs to be added.
(4) Drop 'left-handed and get the -1/1 from (sign string-distance)

Opinions?

https://codereview.appspot.com/339270043/