Issue 754: don't transpose generic property-setting music commands

Issue 754: don't transpose generic property-setting music commands

The actual issue was that
\transpose c e { \transposition bes ... }

created a Midi corresponding to an instrument using a transposition of
ges, leaving the Midi unchanged.  Making the generic property-setting
commands (\set and \override) impervious to transposition will also
keep \transpose from tampering with user-set values.

This is particularly important since the pitch data type in LilyPond
is also being used for signifying intervals or pitch differences
rather than absolute pitches.

[dak, 2013-02-07 14:19:55]

Cherry-pick DOC change from other approach.

[Keith, 2013-02-08 04:46:27]

Yesterday's patch was better.
The 'untransposable property is more closely related to the source of the events
(\transposition or \instrumentSwitch) than to the event type (override or
propertySet).

https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm
File scm/define-music-types.scm (right):

https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm#n...
scm/define-music-types.scm:379: (untransposable . #t)
Well, not every Grob property that is a pitch is untransposable. The second
pitch in a pitchedTrill, for example, should be transposed along with the first
pitch.
Do we need to remember to avoid \override on those that are transposable?

https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm#n...
scm/define-music-types.scm:446: (untransposable . #t)
Not every context property that is a pitch is untransposable. The tonic of the
Staff, for example.

[dak, 2013-02-08 08:40:29]

On 2013/02/08 04:46:27, Keith wrote:
> Yesterday's patch was better.

I beg to differ.  It did not address instrument definitions, or manual settings
of instrumentTransposition.

> The 'untransposable property is more closely related to the source of the
events
> (\transposition or \instrumentSwitch) than to the event type (override or
> propertySet).

To be perfectly clear about this: untransposability is related to individual
properties.  instrumentTransposition should be untransposable no matter how it
is being set.

One problem is that "untransposable" is a very coarse tool compared to, say, the
callback system for \relative.

> https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm
> File scm/define-music-types.scm (right):
> 
>
https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm#n...
> scm/define-music-types.scm:379: (untransposable . #t)
> Well, not every Grob property that is a pitch is untransposable. The second
> pitch in a pitchedTrill, for example, should be transposed along with the
first
> pitch.

I don't see that there is any interface for pitched Trills that would ride on
explicit property setting commands.

And don't you think it strange that if you use \override for setting a property,
the pitches get transposed, whereas if you use \tweak for setting a property,
the pitch does not get transposed?  Where is the logic with that?

> Do we need to remember to avoid \override on those that are transposable?

I don't see anybody using \override here on pitched trills.  And yes: it would
appear that for things sufficiently sophisticated to require transposition, one
would want an own music type.

>
https://codereview.appspot.com/7303057/diff/2001/scm/define-music-types.scm#n...
> scm/define-music-types.scm:446: (untransposable . #t)
> Not every context property that is a pitch is untransposable. The tonic of the
> Staff, for example.

The tonic of the Staff is set via KeyChangeEvent, not via PropertySet.  It is
not manipulated independently from the scale.

[dak, 2013-02-08 09:20:58]

On 2013/02/08 08:40:29, dak wrote:
> On 2013/02/08 04:46:27, Keith wrote:
> > Yesterday's patch was better.
> 
> I beg to differ.  It did not address instrument definitions, or manual
settings

Concretely: can you show an example of not-just-academical LilyPond source that
would be change to the worse with patch applied?

Do you consider it sensible that values set via \override are transposed (but
only if they are not set via a callback) while values set via \tweak aren't?

Can you see any internal use of \set/\override in LilyPond source (or the
corresponding usage of PropertySet/OverrideProperty) that would now behave
inappropriately under transposition?

[Keith, 2013-02-09 05:46:45]

On Fri, 08 Feb 2013 01:20:59 -0800, <dak@gnu.org> wrote:

> On 2013/02/08 08:40:29, dak wrote:
>> On 2013/02/08 04:46:27, Keith wrote:
>> > Yesterday's patch was better.
>
>> I beg to differ.  It did not address instrument definitions, or manual
> settings

Well, we had a nice plane for \instrumentSwitch,
and I did not think it wise to assume that the pitch should be protected in
   \set property = #(ly:make-pitch 0 -4 0)

I only said that patch was better; this patch is good enough.

> Concretely: can you show an example of not-just-academical LilyPond
> source that would be change to the worse with patch applied?
>
Nope.  I still encourage caution, because people use LilyPond for very academic
projects.  Also, it is academically possible that some programmer will want to
add a override-able property that is a pitch that should be transposed; he would
need to find and understand this patch.

> Do you consider it sensible that values set via \override are transposed
> (but only if they are not set via a callback) while values set via
> \tweak aren't?
>
I have given it no thought.

> Can you see any internal use of \set/\override in LilyPond source (or
> the corresponding usage of PropertySet/OverrideProperty) that would now
> behave inappropriately under transposition?
>
No.

> https://codereview.appspot.com/7303057/

[dak, 2013-02-09 08:54:03]

On 2013/02/09 05:46:45, Keith wrote:
> On Fri, 08 Feb 2013 01:20:59 -0800, <mailto:dak@gnu.org> wrote:
> 
> > On 2013/02/08 08:40:29, dak wrote:
> >> On 2013/02/08 04:46:27, Keith wrote:
> >> > Yesterday's patch was better.
> >
> >> I beg to differ.  It did not address instrument definitions, or manual
> > settings
> 
> Well, we had a nice plane for \instrumentSwitch,

Did we?  The plan more or less was to put 'untransposed on it, and
that would mean that properties travelling through \instrumentSwitch
don't get any of their pitches transformed, while properties given
through \set do.  Alternatively, you just treat
instrumentTransposition, and that would still mean that setting
instrumentTransposition via \instrumentSwitch is not prone to
transposition, while setting instrumentTransposition via \set _is_
prone to transposition.

> and I did not think it wise to assume that the pitch should be protected in
>    \set property = #(ly:make-pitch 0 -4 0)

You call it "protected".  I call it "unmolested".  There is no
inherent reason that pitches in \set/\override should be subjected to
transposition.

\transpose works on music expressions and stream events in a rather
rough manner.  Take a look at transpose_mutable in lily/music.cc.  The
rule is that any music or event property that is a pitch gets
transposed.  Every property that is named "tonic" gets a normalized
octave afterwards (ugh).  A property that is named "element" gets
recursively transposed (only if it is music, not an event), and
"elements" and "articulations" get transposed elementwise.  And if a
property is named "pitch-alist", its keys get transposed.

Now with PropertySet and OverrideProperty, the property name in the
music that we are talking about is invariably "value", so it only gets
the pitch treatment.  And if we are talking about tweaks, they appear
in arbitrary music events as a field "tweaks" always containing a
list, so nothing in tweaks gets ever transposed.

You complained about \set tonic ... not being transposable.  But
neither is \set keySignature ...  And I don't see any code which would
expect otherwise.

> I only said that patch was better; this patch is good enough.

Well, LilyPond only deserves the best.  The previous patch was most
definitely more confined, and that is its main virtue.  As a treatment
for the reported issue, this seems like the least-impact change.  In
fact, its impact was limited enough that \instrumentSwitch already was
not being covered.  So you proposed "Instrument definitions carry
context definitions for an instrument, except that
instrumentTransposition is not being transposed unlike what \set would
do."

Ugh.

The original approach with regard to the sign of
instrumentTransposition might be summarized as

    Oops, we encountered some bad side effects from set/override
    accidentally transposing values that are pitches, like
    instrumentTransposition.  What are we going to do?

    Let us change the definition of the sign of
    instrumentTransposition so that it does something really clever
    even though it is incoherent and quite useless as opposed to
    incoherent and totally useless as previously.

Now that is not cleaning up the mess but rather hanging Christmas
ornaments from it.  Some people might have gotten enamored to the
ornaments, but I don't see a reasonable way to save them.

> > Concretely: can you show an example of not-just-academical
> > LilyPond source that would be change to the worse with patch
> > applied?

> Nope.  I still encourage caution, because people use LilyPond for
> very academic projects.  Also, it is academically possible that some
> programmer will want to add a override-able property that is a pitch
> that should be transposed; he would need to find and understand this
> patch.

You are constantly working from the assumption that it is natural to
assume context-property setting commands setting pitches should be
subject to transposition.  And you list "tonic" as an example.  Except
that "tonic" needs to get _octave-normalized_ after transposition, and
\set tonic ... will _not_ achieve that.  The transposition of
instrumentTransposition is spectacularly weird and almost entirely
useless.  And so on.  Yes, there is the conceivable case of this
weirdness being _exploitable_ in a clever way for some clever purpose.

It doesn't matter.  LilyPond is still being actively maintained.  If a
feature is required, it can be implemented properly, with a proper
interface, _without_ resorting to exploits.

> > Do you consider it sensible that values set via \override are
> > transposed (but only if they are not set via a callback) while
> > values set via \tweak aren't?

> I have given it no thought.

You should before labelling one approach better than another.
instrumentTransposition turns out to be just one facet of a larger
issue, and in this case I think we are better off solving the larger
issue even though exploits for the inconsistent behavior already
exist: we have been pointed to mailing list threads demonstrating code
catered for this, like

\transpose bes c' { \transposition c' ...

for writing for an instrument with transposition bes in concert pitch.
But this particular usage will break even with the small "solution" to
the issue.  I don't know of exploits other than
instrumentTransposition, and if there are any, we'd better plug them
along with the rest.

> > Can you see any internal use of \set/\override in LilyPond source
> > (or the corresponding usage of PropertySet/OverrideProperty) that
> > would now behave inappropriately under transposition?

> No.

I am putting the patch back on countdown for 2013/02/11 then.  I am
fully aware that it is not healthy that as "Patch meister" I am judge
and jury in a process where I also happen to be defendant (or, in the
case of Mike's work, state attorney).  And I can't really pretend to
act as an impartial instance when I have very strong opinions.  But I
can't recluse myself when nobody else picks up.  So you have to make
do, or volunteer.

[Keith, 2013-02-11 06:18:57]

On 2013/02/09 08:54:03, dak wrote:

> It doesn't matter.  LilyPond is still being actively maintained.  If a
> feature is required, it can be implemented properly, with a proper
> interface, _without_ resorting to exploits.
> 

I was hoping to be able to provide a backward-compatibility function, as in
<https://codereview.appspot.com/7312074/>, just in case someone used
\transposition inside of \transpose the way it used to work.

[dak, 2013-02-11 09:25:20]

On 2013/02/11 06:18:57, Keith wrote:
> On 2013/02/09 08:54:03, dak wrote:
> 
> > It doesn't matter.  LilyPond is still being actively maintained.  If a
> > feature is required, it can be implemented properly, with a proper
> > interface, _without_ resorting to exploits.
> > 
> 
> I was hoping to be able to provide a backward-compatibility function, as in
> <https://codereview.appspot.com/7312074/>, just in case someone used
> \transposition inside of \transpose the way it used to work.

I did not even look at the proposed patch but it touches several binaries.  That
should not be necessary.  Of course, in this variant of the patch, it would be
feasible just to put untransposable back to #f, but since the followup work
includes reversing the sign of the pitch again, this is a dead end.  I think
that something using ApplyContext should be workable, though, so that the
compatibility function gets along just using LilyPond/Scheme and can be loaded
on-demand.

[dak, 2013-02-11 12:39:02]

On 2013/02/11 09:25:20, dak wrote:
> On 2013/02/11 06:18:57, Keith wrote:
> > On 2013/02/09 08:54:03, dak wrote:
> > 
> > > It doesn't matter.  LilyPond is still being actively maintained.  If a
> > > feature is required, it can be implemented properly, with a proper
> > > interface, _without_ resorting to exploits.
> > > 
> > 
> > I was hoping to be able to provide a backward-compatibility function, as in
> > <https://codereview.appspot.com/7312074/>, just in case someone used
> > \transposition inside of \transpose the way it used to work.
> 
> I did not even look at the proposed patch but it touches several binaries. 
That
> should not be necessary.  Of course, in this variant of the patch, it would be
> feasible just to put untransposable back to #f, but since the followup work
> includes reversing the sign of the pitch again, this is a dead end.  I think
> that something using ApplyContext should be workable, though, so that the
> compatibility function gets along just using LilyPond/Scheme and can be loaded
> on-demand.

Ok, ApplyContext does not cut it since it does not offer access to the original
music.  But try, for example,
oldTransposition =
#(define-music-function (parser location pitch) (ly:pitch?)
  (make-music 'SequentialMusic
   'pitch (ly:pitch-negate pitch)
   'elements-callback
   (lambda (m)
    (list
     #{ \transposition $(ly:pitch-negate
			 (ly:music-property m 'pitch))
     #}))))

and you'll see that \oldTransposition will just make the old behavior available
again.  You can't assign this particular definition to transposition, for
obvious reasons, but saving and using the old value of \transposition should be
doable in an obvious manner.

[Keith, 2013-02-11 20:53:53]

On 2013/02/11 12:39:02, dak wrote:
> On 2013/02/11 09:25:20, dak wrote:
> > On 2013/02/11 06:18:57, Keith wrote:
> > > 
> > > I was hoping to be able to provide a backward-compatibility function, as
in

> > I did not even look at the proposed patch but it touches several binaries. 
> That
> > should not be necessary.  Of course, in this variant of the patch, it would
be
> > feasible just to put untransposable back to #f, but since the followup work
> > includes reversing the sign of the pitch again, this is a dead end.  I think
> > that something using ApplyContext should be workable, though, so that the
> > compatibility function gets along just using LilyPond/Scheme and can be
loaded
> > on-demand.

Well, my previous suggestion for backward-compatibility used LilyPond, but you
promptly adjusted your patch to stop my suggestion from working.

> oldTransposition =
> #(define-music-function (parser location pitch) (ly:pitch?)
>   (make-music 'SequentialMusic
>    'pitch (ly:pitch-negate pitch)
>    'elements-callback
>    (lambda (m)
>     (list
>      #{ \transposition $(ly:pitch-negate
> 			 (ly:music-property m 'pitch))
>      #}))))

Great.  The pitches in structures built by make-music (differently to those
created by property-set) are molested by \transpose, but then a sneaky callback
tampers with the music and creates -- wait for it -- a nested property-set!  It
just goes to show that people will take advantage of _any_ reproducible
behavior, no matter how inconsistent.

Just kidding.  This should be fine for a backward-compatibility function, in
case anyone has a 200-page score that accidentally depended on the old behavior.

[dak, 2013-02-12 01:18:00]

On 2013/02/11 20:53:53, Keith wrote:
> On 2013/02/11 12:39:02, dak wrote:
> > On 2013/02/11 09:25:20, dak wrote:
> > > On 2013/02/11 06:18:57, Keith wrote:
> > > > 
> > > > I was hoping to be able to provide a backward-compatibility function, as
> in
> 
> > > I did not even look at the proposed patch but it touches several binaries.

> > That
> > > should not be necessary.  Of course, in this variant of the patch, it
would
> be
> > > feasible just to put untransposable back to #f, but since the followup
work
> > > includes reversing the sign of the pitch again, this is a dead end.  I
think
> > > that something using ApplyContext should be workable, though, so that the
> > > compatibility function gets along just using LilyPond/Scheme and can be
> loaded
> > > on-demand.
> 
> Well, my previous suggestion for backward-compatibility used LilyPond, but you
> promptly adjusted your patch to stop my suggestion from working.
> 
> > oldTransposition =
> > #(define-music-function (parser location pitch) (ly:pitch?)
> >   (make-music 'SequentialMusic
> >    'pitch (ly:pitch-negate pitch)
> >    'elements-callback
> >    (lambda (m)
> >     (list
> >      #{ \transposition $(ly:pitch-negate
> > 			 (ly:music-property m 'pitch))
> >      #}))))
> 
> Great.  The pitches in structures built by make-music (differently to those
> created by property-set) are molested by \transpose, but then a sneaky
callback
> tampers with the music and creates -- wait for it -- a nested
property-set!

The copy&paste job does not match what happens here.  In this case,
transpose manipulates a pitch field that is by naming and function
_chosen_ to get acted upon.  That's not the abomination.  The
abomination is using a music type intended for something different and
printing as something different for setting a transposition using a
callback not intended for it.

Arguably, one should rather use TransposedMusic or similar for this
abomination.  However, this would involve tampering _both_ with
elements-callback as well as iterator-ctor.  And TransposedMusic does
not really have anything to do with \transposition except by relation
of the names.

I'd have preferred, in order of priority, using something like
a) ApplyContext.  This would not have access to the event, however.
b) a user-defined event.  We don't have those in a reliable manner
yet.

This is a compatibility hack.  The first version (and most likely also
the ultimate version) did not use #{ ... #} but rather copy&paste from
the internals of \transposition.  However, those are going to change
when I change the sign of instrumentTransposition.  So for better
longevity of this comment, I picked #{ ... #}.

> It just goes to show that people will take advantage of _any_
> reproducible behavior, no matter how inconsistent.

It is a compatibility hack purposefully not affecting _any_ of the
LilyPond code base when not used.  Once user-defined events are more
than a semi-manual hack, I'd use them.  Since they aren't, I chose
this self-contained hack.  I'd not recommend it for user-maintained
code: it relies on SequentialMusic going through elements-callback.
But maintaining this hack through changes of SequentialMusic should be
less work than maintaining explicit code paths in the main code.  We
have still lily_1_8_relative mottled through our code base, and
ripping out this compatibility cruft from versions of 1.8 or earlier
(seriously?!?)  was vetoed because people might have been using it as
a user choice option.

Since it seems almost impossible to stamp such compatibility cruft
out, I prefer not letting it in in the first place.

> Just kidding.

Sure, but the amount of hypocrisy of mine that you are basing the
kidding on might be different from what you thing and/or suggest, so I
prefer to have this in the open.

> This should be fine for a backward-compatibility function, in case
> anyone has a 200-page score that accidentally depended on the old
> behavior.

The situation is worse than that.  The mailing list carries examples
where people have purposefully rather than accidentally based their
code on the old behavior.  And in light of the available
documentation, I can't blame them for doing experimentation and taking
bad choices, choices that it takes someone with a clue about
maintainability, predictability and system design to do right.

So we are not talking about "accidentally" but rather "imprudently"
or, as that level of prudence is nothing to require of a user,
"unwisely" here.

Curing the fallout with documentation, convert-ly rules and backward
compatibility include file will be much more of a wart and disgrace
than the above code.