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

[dak]

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/