Re: Issue 754: don't transpose generic property-setting music commands (issue 7303057)

[dak]

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.


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