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

[address@hidden]

On 18 févr. 2013, at 20:07, address@hidden wrote:

> 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.

Currently, the previous functionality that breaks is that setting 
outside-staff-priority by itself no longer has an effect, it must be 
accompanied by a function that uses this to compute Y-offset, which is now in 
the side-position-interface.

The reason is best summarized by Keith, which I've copied and pasted below:

<snip>
The decision-making is still top-down in the sorting, bottom-up in
choosing padding, as it was before.  The change is that evaluation of
Y-offset of any grob initiates the decision-making, and decisions are
stored in properties of the appropriate grobs (grouping- or graphical-
objects) so that any of these properties could be reset to its callback
function, and evaluated again.
</snip>

Please let me know what other information you (or anyone) needs to understand 
what is going on.  It is an important change, and I want to make sure it is 
properly discussed on this list, documented, commented and well written before 
it is pushed.

> 
>> 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.

Could you elaborate?

> 
>> 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.

 It is true that this breaks old functionality for user overrides.

The goal is certainly not to mask things.  I will make sure to put this in the 
change log and will write a not-smart convert-ly rule in my next patch set.

Cheers,
MS