On 18 févr. 2013, at 20:07, d...@gnu.org 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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
