[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Caches the interior skylines of vertical axis groups and systems. (i
From: |
address@hidden |
Subject: |
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044) |
Date: |
Mon, 18 Feb 2013 22:32:25 +0200 |
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
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044), dak, 2013/02/19
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044), dak, 2013/02/23
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044), dak, 2013/02/23
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044), tdanielsmusic, 2013/02/23
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044), k-ohara5a5a, 2013/02/24