bug#51550: 29.0.50; Customize Group INS buttons sometimes don't have a left box line

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  stephen.berman@gmx.net,
>   51550@debbugs.gnu.org
> Date: Mon, 27 Dec 2021 18:50:22 +0800
> 
> I think I found the bug, it's in display_line.
> 
> When the label at_end_of_line is reached, it->start_of_box_run_p can be
> false.
> 
> Afterwards, set_iterator_to_next is called, which reseats the iterator
> to the next line, but doesn't set it->start_of_box_run_p if the face is
> now different from the previous face and also has a box.
> 
> I think the solution is to save the face ID of the iterator after the
> call to extend_face_to_end_of_line, then compare it to the face after
> the iterator is reseated to the next line, and set
> it->start_of_box_run_p to true if that face is different and also has a
> box.

I don't think this is right.  At the very least, it will change a
long-standing behavior: we don't set the start_of_box_run flag just
because we found a different face.  Instead, the entire run of
characters whose face(s) have the :box attribute set will appear as a
single box, with a single start at the beginning and a single end at
the end of the run.

Drawing a beginning of a new box each time we find a new face is
certainly not what's expected: the most striking example of that is
the mode line: it could have different strings with different faces,
but we want only one start_of_box_run and only one end_of_box_run_p:
at the very left and the very right, respectively.

Even if you suggest to limit this to a new face that happens to begin
at the start of a new line, I still don't think this is TRT: why is a
new line special for this purpose?  If the newline before it had the
:box attribute set in its face, the character after the newline is
just the continuation of the same run of characters that started on
the previous line.  We shouldn't handle this specially when the :box
attribute crosses a newline.

So I see no bug in how the display code works in this case.

> I have this code mostly working now, but I don't understand why
> set_iterator_to_next doesn't seem to be updating it->face_id.

It does update it->face_id, just not where you expected it to.  See
below.

> AFAIU, set_iterator_to_next called with RESEAT set to true eventually
> calls reseat, which in turn calls handle_face_prop through handle_stop
> if the iterator was reseated past the stop position, which should be set
> to the beginning of the next line when the face property at that next
> line differs from the line whose face is being extended.

No, set_iterator_to_next called with RESEAT non-zero only calls reseat
if it skips some text, e.g., due to invisibility.  If it just moves
from a newline to the first character of the next line, it doesn't
call reseat, because there's no need to do so: reseat is for when we
need to move the iterator non-linearly, or jump over several character
positions.

> However, handle_face_prop is never called, so I have to call it manually
> like this:
> 
>         /* Consume the line end.  This skips over invisible lines.  */
>         set_iterator_to_next (it, true);
> 
> -->     handle_face_prop (it);

No, this is not needed.  handle_face_prop will be called once
display_line continues walking the buffer and processes the first
character of the new line.  At that time, it will notice that it
overstepped the stop position, and will call handle_stop, which will
call handle_face_prop, as expected.

You expected it to be called when processing the newline, but that is
too early.

But once again, I don't see anything that needs to be fixed in the
display engine.  Perhaps we need to rethink how the Lisp code sets up
the buttons as regards to the various faces or something.  IOW, let's
have a look at how the faces are set on the text in that case, and see
why we end up with two consecutive faces each one with a :box
attribute, because this has got to trigger this behavior, which you
find surprising and possibly incorrect.  The question is: can we
achieve whatever effect we wanted with the :box attributes without
losing the button appearance in the process by changing the Lisp code
there.