Re: Question about display engine

[Eli Zaretskii]

> Date: Tue, 17 Sep 2019 12:48:02 +0300
> From: Eli Zaretskii <address@hidden>
> Cc: address@hidden, address@hidden
> 
> > Date: Tue, 17 Sep 2019 04:17:26 +0200
> > From: Ergus <address@hidden>
> > Cc: address@hidden, address@hidden
> > 
> > >>>>The filter now in merge_ref only works when !CONSP(ref_name). As it only
> > >>>>bypass the extra parameter to merge_named... it this right in the
> > >>>>general case?
> > >>
> > >>I think we should support all the cases, otherwise the feature will
> > >>behave inconsistently, and we will get bug reports.
> > >>
> > >About this; the problem is that in the general case I'm not sure what's
> > >the "right" behavior for the other cases. When !CONSP(ref_name) it means
> > >that the parameter is a face_name; but in the other cases it means that
> > >we are explicitly specifying the attributes as a cons list or as
> > >:atribute value pairs... What's expected to happen in those cases??
> > >
> > Hi:
> > Any Idea about this? Could you suggest what to do when CONSP(ref_name)
> > is true?
> 
> I will reply to this soon, too swamped now.

Sorry for the delay.

Having looked at the code, I'm not sure I understand the problem.  Why
not simply pass the attr_filter down to the respective merge_face_ref
calls, where you currently force zero instead?  Am I missing
something?

Some other comments about your code:

 . Please rename handle_face_prop_general into something like
   face_at_pos, and make it just return the face ID, without assigning
   any field in 'struct it'.  handle_face_prop will then call
   face_at_pos and assign the face ID as needed.
 . handle_face_prop_general is supposed to be called just once with
   the last argument non-zero, so I see no reason why it should be
   also passed the initial_face_id argument.  It looks wrong to call
   that function with it->extend_face_id as the 2nd argument, and have
   it compute it->extend_face_id, because the value you pass as an
   argument is undefined: it hasn't been computed yet.  I think the
   function should use it->face_id internally instead of that
   argument.
 . I don't understand why you need new members of 'struct it', like
   extend_face_id, saved_extend_face_id, etc.
   extend_face_to_end_of_line correctly assigns the value of extend
   face ID to it->face_id, after saving it->face_id in a local
   variable, so I see no need for it->extend_face_id, certainly not
   for it->saved_extend_face_id.  You also have extend_face_id in
   other related structures, where it is never used.

Regarding documentation: if you have difficulties with the Texinfo
markup, you could write plain text, and someone else could then add
markup.  Adding markup is a mostly mechanical procedure, unlike coming
up with a useful text.

Thanks.