Re: Question about display engine

[Eli Zaretskii]

> Date: Sun, 8 Sep 2019 02:51:10 +0200
> From: Ergus <address@hidden>
> Cc: address@hidden, address@hidden
> 
> Please give a look to the attached patch and check if it is working fine
> for you. (I added a new branch "extend_face_id" in savannah too)
> 
> I tried to make it as optimized and less changes as possible from the
> beginning.
> 
> In general I added a parameter to handle_face_prop renamed to
> handle_face_prop_general (keeping the original name as a wrapper with
> the same signature.)
> 
> And the extra parameter is an enum lface_attribute_index. The parameter
> is passed from function to function until merge_named_face which checks:
> 
> (attr_filter == 0 || (!NILP (from[attr_filter])
>                    && !UNSPECIFIEDP (from[attr_filter])))
> 
> So if we pass zero as the parameter then the merge is unconditional;
> else the attribute needs to be non-nil and specified to merge.
> 
> I made it in this way because it is general enough and with low overhead
> in case we want to condition the merge for different conditions in
> the future.

I didn't yet have time to have a close look at the changes, but I can
already say that this handle_face_prop_general business I don't like.
Passing a pointer to a face ID, like this:

  +handle_face_prop_general (struct it *it, int *face_id_ptr,
  +                          enum lface_attribute_index attr_filter)

and then assigning to it via the pointer is gross.  Also, the
extension code doesn't need to return the HANDLED_NORMALLY value,
AFAIU.

Instead, it is much cleaner to have a new function with the guts of
handle_face_prop, which would _return_ a face ID.  Then
handle_face_prop would then plug this into it->face_id, and the
extension code will do what it needs.  Can you make this change,
please?

> Now the only annoying thing is that when extend is disabled for the
> region, the extra space after eol has the same face than the text before
> (which for me is fine) but in the terminal it is not added... so I
> should ask if you consider correct to add the space in terminal or
> remove the extra "colored" space in gui? I vote for the first... but you
> say.

Yes, the terminal should use the same face as GUI for the first blank
after end of line.

Thanks.