Add the command \offset to LilyPond

Add the command \offset to LilyPond

The commands \override and \tweak set grob properties to absolute values.  The ability to specify relative values--to displace values of various properties from current settings--would be a useful enhancement of LilyPond.  Currently, this is possible for default values of the property 'control-points using the \shape command.

The following patch seeks to generalize the application of offsets to grob properties.  Both overrides and tweaks are supported, on the model of the commands \shape and \alterBroken.

Offsets are currently limited to three data types: number, number-pair, and number-pair-list (this last is defined by this patch and represents the type used by 'control-points, for example).  Offsets are limited to grob properties listed in the default descriptions found in `scm/define-grobs.scm'.)  Displacements will be reckoned against an override in effect; otherwise, the default setting will be used.  There is some ability for accumulation of offsets.

[david.nalesnik, 2013-04-13 16:46:20]

Please review.

Thanks,
David

[janek, 2013-04-13 21:39:40]

There's one thing that puzzles me.  Current syntax is

\offset property offset-value grob-name

I understand that grob-name is at the end because it's optional, and we want to
omit it when we're using \offset as a tweak. 
However, i find this syntax awkward. Since David K's change that allowed to use
dot-separated list for specyfying grobs "together" with properties, couldn't we
process both the property and grobname as one argument, and therefore keep the
usual order?  In other words, what about syntax like this:

\offset grob-property-path offset-value

where grob-property-path would be either Grob.property (when using \offset as an
override) or just property (when using it as a tweak)?

best,
Janek

BTW, nice comments!

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:11: 
generally, i'd make the regtest shorter.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:40: <c e g b>1-\offset #'positions #'(-2 . 2)
\arpeggio
for example, one instance of arpeggio should suffice here.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:50: \once \offset #'X-offset #0.5 DynamicText
similarly here

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:57: c-\offset #'padding #1.5 \f
ditto

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:102: \once \offset #'Y-offset #2 BreathingSign
ditto

[david.nalesnik, 2013-04-16 12:41:35]

I've considerably shortened the regtests, removing redundancies and unnecessary
line breaks.  Thanks for the comments!

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:11: 
On 2013/04/13 21:39:41, janek wrote:
> generally, i'd make the regtest shorter.

Done.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:40: <c e g b>1-\offset #'positions #'(-2 . 2)
\arpeggio
On 2013/04/13 21:39:41, janek wrote:
> for example, one instance of arpeggio should suffice here.

Done.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:50: \once \offset #'X-offset #0.5 DynamicText
On 2013/04/13 21:39:41, janek wrote:
> similarly here

Done.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:57: c-\offset #'padding #1.5 \f
On 2013/04/13 21:39:41, janek wrote:
> ditto

Done.

https://codereview.appspot.com/8647044/diff/1/input/regression/offsets.ly#new...
input/regression/offsets.ly:102: \once \offset #'Y-offset #2 BreathingSign
On 2013/04/13 21:39:41, janek wrote:
> ditto

Done.

[david.nalesnik, 2013-04-16 12:50:47]

On 2013/04/13 21:39:40, janek wrote:
> There's one thing that puzzles me.  Current syntax is
> 
> \offset property offset-value grob-name
> 
> I understand that grob-name is at the end because it's optional, and we want
to
> omit it when we're using \offset as a tweak. 
> However, i find this syntax awkward. Since David K's change that allowed to
use
> dot-separated list for specyfying grobs "together" with properties, couldn't
we
> process both the property and grobname as one argument, and therefore keep the
> usual order?  In other words, what about syntax like this:
> 
> \offset grob-property-path offset-value
> 
> where grob-property-path would be either Grob.property (when using \offset as
an
> override) or just property (when using it as a tweak)?

Here I kept the same pattern as \alterBroken and \shape, as they have been
revised by David Kastrup.  I agree that the syntax is a little awkward, and that
it would be nice if the pattern you give were workable.  However, IIRC, this
syntax is the only one currently feasible.  I'm not an expert here, though.

[dak, 2013-04-16 13:03:25]

On 2013/04/16 12:50:47, david.nalesnik wrote:
> On 2013/04/13 21:39:40, janek wrote:
> > There's one thing that puzzles me.  Current syntax is
> > 
> > \offset property offset-value grob-name
> > 
> > I understand that grob-name is at the end because it's optional, and we want
> to
> > omit it when we're using \offset as a tweak. 
> > However, i find this syntax awkward. Since David K's change that allowed to
> use
> > dot-separated list for specyfying grobs "together" with properties, couldn't
> we
> > process both the property and grobname as one argument, and therefore keep
the
> > usual order?  In other words, what about syntax like this:
> > 
> > \offset grob-property-path offset-value
> > 
> > where grob-property-path would be either Grob.property (when using \offset
as
> an
> > override) or just property (when using it as a tweak)?
> 
> Here I kept the same pattern as \alterBroken and \shape, as they have been
> revised by David Kastrup.  I agree that the syntax is a little awkward, and
that
> it would be nice if the pattern you give were workable.  However, IIRC, this
> syntax is the only one currently feasible.  I'm not an expert here, though.

For \alterBroken and \shape, the syntax is actually
\shape ... item where item is either music (which is then tweaked) _or_ a grob
(which then gets an override).  The syntax is only due to this double-function.

If \offset does not have the same characteristic (nor intends to have it), then
one should be able to make do with a single specification.

I have not reviewed this so far, so I can't tell.

[janek, 2013-04-16 13:59:17]

On 2013/04/16 13:03:25, dak wrote:
> On 2013/04/16 12:50:47, david.nalesnik wrote:
> > Here I kept the same pattern as \alterBroken and \shape,
> > as they have been revised by David Kastrup.  I agree that
> > the syntax is a little awkward, and that
> > it would be nice if the pattern you give were workable. 
> > However, IIRC, this syntax is the only one currently feasible. 
> > I'm not an expert here, though.
> 
> For \alterBroken and \shape, the syntax is actually
> \shape ... item where item is either music
> (which is then tweaked) _or_ a grob
> (which then gets an override).  The syntax
> is only due to this double-function.
> 
> If \offset does not have the same characteristic (nor intends to have it),
then
> one should be able to make do with a single specification.
> 
> I have not reviewed this so far, so I can't tell.

I've always thought that the syntax of \shape was done
this way because there was no "property" argument (as
it always concerned control-points) - in other words,
\shape's syntax is 

\shape value grobname-or-music

because we need to keep the number and order of arguments 
quite constant. The alternative (specifying grobname before
value) would be bad because then we'd have

\shape grobname-or-nothing value nothing-or-musc

and there's too many "or-nothing"s there (i suppose
that the biggest problem is an optional argument
followed by non-optional one ("value")).  However,
in case of \offset, we can have either a \shape-like
syntax

\offset property value grobname-or-music

or 

\offset grobwithproperty-or-property value nothing-or-music

since the first argument is not optional (i.e there's 
always at least the property to specify), i guess that 
it should be easier to do?

best,
Janek

[dak, 2013-04-16 14:29:54]

> and there's too many "or-nothing"s there (i suppose
> that the biggest problem is an optional argument
> followed by non-optional one ("value")).  However,
> in case of \offset, we can have either a \shape-like
> syntax
> 
> \offset property value grobname-or-music
> 
> or 
> 
> \offset grobwithproperty-or-property value nothing-or-music
> 
> since the first argument is not optional (i.e there's 
> always at least the property to specify), i guess that 
> it should be easier to do?

nothing-or-music in the last position only works if "nothing" is
_explicitly_ specified with \default

There is no way that the last argument of a music function can be
"nothing" silently.

I guess that was the design decision also to be made regarding the
other functions.

[janek, 2013-04-16 14:33:50]

2013/4/16  <dak@gnu.org>:
>> and there's too many "or-nothing"s there (i suppose
>> that the biggest problem is an optional argument
>> followed by non-optional one ("value")).  However,
>> in case of \offset, we can have either a \shape-like
>> syntax
>>
>> \offset property value grobname-or-music
>>
>> or
>>
>> \offset grobwithproperty-or-property value nothing-or-music
>>
>> since the first argument is not optional (i.e there's
>> always at least the property to specify), i guess that
>> it should be easier to do?
>
> nothing-or-music in the last position only works if "nothing" is
> _explicitly_ specified with \default
>
> There is no way that the last argument of a music function can be
> "nothing" silently.

pity.  would it possible (and reasonable) to change this?

JAnek

[dak, 2013-04-16 15:09:02]

On 2013/04/16 14:33:50, janek wrote:
> 2013/4/16  <dak@gnu.org>:

> > nothing-or-music in the last position only works if "nothing" is
> > _explicitly_ specified with \default
> >
> > There is no way that the last argument of a music function can be
> > "nothing" silently.
> 
> pity.  would it possible (and reasonable) to change this?

Uh, a call of offset can be followed by music quite naturally.  So this is not
just a "technical restriction".  It is one of logic.

[janek, 2013-04-21 07:59:11]

Sorry for the delay, got little lilypond-time recently...

2013/4/16  <dak@gnu.org>:
> On 2013/04/16 14:33:50, janek wrote:
>>
>> 2013/4/16  <dak@gnu.org>:
>> > nothing-or-music in the last position only works if "nothing" is
>> > _explicitly_ specified with \default
>> >
>> > There is no way that the last argument of a music function can be
>> > "nothing" silently.
>>
>> pity.  would it possible (and reasonable) to change this?
>
>
> Uh, a call of offset can be followed by music quite naturally.  So this
> is not just a "technical restriction".  It is one of logic.

So, do i understand correctly that \offset could have a syntax like

\offset grob-property-path value music

And then, if grob-property-path contained only property,
the function would be used as a tweak (with "music" being
what's tweaked), and if the function was being used as an
override (grob-property-path contains a grobname), "music"
would be sort-of-unused?

best,
Janek

[dak, 2013-04-21 08:23:34]

Janek Warchoł <janek.lilypond@gmail.com> writes:

> Sorry for the delay, got little lilypond-time recently...
>
> 2013/4/16  <dak@gnu.org>:
>> On 2013/04/16 14:33:50, janek wrote:
>>>
>>> 2013/4/16  <dak@gnu.org>:
>>> > nothing-or-music in the last position only works if "nothing" is
>>> > _explicitly_ specified with \default
>>> >
>>> > There is no way that the last argument of a music function can be
>>> > "nothing" silently.
>>>
>>> pity.  would it possible (and reasonable) to change this?
>>
>>
>> Uh, a call of offset can be followed by music quite naturally.  So this
>> is not just a "technical restriction".  It is one of logic.
>
> So, do i understand correctly that \offset could have a syntax like
>
> \offset grob-property-path value music
>
> And then, if grob-property-path contained only property,
> the function would be used as a tweak (with "music" being
> what's tweaked), and if the function was being used as an
> override (grob-property-path contains a grobname), "music"
> would be sort-of-unused?

Possible, but "sort-of-unused" is not a reasonable interface.

-- 
David Kastrup

[janek, 2013-04-23 13:14:23]

anyway, this patch LTGM.

[david.nalesnik, 2013-04-23 19:03:18]

On 2013/04/23 13:14:23, janek wrote:
> anyway, this patch LTGM.

Thanks, Janek!

The patch has passed countdown, but in view of the fact that a stable release is
on the horizon, it may not be appropriate to push it at this time.

On the other hand, an argument for pushing it right away might be to guard it
against loss of functionality, as happened recently regarding Stem.length and
Y-offset for certain grobs.

What do you think about this?  If it's OK to push, I can supply a patch to
whoever is willing to take it on.

-David

[dak, 2013-04-23 19:38:09]

On 2013/04/23 19:03:18, david.nalesnik wrote:
> On 2013/04/23 13:14:23, janek wrote:
> > anyway, this patch LTGM.
> 
> Thanks, Janek!
> 
> The patch has passed countdown, but in view of the fact that a stable release
is
> on the horizon, it may not be appropriate to push it at this time.
> 
> On the other hand, an argument for pushing it right away might be to guard it
> against loss of functionality, as happened recently regarding Stem.length and
> Y-offset for certain grobs.

Huh?  I don't understand that argument.  On the other hand, the patch is missing
a Changes entry and any integration into the user documentation, so it is mostly
inaccessible to users anyway (and the means to discuss/review its interface
design are limited) and there seems little point to include it in this state. 
If we don't have documentation for the next unstable release, the chances for
translations etc to arrive timely are slim.

[david.nalesnik, 2013-04-23 20:05:46]

On 2013/04/23 19:38:09, dak wrote:
> On 2013/04/23 19:03:18, david.nalesnik wrote:
> > On 2013/04/23 13:14:23, janek wrote:
> > > anyway, this patch LTGM.
> > 
> > Thanks, Janek!
> > 
> > The patch has passed countdown, but in view of the fact that a stable
release
> is
> > on the horizon, it may not be appropriate to push it at this time.
> > 
> > On the other hand, an argument for pushing it right away might be to guard
it
> > against loss of functionality, as happened recently regarding Stem.length
and
> > Y-offset for certain grobs.
> 
> Huh?  I don't understand that argument.  

I mean simply that future changes would need to take the functionality of
\offset into account.  There's no real guarantee that something isn't going to
be pushed between the present and the date the the stable release actually comes
out that won't affect it.

On the other hand, the patch is missing
> a Changes entry and any integration into the user documentation, so it is
mostly
> inaccessible to users anyway (and the means to discuss/review its interface
> design are limited) and there seems little point to include it in this state. 
> If we don't have documentation for the next unstable release, the chances for
> translations etc to arrive timely are slim.

I'm not entirely clear what you mean here.  Are you saying that the patch should
be withdrawn and resubmitted with full documentation--if it is to be considered
for the _stable_ release?

Thanks,
David

[dak, 2013-04-23 20:24:57]

Sorry for the late review.

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
input/regression/offsets.ly:5: the @code{\\offset} command.  These properties
are limited to immutable
What does "immutable" mean here?

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
input/regression/offsets.ly:8: in its default appearance.  The command is used
both as a tweak and an
"demonstrated as a tweak and as an override".  The double "as" is important, and
I'd remove "both" since otherwise the impression arises that it is both at the
same time.

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
scm/c++.scm:30: (every number-pair? x)))
Isn't it dangerous to call "every" on something that is not necessarily a list? 
Like (cons 2 3)?

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so our
search will return
Why would tweak/override add to the _immutable_ properties?  How could they?  Is
there something I don't understand here?

[dak, 2013-04-23 22:09:04]

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so our
search will return
On 2013/04/23 20:24:57, dak wrote:
> Why would tweak/override add to the _immutable_ properties?  How could they? 
Is
> there something I don't understand here?

To answer myself: the basic properties would contain overrides (set there from
the respectively changed context property) but not tweaks (which are applied
after initialization of the grob).

"If our search returns an anonymous procedure" is quite strained.  We don't need
to _speculate_ about the identity of the anonymous procesure.  If we want to be
sure, we can just remember it:

(define (offsetter property offsets)
  (define (self grob)
     ...

and we can now check through both mutable and immutable grob properties, find
self, and look behind self in the aliast to see whether we find another
occurence of the property which, if found, we duly evaluate recursively.  That
should allow multiple offsetter calls to work recursively.

Huh.  Maybe not the best idea since
\offset Y-offset ... Grob
\offset Y-offset ... Grob
would not accumulate while
\offset Y-offset ... Grob
\temporary\offset Y-offset ... Grob
or even
\offset Y-offset ... Grob
\once\offset Y-offset ... Grob
would.

But still seems better than forgetting all overrides except the first one
(likely in the grob definition).

[david.nalesnik, 2013-04-24 11:21:37]

David,

Thank you for your reviews.  It will take me some time to make the changes you
propose, so I've changed the status of the patch to "needs work".

[david.nalesnik, 2013-04-25 13:48:55]

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so our
search will return
On 2013/04/23 22:09:04, dak wrote:
> On 2013/04/23 20:24:57, dak wrote:


> "If our search returns an anonymous procedure" is quite strained.  We don't
need
> to _speculate_ about the identity of the anonymous procesure.  If we want to
be
> sure, we can just remember it:
> 
> (define (offsetter property offsets)
>   (define (self grob)

In view of the way offsetter is called, the first line of the above would still
need to be
(define ((offsetter property offsets) grob)

I see the value of the procedure recognizing itself, but I'm afraid I don't
understand how to implement this from the start you've given me.  Could you give
me a further hint as to what

(define (self grob) ...

would contain?

> 
> and we can now check through both mutable and immutable grob properties, find
> self, and look behind self in the aliast to see whether we find another
> occurence of the property which, if found, we duly evaluate recursively.  That
> should allow multiple offsetter calls to work recursively.
> 
> Huh.  Maybe not the best idea since
> \offset Y-offset ... Grob
> \offset Y-offset ... Grob
> would not accumulate while
> \offset Y-offset ... Grob
> \temporary\offset Y-offset ... Grob
> or even
> \offset Y-offset ... Grob
> \once\offset Y-offset ... Grob
> would.
> 
> But still seems better than forgetting all overrides except the first one
> (likely in the grob definition).

Accumulation of offsets, while nice in theory, does not seem to me to have
practical application.  (And its usage would lead to rather confusing
documentation, judging by the above.)

Here's an example of remembering which is already possible:
\relative c'' {
  c8 d e f
  \offset #'positions #'(-1 . -3) Beam
  c d e f
  \temporary \offset #'positions #'(-5 . -5) Beam
  c d e f
  \revert Beam.positions
  c d e f
}

In view of this, I could simply look for "self" at the head of the properties
alist and use that for comparison, rather than my opaque use of procedure-name.

[david.nalesnik, 2013-10-04 01:17:08]

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
scm/c++.scm:30: (every number-pair? x)))
On 2013/04/23 20:24:57, dak wrote:
> Isn't it dangerous to call "every" on something that is not necessarily a
list? 
> Like (cons 2 3)?

I would think so... However,
(every number-pair? (cons 2 3))
returns #f

[dak, 2013-10-04 05:59:00]

On 2013/10/04 01:17:08, david.nalesnik wrote:
> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
> File scm/c++.scm (right):
> 
> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
> scm/c++.scm:30: (every number-pair? x)))
> On 2013/04/23 20:24:57, dak wrote:
> > Isn't it dangerous to call "every" on something that is not necessarily a
> list? 
> > Like (cons 2 3)?
> 
> I would think so... However,
> (every number-pair? (cons 2 3))
> returns #f

Well, relying on undefined behavior that happens to do what we want now seems
imprudent.

[dak, 2013-10-04 05:59:15]

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so our
search will return
On 2013/04/25 13:48:55, david.nalesnik wrote:
> On 2013/04/23 22:09:04, dak wrote:
> > On 2013/04/23 20:24:57, dak wrote:
> 
> 
> > "If our search returns an anonymous procedure" is quite strained.  We don't
> need
> > to _speculate_ about the identity of the anonymous procesure.  If we want to
> be
> > sure, we can just remember it:
> > 
> > (define (offsetter property offsets)
> >   (define (self grob)
> 
> In view of the way offsetter is called, the first line of the above would
still
> need to be
> (define ((offsetter property offsets) grob)

Not at all.

> I see the value of the procedure recognizing itself, but I'm afraid I don't
> understand how to implement this from the start you've given me.  Could you
give
> me a further hint as to what
> 
> (define (self grob) ...
> 
> would contain?

The same as it does now, except that it can compare the callbacks in
the alist to "self" with "eq?".

It is probably more interesting how the function offsetter is then ended:

(define (offsetter property offsets)
  (define (self grob) .............)
  self)

The basics, namely having a function returning a function (or rather a
closure since it uses "property" and "offsets" internally) remain the
same.  It is just that the closure is no longer anonymous but rather
called "self" (or whatever else) and thus is able to recognize itself.

[david.nalesnik, 2013-10-06 01:15:12]

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
input/regression/offsets.ly:5: the @code{\\offset} command.  These properties
are limited to immutable
On 2013/04/23 20:24:57, dak wrote:
> What does "immutable" mean here?

Hopefully this rewrite is less opaque.

https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
input/regression/offsets.ly:8: in its default appearance.  The command is used
both as a tweak and an
On 2013/04/23 20:24:57, dak wrote:
> "demonstrated as a tweak and as an override".  The double "as" is important,
and
> I'd remove "both" since otherwise the impression arises that it is both at the
> same time.

Done.

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
scm/c++.scm:30: (every number-pair? x)))
On 2013/10/04 01:17:08, david.nalesnik wrote:
> On 2013/04/23 20:24:57, dak wrote:
> > Isn't it dangerous to call "every" on something that is not necessarily a
> list? 
> > Like (cons 2 3)?
> 
> I would think so... However,
> (every number-pair? (cons 2 3))
> returns #f

Fixed.  No more reliance on undefined behavior.

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so our
search will return
Thank you for your explanations!

On 2013/10/04 05:59:15, dak wrote:

> It is probably more interesting how the function offsetter is then ended:
> 
> (define (offsetter property offsets)
>   (define (self grob) .............)
>   self)

This is what stumped me.  I was attempting to return (self grob).
--------
There is a major problem with this patch set which I don't know how to address.

The examples below represent my efforts to test the effects of multiple
applications of \offset.  You can see that some accumulation is possible. 
However, the last two examples (commented out) will cause a crash.  All I can
think of is that this relates to the multiple instances of "self" in the
properties alist.  I've verified that the instances aren't identical despite
having the same name, so I don't understand why there should be a problem.

%%%%
\relative c' {
  %% TESTS FOR ACCUMULATION %%
  
  % default
  <c e g b>1\arpeggio
  
  \override Arpeggio.positions = #'(-3.5 . 0.5)
  <c e g b>1\arpeggio
  
  % values created by override are offset
  \offset #'positions #'(-2 . 2) Arpeggio
  <c e g b>1\arpeggio
  
  % This is the result of new offset and override still in effect;
  % two offsets have not accumulated.
  \offset #'positions #'(-1 . 1) Arpeggio
  <c e g b>1\arpeggio
  
  % override + last offset + offset tweak
  <c e g b>1-\offset #'positions #'(-0.5 . 0.5) \arpeggio 
}

\relative c' {
  %% MORE TESTS FOR ACCUMULATION %%
  
  % default
  <c e g b>1\arpeggio
  
  \once \offset #'positions #'(-2 . 2) Arpeggio
  <c e g b>1\arpeggio
  
  % This accumulates:
  \offset #'positions #'(-2 . 2) Arpeggio
  <c e g b>1-\offset #'positions #'(-2 . 2) \arpeggio
  
  %%%%%%%%%%%%%%%%%%% This causes a crash:
  
  %\offset #'positions #'(-2 . 2) Arpeggio
  %\once \offset #'positions #'(-2 . 2) Arpeggio
  %<c e g b>1\arpeggio
  
  %%%%%%%%%%%%%%%%%% This causes a crash:
  %\once \offset #'positions #'(-2 . 2) Arpeggio
  %\temporary \offset #'positions #'(-2 . 2) Arpeggio
  %<c e g b>1\arpeggio
}

[lemzwerg, 2013-10-06 06:06:40]

LGTM.

Maybe you can somewhere add a sentence like

  Note that \tweak and friends provide absolute positioning, not relative.

to the documentation.  It took me a while to recognize the difference.

[dak, 2013-10-06 09:35:56]

Just before I forget: this is not a real comment on this issue.

David N posted some update on this issue maybe a week ago (details are not all
that important) after it lay dormant for months.  I replied to that issue, and
then figured I apparently didn't properly reply since Rietveld flagged
"unpublished drafts".  So I did the "Publish all my drafts" routine.

Turns out that the unpublished drafts were detailed advice to technical
questions that I wrote months ago, about the time when the issue became dormant.
 And it probably became dormant not least of all because David N actually could
have moved forward with that information.

It's not the first time that I had accidentally let reviews of mine go
unpublished: for some reason, Rietveld is designed to require an additional step
after a step that already quite feels like being the final step to me.

What to make of that?  No idea.

[david.nalesnik, 2013-10-07 13:31:48]

On 2013/10/06 06:06:40, lemzwerg wrote:
> LGTM.
> 
> Maybe you can somewhere add a sentence like
> 
>   Note that \tweak and friends provide absolute positioning, not relative.
> 
> to the documentation.  It took me a while to recognize the difference.

OK--will do.  (I just made some changes to the Rietveld description so at least
it's clearer to developers looking at the issue.)

[dak, 2013-10-07 14:10:09]

On 2013/10/06 01:15:12, david.nalesnik wrote:
> https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly
> File input/regression/offsets.ly (right):
> 
>
https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
> input/regression/offsets.ly:5: the @code{\\offset} command.  These properties
> are limited to immutable
> On 2013/04/23 20:24:57, dak wrote:
> > What does "immutable" mean here?
> 
> Hopefully this rewrite is less opaque.
> 
>
https://codereview.appspot.com/8647044/diff/5001/input/regression/offsets.ly#...
> input/regression/offsets.ly:8: in its default appearance.  The command is used
> both as a tweak and an
> On 2013/04/23 20:24:57, dak wrote:
> > "demonstrated as a tweak and as an override".  The double "as" is important,
> and
> > I'd remove "both" since otherwise the impression arises that it is both at
the
> > same time.
> 
> Done.
> 
> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm
> File scm/c++.scm (right):
> 
> https://codereview.appspot.com/8647044/diff/5001/scm/c++.scm#newcode30
> scm/c++.scm:30: (every number-pair? x)))
> On 2013/10/04 01:17:08, david.nalesnik wrote:
> > On 2013/04/23 20:24:57, dak wrote:
> > > Isn't it dangerous to call "every" on something that is not necessarily a
> > list? 
> > > Like (cons 2 3)?
> > 
> > I would think so... However,
> > (every number-pair? (cons 2 3))
> > returns #f
> 
> Fixed.  No more reliance on undefined behavior.
> 
> https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm
> File scm/music-functions.scm (right):
> 
>
https://codereview.appspot.com/8647044/diff/5001/scm/music-functions.scm#newc...
> scm/music-functions.scm:2103: ; head of the alist.  We reverse the alist so
our
> search will return
> Thank you for your explanations!
> 
> On 2013/10/04 05:59:15, dak wrote:
> 
> > It is probably more interesting how the function offsetter is then ended:
> > 
> > (define (offsetter property offsets)
> >   (define (self grob) .............)
> >   self)
> 
> This is what stumped me.  I was attempting to return (self grob).
> --------
> There is a major problem with this patch set which I don't know how to
address.
> 
> The examples below represent my efforts to test the effects of multiple
> applications of \offset.  You can see that some accumulation is possible. 
> However, the last two examples (commented out) will cause a crash.  All I can
> think of is that this relates to the multiple instances of "self" in the
> properties alist.  I've verified that the instances aren't identical despite
> having the same name, so I don't understand why there should be a problem.

I suppose the problem is with
+(define (find-value-to-offset prop self alist)
+  "Return the first value of the property @var{prop} in the property alist @var
+that is not @var{self}."
+  (let loop ((ls alist))
+    (if (null? ls)
+        #f
+        (if (eq? prop (car (first ls)))
+            (if (eq? self (cdr (first ls)))
+                (loop (cdr ls))
+                (cdr (first ls)))
+            (loop (cdr ls))))))

First: try using GUILE srfi-1 functions for that kind of thing: they are more
readable than explicit loops.

And nice to have accurate documentation: this tells us exactly what is wrong:

+  "Return the first value of the property @var{prop} in the property alist @var
+that is not @var{self}."

But what you need here is "return the first value of the property @var{prop} in
the property
alist @var{alist} @em{after} having found @var{self}."

You _always_ have to _first_ pass self before looking for anything else.  If you
find a valid property _before_ finding self, ignore it and continue looking. 
Otherwise you end up in a mutual recursion.

If you don't find self at all, its head-scratching time: after all, who called
you then?  I lean toward throwing an error: continuation strategies might be
fragile.

[dak, 2013-10-07 14:10:31]

https://codereview.appspot.com/8647044/diff/35001/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/35001/input/regression/offsets.ly...
input/regression/offsets.ly:31: 
Spurious space in otherwise empty line.

[dak, 2013-10-07 14:13:31]

https://codereview.appspot.com/8647044/diff/35001/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/8647044/diff/35001/scm/c++.scm#newcode30
scm/c++.scm:30: (not (null? x))
Why not null?  An empty list is also a list, so this makes the predicate a bit
of a misnomer.

[david.nalesnik, 2013-10-07 17:01:00]

https://codereview.appspot.com/8647044/diff/35001/input/regression/offsets.ly
File input/regression/offsets.ly (right):

https://codereview.appspot.com/8647044/diff/35001/input/regression/offsets.ly...
input/regression/offsets.ly:31: 
On 2013/10/07 14:10:32, dak wrote:
> Spurious space in otherwise empty line.

Done.

https://codereview.appspot.com/8647044/diff/35001/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/8647044/diff/35001/scm/c++.scm#newcode30
scm/c++.scm:30: (not (null? x))
On 2013/10/07 14:13:32, dak wrote:
> Why not null?  An empty list is also a list, so this makes the predicate a bit
> of a misnomer.

OK, above condition removed so predicate returns #t for empty list.

[david.nalesnik, 2013-10-07 17:13:23]

On 2013/10/07 14:10:09, dak wrote:
> On 2013/10/06 01:15:12, david.nalesnik wrote:

> > There is a major problem with this patch set which I don't know how to
> address.
> > 
> > The examples below represent my efforts to test the effects of multiple
> > applications of \offset.  You can see that some accumulation is possible. 
> > However, the last two examples (commented out) will cause a crash.  All I
can
> > think of is that this relates to the multiple instances of "self" in the
> > properties alist.  I've verified that the instances aren't identical despite
> > having the same name, so I don't understand why there should be a problem.
> 
> I suppose the problem is with
> +(define (find-value-to-offset prop self alist)
> +  "Return the first value of the property @var{prop} in the property alist
@var
> +that is not @var{self}."
> +  (let loop ((ls alist))
> +    (if (null? ls)
> +        #f
> +        (if (eq? prop (car (first ls)))
> +            (if (eq? self (cdr (first ls)))
> +                (loop (cdr ls))
> +                (cdr (first ls)))
> +            (loop (cdr ls))))))
> 
> First: try using GUILE srfi-1 functions for that kind of thing: they are more
> readable than explicit loops.

I've used member, and I've also incorporated Lily's assoc-get.

> 
> But what you need here is "return the first value of the property @var{prop}
in
> the property
> alist @var{alist} @em{after} having found @var{self}."
> 
> You _always_ have to _first_ pass self before looking for anything else.  If
you
> find a valid property _before_ finding self, ignore it and continue looking. 
> Otherwise you end up in a mutual recursion.

Changes made--works like a charm!  There are now more possibilities for
accumulation.  Possibly another regtest `offset-accumulation.ly' is warranted? 
(As far as documenting the abilities for accumulation: this may be more
confusing than worthwhile.)
> 
> If you don't find self at all, its head-scratching time: after all, who called
> you then?  I lean toward throwing an error: continuation strategies might be
> fragile.

Well, if self isn't found, then we're dealing with a tweak, which wouldn't have
added to the alist.  In that case, the function `find-value-to-offset' as
rewritten will simply return the first instance of the property in the alist. 
`offsetter' should catch situations where the user tries to offset something not
in the basic properties alist.

[dak, 2013-10-07 19:02:47]

https://codereview.appspot.com/8647044/diff/48001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/8647044/diff/48001/ly/music-functions-init.ly#...
ly/music-functions-init.ly:705: (if (ly:music? item)
Ok, I am annoyed at the necessity for those distinctions.  Issue 3603 might
help.  Not sure about the error message quality, though.

[janek, 2013-10-15 18:04:43]

Hi,

an afterthought.

On 2013/10/06 01:15:12, david.nalesnik wrote:
> The examples below represent my efforts to test the effects of multiple
> applications of \offset.  You can see that some accumulation is possible. 
> [...] 
> %%%%
> \relative c' {
>   %% TESTS FOR ACCUMULATION %%
>   
>   % default
>   <c e g b>1\arpeggio
>   
>   \override Arpeggio.positions = #'(-3.5 . 0.5)
>   <c e g b>1\arpeggio
>   
>   % values created by override are offset
>   \offset #'positions #'(-2 . 2) Arpeggio
>   <c e g b>1\arpeggio

I'm not sure if i haven't missed something, but to your realize that in this
case the offset isn't applied on top of the override (as the comment suggests),
but replaces it?  This is self-evident in the example below:

\relative c' {
  % default
  <c e g b>1\arpeggio
  
  \override Arpeggio.positions = #'(-5 . 5)
  <c e g b>1\arpeggio
  
  \offset #'positions #'(-1 . 2) Arpeggio
  <c e g b>1\arpeggio
}
  
with current master (a82d8622e6b1be36169de7d2fe1f9aa88618933b, containing offset
patch) the last arpeggio is shorter than the second, while it should be longer
in both directions.

best,
Janek

[dak, 2013-10-15 18:19:58]

On 2013/10/15 18:04:43, janek wrote:
> Hi,
> 
> an afterthought.
> 
> On 2013/10/06 01:15:12, david.nalesnik wrote:
> > The examples below represent my efforts to test the effects of multiple
> > applications of \offset.  You can see that some accumulation is possible. 
> > [...] 
> > %%%%
> > \relative c' {
> >   %% TESTS FOR ACCUMULATION %%
> >   
> >   % default
> >   <c e g b>1\arpeggio
> >   
> >   \override Arpeggio.positions = #'(-3.5 . 0.5)
> >   <c e g b>1\arpeggio
> >   
> >   % values created by override are offset
> >   \offset #'positions #'(-2 . 2) Arpeggio
> >   <c e g b>1\arpeggio
> 
> I'm not sure if i haven't missed something, but to your realize that in this
> case the offset isn't applied on top of the override (as the comment
suggests),
> but replaces it?  This is self-evident in the example below:
> 
> \relative c' {
>   % default
>   <c e g b>1\arpeggio
>   
>   \override Arpeggio.positions = #'(-5 . 5)
>   <c e g b>1\arpeggio
>   
>   \offset #'positions #'(-1 . 2) Arpeggio
>   <c e g b>1\arpeggio
> }
>   
> with current master (a82d8622e6b1be36169de7d2fe1f9aa88618933b, containing
offset
> patch) the last arpeggio is shorter than the second, while it should be longer
> in both directions.

Well, that's the slightly icky thing.  The \offset is with respect to the
situation you'll get when doing \revert Arpeggio.positions afterwards.  So if
you want to make a _relative_ offset to the first setting, you have to do
\temporary \offset #'positions #'(-1 . 2) Arpeggio

I'm not overly enthusiastic about how the \offset command ends up when entered
as an override.  Maybe we should allow
\offset #'(-1 . 2) Arpeggio.positions here?  It seems that the offset list
should not be confusable with a symbol? or whatever is used in the front?  So we
could make that first argument optional, and complain when the last argument is
not a symbol list making up for it?

[janek, 2013-10-15 18:42:52]

2013/10/15  <dak@gnu.org>:
> On 2013/10/15 18:04:43, janek wrote:
> Well, that's the slightly icky thing.  The \offset is with respect to
> the situation you'll get when doing \revert Arpeggio.positions
> afterwards.  So if you want to make a _relative_ offset to the first
> setting, you have to do
> \temporary \offset #'positions #'(-1 . 2) Arpeggio
>
> I'm not overly enthusiastic about how the \offset command ends up when
> entered as an override.  Maybe we should allow
> \offset #'(-1 . 2) Arpeggio.positions here?  It seems that the offset
> list should not be confusable with a symbol? or whatever is used in the
> front?  So we could make that first argument optional, and complain when
> the last argument is not a symbol list making up for it?

No idea :-(

[david.nalesnik, 2013-10-20 15:51:57]

https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm#new...
scm/music-functions.scm:2118: (let* ((immutable (ly:grob-basic-properties grob))
I just noticed something unfortunate while attempting to write documentation for
\offset.  Basically, `property' needs to be specified using the older notation
-- i.e., with #' prefixed.  A quick fix would be to add the line:

(property (if (symbol-list? property) (car property) property)))

to the let-block here.  I don't know if that's adequate, however.

[dak, 2013-10-20 16:15:56]

On 2013/10/20 15:51:57, david.nalesnik wrote:
> https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm
> File scm/music-functions.scm (right):
> 
>
https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm#new...
> scm/music-functions.scm:2118: (let* ((immutable (ly:grob-basic-properties
grob))
> I just noticed something unfortunate while attempting to write documentation
for
> \offset.  Basically, `property' needs to be specified using the older notation
> -- i.e., with #' prefixed.  A quick fix would be to add the line:
> 
> (property (if (symbol-list? property) (car property) property)))
> 
> to the let-block here.  I don't know if that's adequate, however.

I suggest looking at the code in issue 3625.  That "sloppy" approach let's you
concatenate the material first (requiring the symbol? check, though) and then
pull the whole thing through the property path checker.  In this case, it would
appear that you want to call it with #:min 3 #:max 3 #:default 'Bottom as
anything but a toplevel property would be a cause for problems.

And you should be able to get at your property using (third p) or so.  A similar
recipe should be good for tweaks, just using a different #:start value for the
check.

[david.nalesnik, 2013-10-20 20:35:20]

On 2013/10/20 16:15:56, dak wrote:
> On 2013/10/20 15:51:57, david.nalesnik wrote:
> > https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm
> > File scm/music-functions.scm (right):
> > 
> >
>
https://codereview.appspot.com/8647044/diff/48001/scm/music-functions.scm#new...
> > scm/music-functions.scm:2118: (let* ((immutable (ly:grob-basic-properties
> grob))
> > I just noticed something unfortunate while attempting to write documentation
> for
> > \offset.  Basically, `property' needs to be specified using the older
notation
> > -- i.e., with #' prefixed.  
> 
> I suggest looking at the code in issue 3625.  That "sloppy" approach let's you
> concatenate the material first (requiring the symbol? check, though) and then
> pull the whole thing through the property path checker.  In this case, it
would
> appear that you want to call it with #:min 3 #:max 3 #:default 'Bottom as
> anything but a toplevel property would be a cause for problems.
> 
> And you should be able to get at your property using (third p) or so.  A
similar
> recipe should be good for tweaks, just using a different #:start value for the
> check.

OK, that makes sense.  I've rewritten offset according to these guidelines.  A
side benefit
is that it was no trouble to allow directed tweaks, so the following is
possible:

{
  <c'' e'' \offset AccidentalPlacement.right-padding #2 ges''!>4
  <c'' e'' \offset X-offset #-2 ges''!>4
}

Tweaks following tweaks work, too:

{
  <c'' e''
  \offset AccidentalPlacement.right-padding #2
  \offset X-offset #-2 ges''!>4
}

Here's what I come up for offset:

offset =
#(define-music-function (parser location property offsets item)
  (symbol-list-or-symbol? scheme? symbol-list-or-music?)
  (_i "Offset the default value of @var{property} of @var{item} by
@var{offsets}.  If @var{item} is a string, the result is
@code{\\override} for the specified grob type.  If @var{item} is
a music expression, the result is the same music expression with an
appropriate tweak applied.")
  (if (ly:music? item)
      ; In case of a tweak, our grob property path
      ; is Grob.property.
      (let ((prop-path (check-grob-path
                          (if (symbol? property)
                              (list property)
                              property)
                          parser location
                          #:start 1 #:default #t #:min 2 #:max 2)))
        (if prop-path
            ; If the head of the grob property path is a
            ; symbol--i.e., a grob name--produce a directed
            ; tweak.  Otherwise, create an ordinary tweak.
            (if (symbol? (car prop-path))
                #{
                  \tweak #prop-path
                         #(offsetter (second prop-path) offsets) #item
                #}
                #{
                  \tweak #(second prop-path)
                         #(offsetter (second prop-path) offsets) #item
                #})
            (make-music 'Music)))
      ; In case of an override, our grob property path
      ; is Context.Grob.property
      (let ((prop-path (check-grob-path
                          (append item
                                  (if (symbol? property)
                                      (list property)
                                      property))
                          parser location
                          #:default 'Bottom #:min 3 #:max 3)))
        (if prop-path
            #{
              \override #prop-path =
                #(offsetter (third prop-path) offsets)
            #}
            (make-music 'Music)))))

%%%%%%
Thank you very much for your help!

[dak, 2013-10-20 21:08:00]

On 2013/10/20 20:35:20, david.nalesnik wrote:
> 
> OK, that makes sense.  I've rewritten offset according to these guidelines.  A
> side benefit
> is that it was no trouble to allow directed tweaks, so the following is
> possible:
> 
> {
>   <c'' e'' \offset AccidentalPlacement.right-padding #2 ges''!>4
>   <c'' e'' \offset X-offset #-2 ges''!>4
> }

Oh.  Indeed.