Re: Tied accidentals in chords still taking up space (was: Issue 415 in lilypond: Hidden accidental of tied note still takes space)

[Joe Neeman]

On Sat, 2009-03-28 at 16:16 -0300, Han-Wen Nienhuys wrote:
> On Thu, Mar 26, 2009 at 3:21 PM, Joe Neeman <address@hidden> wrote:
> >> > One solution might be the following, which involves modifying only
> >> > accidental-placement: make 2 copies of every accidental, one for the
> >> > start-of-a-line case and one for the middle-of-a-line case (so the
> >> > start-of-a-line accidentals would be, in the terminology of
> >> > Accidental_placement::split_accidentals, the break_reminder accidentals
> >> > and the real_accidentals while the middle-of-a-line accidentals would
> >> > have copies of only the real_accidentals). Run the accidental layout
> >> > algorithm (currently in Accidental_placement::calc_positioning_done, but
> >> > you'll want to move it to a different function and make
> >> > calc_positioning_done call it twice) twice, once for each set of
> >> > accidentals. Then modify Accidental_placement::get_relevant_accidentals
> >> > to return one of the sets of accidentals.
> >>
> >> I've looked into this approach, and have a few questions:
> >>
> >> I tried cloning the accidentals in Accidental_placement::add_accidental,
> >> but that led to a segfault later on while trying to calculate the
> >> X-extent of the cloned accidental, since its layout_ was null. From what
> >> I can tell, this is because the cloned grob was never announced. Perhaps
> >> I should modify accidental-engraver as well, and have it create two
> >> copies there, passing them both to Accidental_placement::add_accidental?
> >
> > It's not necessary to announce every grob (for example, of the three
> > copies of every breakable grob, only the middle-of-line copy gets
> > announced). The code for creating these non-announced grobs is in
> > Item::copy_breakable_items. In particular, you need the
> > get_root_system(original_grob)->typeset_grob(cloned_grob) line to give
> > your clone a layout_.
> 
> This is not applicable here: the accidentals themselves are not on
> linebreaks, spacing wise, so there should be only one copy of them.
> Also, I seem to recall that we already have support for this in the
> form of  conditional elements (grep for conditional) in skyline and
> separation items.  Why aren't we using or extending that?

This is essentially an extension of the idea of conditional accidentals;
see below.

> 
> The last time I looked, I had to conclude the wenot can ever
> completely get correct behavior for tie-accidental reminder; I forgot
> the details though.  Can you point me to the discussion that led to
> this patch?  It seems that the conversation is fragmented over several
> threads.

The discussion began in Issue 415 on the bug tracker, here is a
reasonably detailed suggestion from me about how to address the issue:
http://article.gmane.org/gmane.comp.gnu.lilypond.bugs/14943

The problem is that the current support for conditional accidentals is
insufficient if there are multiple accidentals, not all of which are
tied (see bug 612). The reason is that the positions of all the
accidentals can change completely when an accidental is added or
removed. So I think we need to run the whole layout algorithm twice,
once with the conditional accidentals and once without, and I suggested
that copying all of the accidentals is the simplest way to do it. I
realize that this goes against the convention of only cloning items in
the breakable columns, but I don't see another way to do it; the
formatting of such accidentals seems to be completely tied to their
break status which justifies copying them for the same reason that we
copy items in the breakable columns.

> I've glossed over the patch, and I am less than enthused: it seems to
> add a lot of special casing and duplicate code for the reminder and
> normal accidentals, and I have seen very dubious constructs like
> get_property("accidental-grob") rather than get_object.

I'll review the code shortly; I agree that it should be get_object.

Joe