:extend t inheritance

:extend t inheritance

[Ingo Lohmar]

I am messing around with the face :extend attribute and I am wondering
about sth.  The elisp manual says

     The value should be ‘t’ to display the empty space between end of
     line and edge of the window using this face, or ‘nil’ to not use
     this face for the space between the end of the line and the edge of
     the window.  When Emacs merges several faces for displaying the
     empty space beyond end of line, only those faces with ‘:extend’
     non-‘nil’ will be merged.

I briefly read the original discussion of this feature, but did not find
a reason for this peculiar behavior when it comes to inheritance; if
there was a reason given, could someone point me to it?

Background: I manage all faces without themes on my own, and I rely
heavily on inheritance.  As it stands now, IIUC (and according to some
tests), I have to specify :extend t on ~30 faces (mostly diff, ediff,
smerge, magit-ediff and -blame etc) that all derive from 3 or 4 base
faces.  It seems to me that this renders the whole point of inheritance
moot.

My natural reaction would have been to specify :extend t on some base
faces, and for faces that inherit from them, explicitly say :extend nil
only for those faces for which I do not want that extension.  Isn't that
at least as flexible, and more in the spirit of inheriting attributes?

Re: :extend t inheritance

[Eli Zaretskii]

> From: Ingo Lohmar <ingo.lohmar@posteo.net>
> Date: Wed, 23 Oct 2019 22:34:05 +0200
> 
> I am messing around with the face :extend attribute and I am wondering
> about sth.  The elisp manual says
> 
>      The value should be ‘t’ to display the empty space between end of
>      line and edge of the window using this face, or ‘nil’ to not use
>      this face for the space between the end of the line and the edge of
>      the window.  When Emacs merges several faces for displaying the
>      empty space beyond end of line, only those faces with ‘:extend’
>      non-‘nil’ will be merged.
> 
> I briefly read the original discussion of this feature, but did not find
> a reason for this peculiar behavior when it comes to inheritance; if
> there was a reason given, could someone point me to it?

I don't think I understand what you are saying or asking.  The text
that you cite doesn't mention inheritance at all.  Could you clarify
the issue?

> Background: I manage all faces without themes on my own, and I rely
> heavily on inheritance.  As it stands now, IIUC (and according to some
> tests), I have to specify :extend t on ~30 faces (mostly diff, ediff,
> smerge, magit-ediff and -blame etc) that all derive from 3 or 4 base
> faces.  It seems to me that this renders the whole point of inheritance
> moot.
> 
> My natural reaction would have been to specify :extend t on some base
> faces, and for faces that inherit from them, explicitly say :extend nil
> only for those faces for which I do not want that extension.  Isn't that
> at least as flexible, and more in the spirit of inheriting attributes?

Are you saying that the :extend attribute isn't inherited via
:inherit?  Or are you saying something else?  In any case, could you
please show a recipe or an example of what you tried and how the
results didn't match your expectations?

Thanks.

Re: :extend t inheritance

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> Are you saying that the :extend attribute isn't inherited via
> :inherit?  Or are you saying something else?  In any case, could you
> please show a recipe or an example of what you tried and how the
> results didn't match your expectations?

From emacs -Q:

Add and remove a bunch of lines in a version-controlled file, and open a
*vc-diff* buffer with C-x v =.  Both diff-added and diff-removed faces
extend beyond EOL since by default their :extend attribute is set to t.

Then evaluate:

(set-face-extend 'diff-added 'unspecified)
(set-face-extend 'diff-removed 'unspecified)
(set-face-extend 'diff-changed t)

Intended effect: none, since diff-added and diff-removed inherit
diff-changed.

Actual effect: diff-added and diff-removed no longer extend beyond EOL.

Like Ingo, I'm not sure whether this (:extend not being inheritable) is
intentional or not; like Ingo, I would like to be able to set :extend t
on a few base faces.

Re: :extend t inheritance

[Ingo Lohmar]

On Thu, Oct 24 2019 16:46 (+0300), Eli Zaretskii wrote:

> I don't think I understand what you are saying or asking.  The text
> that you cite doesn't mention inheritance at all.  Could you clarify
> the issue?

Sorry, my mistake: I forgot a) that I had known about face merging
before, and b) the behavior described in the text seemed to fit what I
was seeing with inheritance of the :extend attribute.

On Thu, Oct 24 2019 17:38 (+0200), Kévin Le Gouguec wrote:
> From emacs -Q:
>
> Add and remove a bunch of lines in a version-controlled file, and open a
> *vc-diff* buffer with C-x v =.  Both diff-added and diff-removed faces
> extend beyond EOL since by default their :extend attribute is set to t.
>
> Then evaluate:
>
> (set-face-extend 'diff-added 'unspecified)
> (set-face-extend 'diff-removed 'unspecified)
> (set-face-extend 'diff-changed t)
>
> Intended effect: none, since diff-added and diff-removed inherit
> diff-changed.
>
> Actual effect: diff-added and diff-removed no longer extend beyond EOL.

Thanks a lot for this recipe!

Re: :extend t inheritance

[Ergus]

On Thu, Oct 24, 2019 at 04:46:08PM +0300, Eli Zaretskii wrote:
>> From: Ingo Lohmar <ingo.lohmar@posteo.net>
>> Date: Wed, 23 Oct 2019 22:34:05 +0200
>>
>> I am messing around with the face :extend attribute and I am wondering
>> about sth.  The elisp manual says
>>
>>      The value should be ???t??? to display the empty space between end of
>>      line and edge of the window using this face, or ???nil??? to not use
>>      this face for the space between the end of the line and the edge of
>>      the window.  When Emacs merges several faces for displaying the
>>      empty space beyond end of line, only those faces with ???:extend???
>>      non-???nil??? will be merged.
>>
>> I briefly read the original discussion of this feature, but did not find
>> a reason for this peculiar behavior when it comes to inheritance; if
>> there was a reason given, could someone point me to it?
>
>I don't think I understand what you are saying or asking.  The text
>that you cite doesn't mention inheritance at all.  Could you clarify
>the issue?
>
>> Background: I manage all faces without themes on my own, and I rely
>> heavily on inheritance.  As it stands now, IIUC (and according to some
>> tests), I have to specify :extend t on ~30 faces (mostly diff, ediff,
>> smerge, magit-ediff and -blame etc) that all derive from 3 or 4 base
>> faces.  It seems to me that this renders the whole point of inheritance
>> moot.
>>
>> My natural reaction would have been to specify :extend t on some base
>> faces, and for faces that inherit from them, explicitly say :extend nil
>> only for those faces for which I do not want that extension.  Isn't that
>> at least as flexible, and more in the spirit of inheriting attributes?
>
>Are you saying that the :extend attribute isn't inherited via
>:inherit?  Or are you saying something else?  In any case, could you
>please show a recipe or an example of what you tried and how the
>results didn't match your expectations?
>

Hi:

I just fount that it is not actually.

Lets say when calling merge_face_ref there is a detail missing for
the inherit case. I am looking into it.


>Thanks.
>

Re: :extend t inheritance

[Ergus]

Hi:

I just added a branch `fix/inherit_extend_face` that fixes the issue
with inheritance of the extend attribute with filter condition.

There are two commits because I also detected another issue in gui I
didn't see before because I don't use gui normally.

Please give it a look and if everything is fine I'll move it to master
ASAP.

On Thu, Oct 24, 2019 at 04:46:08PM +0300, Eli Zaretskii wrote:
>> From: Ingo Lohmar <ingo.lohmar@posteo.net>
>> Date: Wed, 23 Oct 2019 22:34:05 +0200
>>
>> I am messing around with the face :extend attribute and I am wondering
>> about sth.  The elisp manual says
>>
>>      The value should be ???t??? to display the empty space between end of
>>      line and edge of the window using this face, or ???nil??? to not use
>>      this face for the space between the end of the line and the edge of
>>      the window.  When Emacs merges several faces for displaying the
>>      empty space beyond end of line, only those faces with ???:extend???
>>      non-???nil??? will be merged.
>>
>> I briefly read the original discussion of this feature, but did not find
>> a reason for this peculiar behavior when it comes to inheritance; if
>> there was a reason given, could someone point me to it?
>
>I don't think I understand what you are saying or asking.  The text
>that you cite doesn't mention inheritance at all.  Could you clarify
>the issue?
>
>> Background: I manage all faces without themes on my own, and I rely
>> heavily on inheritance.  As it stands now, IIUC (and according to some
>> tests), I have to specify :extend t on ~30 faces (mostly diff, ediff,
>> smerge, magit-ediff and -blame etc) that all derive from 3 or 4 base
>> faces.  It seems to me that this renders the whole point of inheritance
>> moot.
>>
>> My natural reaction would have been to specify :extend t on some base
>> faces, and for faces that inherit from them, explicitly say :extend nil
>> only for those faces for which I do not want that extension.  Isn't that
>> at least as flexible, and more in the spirit of inheriting attributes?
>
>Are you saying that the :extend attribute isn't inherited via
>:inherit?  Or are you saying something else?  In any case, could you
>please show a recipe or an example of what you tried and how the
>results didn't match your expectations?
>
>Thanks.
>

Re: :extend t inheritance

[Eli Zaretskii]

> Date: Sat, 26 Oct 2019 03:49:13 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Ingo Lohmar <ingo.lohmar@posteo.net>, emacs-devel@gnu.org
> 
> I just added a branch `fix/inherit_extend_face` that fixes the issue
> with inheritance of the extend attribute with filter condition.

Thanks.

> There are two commits because I also detected another issue in gui I
> didn't see before because I don't use gui normally.
> 
> Please give it a look and if everything is fine I'll move it to master
> ASAP.

I took a look.  I don't understand some of the hunks, so please post
the rationale for the non-trivial changes there, otherwise it's hard
to review the changeset.

Re: :extend t inheritance

[Ergus]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

 Hi Eli: 

The change is actually simple but somehow unexpected for me (as I forgot the inheritance before). 

Before `merge_named_face` filtered the call to `merge_face_vectors` ifthe filter parameter (lets say :extend in our case) was `nil` or`undefined`. That was how the attr_filter worked. 

But in this case it is actually wrong when the face was inherited andthe attribute was `undefined` because maybe the `parent` face definethe filter parameter to `non-nil`. 

So now `merge_named_face` calls `merge_face_vectors` if the parameter isundefined too, and `merge_face_vectors` internally conditions if themerge is needed. The problem is that the merge with the parent faces wasalways made writing the TO vector (in place) as it was not conditional, but wedon't know in advance if the parent (or the grand-parents) define thefilter attribute to non-nil 

So what I did in the `undefined` case was to make a copy of the TOvector (tmp) and merge_ref there instead of for TO, after the merge Icheck if the attribute is set, and if so, then I copy it into TO andcontinue with the merge. 

The two copies and the extra call to merge_ref (in some cases) probably are not the mostefficient solution, but it is the only way I found to do it rightwithout repeating the merge (that must be more expensive) or modifyingthe TO vector. But the vector so far is not too big (only 20 elements upto now) so I thing it is good enough. There are some small optimizationsto avoid the extra call to merge_ref I set in order to optimize a bitwhen possible (you know my obsession about that); like return in advance when some parent sets the filter to nil andso on. 



   
 
 
 
-----Original Message-----
From: Eli Zaretskii <eliz@gnu.org>
To: Ergus <spacibba@aol.com>
Cc: emacs-devel <emacs-devel@gnu.org>; ingo.lohmar <ingo.lohmar@posteo.net>
Sent: Sat, Oct 26, 2019 9:29 am
Subject: Re: :extend t inheritance

> Date: Sat, 26 Oct 2019 03:49:13 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Ingo Lohmar <ingo.lohmar@posteo.net>, emacs-devel@gnu.org
> 
> I just added a branch `fix/inherit_extend_face` that fixes the issue
> with inheritance of the extend attribute with filter condition.

Thanks.

> There are two commits because I also detected another issue in gui I
> didn't see before because I don't use gui normally.
> 
> Please give it a look and if everything is fine I'll move it to master
> ASAP.

I took a look.  I don't understand some of the hunks, so please post
the rationale for the non-trivial changes there, otherwise it's hard
to review the changeset.


[-- Attachment #2: Type: text/html, Size: 4339 bytes --]

Re: :extend t inheritance

[Eli Zaretskii]

> Date: Sat, 26 Oct 2019 19:21:40 +0000 (UTC)
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org, ingo.lohmar@posteo.net
> 
> Before `merge_named_face` filtered the call to `merge_face_vectors` if the filter parameter (lets say :extend
> in our case) was `nil` or `undefined`. That was how the attr_filter worked. 
> 
> But in this case it is actually wrong when the face was inherited and the attribute was `undefined` because
> maybe the `parent` face define the filter parameter to `non-nil`. 
> 
> So now `merge_named_face` calls `merge_face_vectors` if the parameter is undefined too, and
> `merge_face_vectors` internally conditions if the merge is needed. The problem is that the merge with the
> parent faces was always made writing the TO vector (in place) as it was not conditional, but we don't know in
> advance if the parent (or the grand-parents) define the filter attribute to non-nil 
> 
> So what I did in the `undefined` case was to make a copy of the TO vector (tmp) and merge_ref there instead
> of for TO, after the merge I check if the attribute is set, and if so, then I copy it into TO and continue with the
> merge. 

I understood all that, but the changes include more than what you
described (and your original message even said that you fixed a couple
of other bugs as well).  Those other things is what I was asking
about.  Just do

  git diff ...origin/fix/inherit_extend_face

in the master branch, and you will see those other changes right away.
For example, I see 2 comments deleted, and something about
stretch_ascent(?).  What is this stuff, and what does it solve?

Thanks.

Re: :extend t inheritance

[Ergus]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

 Ohh right, that's the other commit. When fixing this issue I foundanother error I didn't see in tui.After EOL we need to fill with the merged filtered face. To do that weneed to add a stretch glyph; but we were just adding a normal glyph witha ' ' so the face was not really extended, just the background colorbecause the X display engine always do that. But, for example, underlinewas not extended.This commit fixes just that. stretch_ascent was moved out of theif(column_indicator) because the new stretch_glyphs needs it any way andthe two comments were 1) for old moved code not there anymore and 2) todescribe the situation of the glyphs color extension (mention inprevious paragraph) that we don't need to work around anymore becausethe glyph fills the entire screen now.. 
 
-----Original Message-----
From: Eli Zaretskii <eliz@gnu.org>
To: Ergus <spacibba@aol.com>
Cc: ingo.lohmar <ingo.lohmar@posteo.net>; emacs-devel <emacs-devel@gnu.org>
Sent: Sat, Oct 26, 2019 9:47 pm
Subject: Re: :extend t inheritance

> Date: Sat, 26 Oct 2019 19:21:40 +0000 (UTC)
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org, ingo.lohmar@posteo.net
> 
> Before `merge_named_face` filtered the call to `merge_face_vectors` if the filter parameter (lets say :extend
> in our case) was `nil` or `undefined`. That was how the attr_filter worked. 
> 
> But in this case it is actually wrong when the face was inherited and the attribute was `undefined` because
> maybe the `parent` face define the filter parameter to `non-nil`. 
> 
> So now `merge_named_face` calls `merge_face_vectors` if the parameter is undefined too, and
> `merge_face_vectors` internally conditions if the merge is needed. The problem is that the merge with the
> parent faces was always made writing the TO vector (in place) as it was not conditional, but we don't know in
> advance if the parent (or the grand-parents) define the filter attribute to non-nil 
> 
> So what I did in the `undefined` case was to make a copy of the TO vector (tmp) and merge_ref there instead
> of for TO, after the merge I check if the attribute is set, and if so, then I copy it into TO and continue with the
> merge. 

I understood all that, but the changes include more than what you
described (and your original message even said that you fixed a couple
of other bugs as well).  Those other things is what I was asking
about.  Just do

  git diff ...origin/fix/inherit_extend_face

in the master branch, and you will see those other changes right away.
For example, I see 2 comments deleted, and something about
stretch_ascent(?).  What is this stuff, and what does it solve?

Thanks.


[-- Attachment #2: Type: text/html, Size: 4015 bytes --]

Re: :extend t inheritance

[Eli Zaretskii]

> Date: Sat, 26 Oct 2019 23:13:40 +0000 (UTC)
> From: Ergus <spacibba@aol.com>
> Cc: ingo.lohmar@posteo.net, emacs-devel@gnu.org
> 
> Ohh right, that's the other commit. When fixing this issue I found another error I didn't see in tui. After EOL we
> need to fill with the merged filtered face. To do that we need to add a stretch glyph; but we were just adding a
> normal glyph with a ' ' so the face was not really extended, just the background color because the X display
> engine always do that. But, for example, underline was not extended. This commit fixes just that.
> stretch_ascent was moved out of the if(column_indicator) because the new stretch_glyphs needs it any way
> and the two comments were 1) for old moved code not there anymore and 2) to describe the situation of the
> glyphs color extension (mention in previous paragraph) that we don't need to work around anymore because
> the glyph fills the entire screen now.. 

I'm still a bit confused: was the problem you found happening in GUI
frames or in text-mode frames?  You say "tui", but then talk about
something X does...

Re: :extend t inheritance

[Ergus]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

In GUI. The issue was in GUI. What I said about tui was that I didn't observe the issue in tui. 

On October 27, 2019 6:51:07 AM GMT+01:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sat, 26 Oct 2019 23:13:40 +0000 (UTC)
>> From: Ergus <spacibba@aol.com>
>> Cc: ingo.lohmar@posteo.net, emacs-devel@gnu.org
>> 
>> Ohh right, that's the other commit. When fixing this issue I found
>another error I didn't see in tui. After EOL we
>> need to fill with the merged filtered face. To do that we need to add
>a stretch glyph; but we were just adding a
>> normal glyph with a ' ' so the face was not really extended, just the
>background color because the X display
>> engine always do that. But, for example, underline was not extended.
>This commit fixes just that.
>> stretch_ascent was moved out of the if(column_indicator) because the
>new stretch_glyphs needs it any way
>> and the two comments were 1) for old moved code not there anymore and
>2) to describe the situation of the
>> glyphs color extension (mention in previous paragraph) that we don't
>need to work around anymore because
>> the glyph fills the entire screen now.. 
>
>I'm still a bit confused: was the problem you found happening in GUI
>frames or in text-mode frames?  You say "tui", but then talk about
>something X does...

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 1764 bytes --]

Re: :extend t inheritance

[Eli Zaretskii]

> Date: Sun, 27 Oct 2019 12:01:04 +0100
> CC: ingo.lohmar@posteo.net,emacs-devel@gnu.org
> From: Ergus <spacibba@aol.com>
> 
> In GUI. The issue was in GUI. What I said about tui was that I didn't observe the issue in tui. 

OK, I think the code is fine and can be merged.

Thanks.

Re: :extend t inheritance

[Ergus]

Should I merge this into master finally?

On Sun, Oct 27, 2019 at 07:51:07AM +0200, Eli Zaretskii wrote:
>> Date: Sat, 26 Oct 2019 23:13:40 +0000 (UTC)
>> From: Ergus <spacibba@aol.com>
>> Cc: ingo.lohmar@posteo.net, emacs-devel@gnu.org
>>
>> Ohh right, that's the other commit. When fixing this issue I found another error I didn't see in tui. After EOL we
>> need to fill with the merged filtered face. To do that we need to add a stretch glyph; but we were just adding a
>> normal glyph with a ' ' so the face was not really extended, just the background color because the X display
>> engine always do that. But, for example, underline was not extended. This commit fixes just that.
>> stretch_ascent was moved out of the if(column_indicator) because the new stretch_glyphs needs it any way
>> and the two comments were 1) for old moved code not there anymore and 2) to describe the situation of the
>> glyphs color extension (mention in previous paragraph) that we don't need to work around anymore because
>> the glyph fills the entire screen now..
>
>I'm still a bit confused: was the problem you found happening in GUI
>frames or in text-mode frames?  You say "tui", but then talk about
>something X does...
>

Re: :extend t inheritance

[Eli Zaretskii]

> Date: Mon, 28 Oct 2019 22:24:59 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org, ingo.lohmar@posteo.net
> 
> Should I merge this into master finally?

It's up to you.  I haven't yet have time to review the changes after
you explained what they intend to do.

Re: :extend t inheritance

[Ingo Lohmar]

On Sat, Oct 26 2019 03:49 (+0200), Ergus wrote:

> Hi:
>
> I just added a branch `fix/inherit_extend_face` that fixes the issue
> with inheritance of the extend attribute with filter condition.
>
> There are two commits because I also detected another issue in gui I
> didn't see before because I don't use gui normally.
>
> Please give it a look and if everything is fine I'll move it to master
> ASAP.

Thanks, I just tried the branch and continued customizing my face
settings: No everything works as I would expect from inheritance.  I did
not see any adverse effects of the changes either, but I only used it
for about an hour.

Looking forward to the changes being merged!

BTW: there seem to be some whitespace-only hunks in the changes..

Re: :extend t inheritance

[Ergus]

On October 26, 2019 10:55:41 AM GMT+02:00, Ingo Lohmar <ingo.lohmar@posteo.net> wrote:
>On Sat, Oct 26 2019 03:49 (+0200), Ergus wrote:
>
>> Hi:
>>
>> I just added a branch `fix/inherit_extend_face` that fixes the issue
>> with inheritance of the extend attribute with filter condition.
>>
>> There are two commits because I also detected another issue in gui I
>> didn't see before because I don't use gui normally.
>>
>> Please give it a look and if everything is fine I'll move it to
>master
>> ASAP.
>
>Thanks, I just tried the branch and continued customizing my face
>settings: No everything works as I would expect from inheritance.  I
>did
>not see any adverse effects of the changes either, but I only used it
>for about an hour.
>
Could you be more specific please? What is.not working as you expect?

>Looking forward to the changes being merged!
>
>BTW: there seem to be some whitespace-only hunks in the changes..

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: :extend t inheritance

[Ingo Lohmar]

On Sat, Oct 26 2019 13:41 (+0200), Ergus wrote:

> On October 26, 2019 10:55:41 AM GMT+02:00, Ingo Lohmar <ingo.lohmar@posteo.net> wrote:
>>
>>Thanks, I just tried the branch and continued customizing my face
>>settings: No everything works as I would expect from inheritance.  I
>>did
>>not see any adverse effects of the changes either, but I only used it
>>for about an hour.
>>
> Could you be more specific please? What is.not working as you expect?

Sorry, just a typo that slipped through: "No" -> "Now".

To be clear --- everything works as expected!