quarter tones in tablature

quarter tones in tablature

issue 4643

micro-tones in TabStaff are now printable.  Unless the chosen
string-tuning will allow it, this feature is disabled for
FretBoards.
- changing determine-frets (adding an optional argument) and
  fret-number-tablature-format
- adding a regtest with quarter-tone-string-tuning
- documenting it in Documentation/notation/fretted-strings.itely
  and Documentation/changes.tely

[thomasmorley651, 2015-11-02 21:27:46]

Please review

[dak, 2015-11-08 14:22:26]

I'm currently trying to get tax declarations finished with a very very looming
deadline, so I am worse at doing reviews properly than I am anyway.  This one
hit me in the face though and would warrant changing before people start relying
on it.

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

https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm...
scm/translation-functions.scm:237: ((determine-frets #:optional
(support-non-integer-fret? #f))
That's pretty bad.  determine-frets was a public function previously, changing
it to a hook it to a function generator is incompatible.

As a note aside, define*-public with curryied functions is broken in Guile-2 and
needs to get split into define* and export separately.  But that's not really
relevant here.

support-non-integer-fret belongs into a context property, and determine-fret
gets passed a context anyway, so that's quite straightforward to do.  No need to
change the call interface to determine-frets.

[thomasmorley651, 2015-11-08 20:19:16]

On 2015/11/08 14:22:26, dak wrote:
> I'm currently trying to get tax declarations finished with a very very looming
> deadline, so I am worse at doing reviews properly than I am anyway.  This one
> hit me in the face though and would warrant changing before people start
relying
> on it.

Ok.
There's
https://sourceforge.net/p/testlilyissues/issues/3088/
currently on count-down, as well.
I could imagine you would like to say something to it. 
Should I put it back on 'review' (or maybe 'needs-work')?

> https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm
> File scm/translation-functions.scm (right):
> 
>
https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm...
> scm/translation-functions.scm:237: ((determine-frets #:optional
> (support-non-integer-fret? #f))
> That's pretty bad.  determine-frets was a public function previously, changing
> it to a hook it to a function generator is incompatible.

I don't understand. Why incompatible? At least it survived our regtests and my
local testings.

> As a note aside, define*-public with curryied functions is broken in Guile-2
and
> needs to get split into define* and export separately.  But that's not really
> relevant here.

Ok, understood.

> support-non-integer-fret belongs into a context property, and determine-fret
> gets passed a context anyway, so that's quite straightforward to do.  No need
to
> change the call interface to determine-frets.

I could create a follow up, introducing "support-non-integer-fret?" as a
context-property and change determine-fret-code accordingly.
I think that's your suggestion.

[dak, 2015-11-08 21:51:27]

On 2015/11/08 20:19:16, thomasmorley651 wrote:
> On 2015/11/08 14:22:26, dak wrote:

> 
> >
https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm
> > File scm/translation-functions.scm (right):
> > 
> >
>
https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm...
> > scm/translation-functions.scm:237: ((determine-frets #:optional
> > (support-non-integer-fret? #f))
> > That's pretty bad.  determine-frets was a public function previously,
changing
> > it to a hook it to a function generator is incompatible.
> 
> I don't understand. Why incompatible? At least it survived our regtests and my
> local testings.

If someone uses it, for example as a component in his own fret-determining
function, it will behave differently from before.  It is an exported function so
people might have used it.

> > support-non-integer-fret belongs into a context property, and determine-fret
> > gets passed a context anyway, so that's quite straightforward to do.  No
need
> to
> > change the call interface to determine-frets.
> 
> I could create a follow up, introducing "support-non-integer-fret?" as a
> context-property and change determine-fret-code accordingly.
> I think that's your suggestion.

Yes.  Its name should be chosen to fit with existing properties (question marks
are likely a killer in property names anyway), but that's the idea.  It might
also be an idea to make this a property valid-fret-predicate and set it index?
by default (and one could override with number? instead).  Though one would need
to check how this combines with instruments with irregular fret arrangement
since those have different valid frets on each string and I think we also
support that in some manner.

[thomasmorley651, 2015-11-08 22:53:30]

On 2015/11/08 21:51:27, dak wrote:
> On 2015/11/08 20:19:16, thomasmorley651 wrote:
> > On 2015/11/08 14:22:26, dak wrote:
> 
> > 
> > >
> https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm
> > > File scm/translation-functions.scm (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm...
> > > scm/translation-functions.scm:237: ((determine-frets #:optional
> > > (support-non-integer-fret? #f))
> > > That's pretty bad.  determine-frets was a public function previously,
> changing
> > > it to a hook it to a function generator is incompatible.
> > 
> > I don't understand. Why incompatible? At least it survived our regtests and
my
> > local testings.
> 
> If someone uses it, for example as a component in his own fret-determining
> function, it will behave differently from before.  It is an exported function
so
> people might have used it.

True. So wouldn't that mean we can't change any public function regarding amount
and/or type of arguments?
At least without convert-rule.

> 
> > > support-non-integer-fret belongs into a context property, and
determine-fret
> > > gets passed a context anyway, so that's quite straightforward to do.  No
> need
> > to
> > > change the call interface to determine-frets.
> > 
> > I could create a follow up, introducing "support-non-integer-fret?" as a
> > context-property and change determine-fret-code accordingly.
> > I think that's your suggestion.
> 
> Yes.  Its name should be chosen to fit with existing properties (question
marks
> are likely a killer in property names anyway), 
 
They are.

> but that's the idea.  

patch is up, see
https://sourceforge.net/p/testlilyissues/issues/4655/
following the above idea.

> It might
> also be an idea to make this a property valid-fret-predicate and set it index?
> by default (and one could override with number? instead).  

Maybe, but this would warant a new issue, I'd say

> Though one would need
> to check how this combines with instruments with irregular fret arrangement
> since those have different valid frets on each string and I think we also
> support that in some manner.

Do we? Right now I'm not sure.