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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel