Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)

[dak]

On 2013/02/15 06:37:20, mike7 wrote:


https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15
> > input/regression/tuplet-number-outside-staff-positioning.ly:15:
> > \override TupletBracket.Y-offset =
> > #ly:side-position-interface::calc-outside-staff-y-offset
> > This is indicating a fundamental design problem: this override is
not
> > related to the topic of the regtest.  If it is necessary for getting
the
> > desired result, it will be necessary in a whole _lot_ of
user-created
> > situations.  But it is not an easily user-accessibly override, and
it is
> > not documented in normal user documentation.
> >
> > This suspiciously looks like tampering with unrelated regtests in
order
> > to mask fundamental problems from review.  Can you explain why this
is
> > not the case?

> Your question gets to the core of the logic of the patch, so I'll
> explain it and then people can comment upon how that links up with
> this regtest.

> In LilyPond, about 85% of grob properties are set as the result of the
> evaluation of a callback or the use of a rote value.

[...]

Mike, that's a whole bunch of smoke screen.  The fact is that your
change, which purports to be just some "factoring" of stuff by its
description, breaks existing and documented functionality.

And you offer no reason for that.

> Now, LilyPond, for various callbacks, other properties must be set
> for those callbacks to make sense.  For example, if I do:

> \override NoteHead #'stencil = #ly:text-interface::print

> Nothing will happen.  But if I do:

> \override NoteHead #'text = #"foo"
> \override NoteHead #'stencil = #ly:text-interface::print

> The NoteHead will be printed as foo.  This is exactly what we're
> seeing in the regtest tuplet-number-outside-staff-positioning.ly.

No, it is not.

> A callback is set for Y-offset that allows the grob to be positioned
> outside the staff.  But, just as the text interface needs to know
> what text to print, the side-position-interface needs to know what
> outside-staff-priority to use to place the grob.  Thus the use of
> two overrides instead of one.

You got your logic backwards.  The _preexisting_ override in the
regtest overrides outside-staff-priority.  This is a documented way of
changing the default order of outside-staff elements.  There are 17
grobs with a preassigned outside-staff-priority.  There are 369
occurences of outside-staff-priority in the LilyPond code base.

You break that.  You use callbacks that ignore outside-staff-priority
and thus break existing functionality.  And then you revert to the
previous callbacks in the regtests in order to mask that you are
breaking functionality.

You can't just throw functionality overboard when you are "improving"
things and tell people they have to revert to the old code if they
care about that functionality.  After all, it is totally unclear how
elements with the old callbacks and elements with the new
outside-staff-priority ignoring callbacks will even combine.

> A music function could be done in the form of:

> \addOutsideStaffPriorityToGrob #'TupletBracket #100

> that rolls the two overrides into one.  This is a UI issue about
> which I have not thought yet but that absolutely deserves attention.

Then give it attention.  Heed outside-staff-priority properly in your
versions of the callbacks.  Until this is done, this patch is not
ready for maintime.

This is a showstopper.

And messing with the regtests in order to hide this is not a proper
way of addressing this showstopper.


https://codereview.appspot.com/7185044/