Re: Question about display engine

[Ergus]

Hi Eli:

I know all that, I was just concerned about functionality and the
changes I made in the merge_ref function if they are conceptually right
or if they could produce some issues in some use cases that maybe I am
ignoring. (specially the method I use to filter.)

I implemented this in the face_at_buffer but in the same function there
is a face_at_string equivalent... is it possible in some conditions to
reach that part of the code at eol? if so I will need to modify that one
too...

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 will change that detail you mention and some others in the interface
later. But I need to be sure if the filter conditions already
implemented are enough. (With the first implementation I
learned that functional does not mean right.)


On Sun, Sep 08, 2019 at 08:51:52PM +0300, Eli Zaretskii wrote:
> > 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.