Re: Issue 4182: avoid checking the offset of cross-staff stems too early (issue 554030043 by address@hidden)

[barrykp]

On 2020/05/10 15:49:43, Dan Eble wrote:
> This is an area of LilyPond that I don't understand very well, so feel
free to
> ignore this question if the answer should be obvious.  This patch
removes a call
> to get_property (stem, "positioning-done") for cross-staff stems, but
doesn't
> add an alternative anywhere.  Is something else already calling it at
an
> appropriate time for cross-staff stems?  Speaking from general
programming
> experience and looking at the change narrowly, I wonder whether a
state machine
> is now left dangling in a state it shouldn't be.

I'd be lying if I said this was an area that I understand very well
either. I did
try to research the code that this modifies. It's from 2005 (except for
one recent
syntax change). The commit message doesn't make any mention of it (it's
a big
commit). Surrounding commits have no log messages (pre-git days I
guess).

Comments in lily/stem.cc ("WARNING: triggers direction") suggest that
checking the
direction of a Stem is sometimes not desirable. Quite why the X-offset
of a
NoteHead runs a callback for the Stem before returning
0 isn't clear. My gut feeling is it could be removed altogether with no
ill effects.

The chain is:
NoteHead:stem_x_shift
-> Stem::calc_positioning_done
   -> get_grob_direction
You can simulate the complete removal of this today in any score by
explicitly
setting the X-offset property of NoteHeads to 0. This prevents
Stem::calc_positioning_done from being called (I confirmed this with
gdb).

The property will be checked/set by some other mechanism, but not via
calc_positioning_done. A few stack traces in testing suggest that the
Stem offset
will be calculated when whatever axis group contains it calculates its
own extents.

So the right thing to do might be to set NoteHead.X-offset to 0 and
remove the
stem_x_shift callback altogether. This small patch just aims to fix the
critical
regression, so I didn't test that.

https://codereview.appspot.com/554030043/