Re: Add the command \offset to LilyPond (issue 8647044)

[david . nalesnik]

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.



https://codereview.appspot.com/8647044/