Re: Question about display engine

[Eli Zaretskii]

> Cc: address@hidden, address@hidden
> From: martin rudalics <address@hidden>
> Date: Wed, 14 Aug 2019 10:58:13 +0200
> 
>  >> Consider a user who sets the :extend attribute to non-nil for the
>  >> 'default' face and wants a 'link' background to not extend to the end
>  >> of line.  Such a user would want to set the :extend attribute of the
>  >> 'link' face to nil to get the desired effect.
>  >
>  > Yes, and my point is that in that case we can simply ignore the 'link'
>  > face when extending the face of the last character of a line.
> 
> How would the display engine then know that it can "simply ignore"
> the background of that face for the rest of the line?

By seeing that it has the :extend attribute set to nil.

> understanding face implementatin _without_ the text you wrote below
> would take a couple of days at least for a humble reader like me.

And I didn't even mention the complications related to bidi ;-)

>  > Neither :background nor any other face attribute is "found and
>  > applied" in the main iterator loop.  The iterator simply records in
>  > each glyph the face ID to be used for displaying that glyph.  That
>  > face ID is the value returned by the last call to
>  > face_at_buffer_position.
> 
> IIUC the latter will (unless the face was cached already) end up
> calling merge_face_ref and somewhere there I find the lines
> 
>             else if (EQ (keyword, QCbackground))
>               {
>                 if (STRINGP (value))
>                   to[LFACE_BACKGROUND_INDEX] = value;
>                 else
>                   err = true;
>               }
> 
> Hence IIUC in the
> 
>         while (CONSP (face_ref) && CONSP (XCDR (face_ref)))
> 
> loop we could manage three booleans background, extend and
> extend_value and instead of the above write
> 
>             else if (EQ (keyword, QCbackground))
>               {
>                 if (STRINGP (value))
>                   {
>                     to[LFACE_BACKGROUND_INDEX] = value;
> 
>                     if (extend)
>                       to[LFACE_EXTEND_BACKGROUND] = extend_value;
> 
>                     background = true;
>                   }
>                 else
>                   err = true;
>               }
>             else if (EQ (keyword, QCextend_background))
>               {
>                 extend = true;
>                 extend_value = !NILP (value);
> 
>                 if (background)
>                   to[LFACE_EXTEND_BACKGROUND] = extend_value;
>               }

This just copies the :extend flag to the merged face.  It sill copies
the :background attribute itself, and so the merged face will have a
non-default background color.

>  > So you propose to have an "extend" bit for every face attribute?
>  > There are 18 of them.  (Maybe some of the attributes won't need that
>  > bit, but quite a few will.)
> 
> I'd propose to add these lazily if there is any need.  AFAIAC
> :background is the only attribute where an extend bit sounds useful.

IMO, it makes very little sense to allow this only for the background
color.  For starters, please keep in mind that this whole discussion
started because we were considering to make TTY frames behave like GUI
frames wrt face extension, and TTY frames currently extend also the
other attributes, notably the :underline.  Some people also complain
about :underline being ugly on GUI frames when it crosses the newline
and continues on the next line.

I think we need to support this for :background, :underline,
:overline, :strike-through, :box, and :stipple.

> The extend_background bit would be set in merge_face_ref as sketched
> above (because this is the one that can read the :background and
> :extend attributes of any face) and examined in
> extend_face_to_end_of_line wherever face->background is involved
> (because this is the one that knows that we want to display a
> newline).

What do you want extend_face_to_end_of_line to do with the
extend_background bit?  extend_face_to_end_of_line doesn't apply the
background, it just produces glyphs, and has only face IDs to work
with.  So if the face ID of the last character specifies a face with a
background color, and the extend_background bit says not to extend the
background color, how would extend_face_to_end_of_line "remove" the
background color attribute from the face for which it only has the ID?

The only way to do that is to make a new face, by merging all the
attributes except the background color, and then use that new face's
ID for producing glyphs that extend the face.  This is why I said
earlier that I assumed we make the extension decisions at face merge
time, and will have 2 different realized faces, not one.  But you
disagreed with that conclusion:

>  > What I wrote above assumed that
>  > we make these decisions at face merge time, and will actually have 2
>  > different realized faces;
> 
> We should have only one realized face.

AFAIU, that's impossible, not on the level on which
extend_face_to_end_of_line (or any code in xdisp.c, really) works.  It
cannot "apply a face sans some attributes", there are no such
capabilities on this level.