On 2013/02/19 14:57:48, mike7 wrote:
The current documentation on outside-staff grobs reads: "Intuitively, there are some objects in musical notation that belong to the staff and there are other objects that should be placed outside the staff. Objects belonging outside the staff include things such as rehearsal marks, text and dynamic markings (from now on, these will be called outside-staff objects). LilyPond’s rule for the vertical placement of outside-staff objects is to place them as close to the staff as possible but not so close that they collide with another object."
If we accept this definition of outside-staff grobs, then this patch correctly typesets all outside staff grobs.
"such as" is not a definition.
The main thing this patch effects are esoteric grobs that normally don't get outside-staff-priority, like MultiMeasureRest, NoteHeads and Rests.
The tampered regtests affect TupletBracket and TupletNumber. Your grob definitions change BassFigureLine, DynamicText, MeasureCounter, System, VerticalAxisGroup, and that's all. So why are tuplet brackets even affected? Your issue description is "Caches the interior skylines of vertical axis groups and systems. This will allow for a more modular approach to outside-staff-spacing, eventually eliminating the call to translate_axis in axis-group-interface.cc." "Caching" means a technique for avoiding repeated calculations by storing intermediate results. Caching does _not_ change results. So obviously, the issue description is blatantly misleading. And there are no discoverable design comments that would explain that discrepancy. The definition of TupletBracket has _not_ changed, yet it has stopped reacting to outside-staff-priority. That means that _every_ unchanged grob will also stop reacting to outside-staff-priority. You don't document what it takes to _keep_ adhering to outside-staff-priority. This was previously something that worked without extra measures. What does it take now? Apparently something like \override TupletBracket.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset Now why is that not the default provided by the interfaces that have provided it previously?
In light of this, what type of additions to the NR do you feel would be necessary to indicate the new behavior?
First, clear the situation up for the programmer. It very much looks like you are trying to make things hard for the documentation writers and users because you can't be bothered to add what it takes to, indeed, make your code only "cache" something without changing the user-visible behavior. At the current point of time, the complete mismatch between the issue/codereview description and the actual content is already enough to render it unreviewable. And indeed, I have not properly reviewed it. I am just poking around with a stick in the code, and even that turns out enough things that make it _very_ obvious that this patch is in need of thorough review, and the provided materials are not sufficient for a thorough review, and not consistent. It is taking _enormous_ amounts of time to hunt this kind of stuff through the dark. And it taking an enormous drain of energy because at every point one feels stupid and helpless, and the reason for that is that the information you are venturing through code and comments is quite insufficient to get a picture of what you are doing in the first place, and the issue "description" does not help one bit. Now obviously it should not contain anything of relevance for understanding that is not also present in code comments (after all, the code has to be read without the issue description), but that does not mean that the remedy for omitting the design from the code would be to make a misleading issue description. I don't have the time and energy to go through the process of what amounts to disassembling code. And anybody needing to maintain the code itself and anybody having to deal with the resulting behavior will also not have this time. I _have_ in my younger years disassembled code. One of the best-written programs I have had the pleasure to peruse was a Z80 assembly language implementation of Reversi. In the old times, people passed around code, and I don't know who was the original author. But the code was textbook quality, a very clean and straightforward implementation of Alpha-Beta pruning, straightforward layout of scoring tables, consistent use of index registers for certain tasks, every piece of code a straightforward rendition of a straightforward task fitting in the overall design, everything as simple and direct as possible. It was _rewarding_ to be reading the mind of a master. These reviews, in contrast, are not rewarding. I never get the impression that I can see the mind of the surgeon dissecting a problem along its principal lines, making it fall into the obvious and necessary pieces. And so you need to document and describe what you are doing because otherwise the whole review procedure is a pure mockery and code gets "accepted" when everybody has given up understanding it. https://codereview.appspot.com/7185044/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
