Re: Question about display engine

[Ergus <spacibba@aol.com>]

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 <spacibba@aol.com>
>> Cc: rudalics@gmx.at, emacs-devel@gnu.org
>>
>> 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.