Re: Question about display engine

[Ergus]

On Sat, Sep 21, 2019 at 11:20:44AM +0300, Eli Zaretskii wrote:
> > 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.
Hi:

Thanks for the reply.

> 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?
That's what I do actually are you in the last changes? Probably I am the
one who is missing something otherwise.

> 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.

Yes, I change that already to return the id without assigning anything.

> . 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.

The initial face if has nothing to do with the last argument. It is
needed for an optimization at the end of the function. But yes, using it
internally is better.

> . 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.
This was for the idea to avoid recomputing it->extend_face_id in some
cases, and reuse the previous value when possible too. (that's why I
pass it as a parameter face_id actually.)

But I can change that if you think it is not needed.

> 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.
Thanks to you