Emacs 27 face :extend attribute not working with pulse.el?

Emacs 27 face :extend attribute not working with pulse.el?

[Adam Porter]

Hi,

I'm still using Emacs 26.3 as my main instance, but while testing
Andrea's native-comp branch, I've also been testing Emacs 28.0.50 which,
of course, includes the changes from Emacs 27.0.90.

A while back I read on this list about the new face :extend attribute,
how it required some changes in packages like Magit, etc.  It sounded
like the issues had been resolved, that setting the :extend attribute
would restore previous behavior, etc.

However, in my Scrollkeeper package
(https://github.com/alphapapa/scrollkeeper.el), it does not seem to be
working.  One of the configurable guideline methods in the package uses
pulse-momentary-highlight-oneline to highlight a line in the buffer when
one of the scrolling commands is called (typically by setting a
background color attribute).  In Emacs 26.3, it works properly: the
entire visual line, up to the width of the window, is highlighted.  But
in Emacs 28.0.50, even when the scrollkeeper-guideline-highlight face
has the :extend attribute set to t, the highlighting does not extend to
the width of the window.

I think this issue may be in pulse.el, because evaluating this code has
the same problem:

  (pulse-momentary-highlight-one-line (point))

I hope this can be fixed before Emacs 27 is released.  Or, if I'm
missing a workaround or change that I'm supposed to apply in my code,
please let me know.

Should I file a bug report about this, or is discussion here sufficient?

Thanks,
Adam

Re: Emacs 27 face :extend attribute not working with pulse.el?

[Adam Porter]

Adam Porter <adam@alphapapa.net> writes:

> Hi,
>
> I'm still using Emacs 26.3 as my main instance, but while testing
> Andrea's native-comp branch, I've also been testing Emacs 28.0.50 which,
> of course, includes the changes from Emacs 27.0.90.
>
> A while back I read on this list about the new face :extend attribute,
> how it required some changes in packages like Magit, etc.  It sounded
> like the issues had been resolved, that setting the :extend attribute
> would restore previous behavior, etc.
>
> However, in my Scrollkeeper package
> (https://github.com/alphapapa/scrollkeeper.el), it does not seem to be
> working.  One of the configurable guideline methods in the package uses
> pulse-momentary-highlight-oneline to highlight a line in the buffer when
> one of the scrolling commands is called (typically by setting a
> background color attribute).  In Emacs 26.3, it works properly: the
> entire visual line, up to the width of the window, is highlighted.  But
> in Emacs 28.0.50, even when the scrollkeeper-guideline-highlight face
> has the :extend attribute set to t, the highlighting does not extend to
> the width of the window.
>
> I think this issue may be in pulse.el, because evaluating this code has
> the same problem:
>
>   (pulse-momentary-highlight-one-line (point))

After further investigation, I've found that setting ":extend t" on the
pulse-highlight-face seems to fix the problem and restore the behavior
seen in Emacs 26.3.

This seems like an obvious, easy fix to restore the previous behavior,
but I don't know if it's the "correct" one, because it appears to have
the side effect of forcing extension even when
pulse-momentary-highlight-one-line is called with a face argument which
does not have ":extend t" set.

Re: Emacs 27 face :extend attribute not working with pulse.el?

[Eli Zaretskii]

> From: Adam Porter <adam@alphapapa.net>
> Date: Thu, 09 Apr 2020 08:18:59 -0500
> 
> > I think this issue may be in pulse.el, because evaluating this code has
> > the same problem:
> >
> >   (pulse-momentary-highlight-one-line (point))
> 
> After further investigation, I've found that setting ":extend t" on the
> pulse-highlight-face seems to fix the problem and restore the behavior
> seen in Emacs 26.3.

Yes, that is the right solution, I think.  But the default should stay
with the :extend attribute nil, IMO.

> This seems like an obvious, easy fix to restore the previous behavior,
> but I don't know if it's the "correct" one, because it appears to have
> the side effect of forcing extension even when
> pulse-momentary-highlight-one-line is called with a face argument which
> does not have ":extend t" set.

Does the patch below produce good results?

diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
index 16243e1..8649254 100644
--- a/lisp/cedet/pulse.el
+++ b/lisp/cedet/pulse.el
@@ -161,6 +161,9 @@ pulse-reset-face
 			   (face-background face nil t)
 			 (face-background 'pulse-highlight-start-face)
 			 ))
+  (and face
+       (set-face-extend 'pulse-highlight-face
+                        (face-extend-p face nil t)))
   (put 'pulse-highlight-face :startface (or face
 					    'pulse-highlight-start-face))
   (put 'pulse-highlight-face :iteration 0))

P.S. And *please* in the future report bugs with report-emacs-bug.  It
is tedious to have to mention the URL of emacs-devel discussion in the
log message, instead of just mentioning the bug number.

Re: Emacs 27 face :extend attribute not working with pulse.el?

[Adam Porter]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Adam Porter <adam@alphapapa.net>
>> Date: Thu, 09 Apr 2020 08:18:59 -0500
>> 
>> > I think this issue may be in pulse.el, because evaluating this code has
>> > the same problem:
>> >
>> >   (pulse-momentary-highlight-one-line (point))
>> 
>> After further investigation, I've found that setting ":extend t" on the
>> pulse-highlight-face seems to fix the problem and restore the behavior
>> seen in Emacs 26.3.
>
> Yes, that is the right solution, I think.  But the default should stay
> with the :extend attribute nil, IMO.
>
>> This seems like an obvious, easy fix to restore the previous behavior,
>> but I don't know if it's the "correct" one, because it appears to have
>> the side effect of forcing extension even when
>> pulse-momentary-highlight-one-line is called with a face argument which
>> does not have ":extend t" set.
>
> Does the patch below produce good results?
>
> diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
> index 16243e1..8649254 100644
> --- a/lisp/cedet/pulse.el
> +++ b/lisp/cedet/pulse.el
> @@ -161,6 +161,9 @@ pulse-reset-face
>  			   (face-background face nil t)
>  			 (face-background 'pulse-highlight-start-face)
>  			 ))
> +  (and face
> +       (set-face-extend 'pulse-highlight-face
> +                        (face-extend-p face nil t)))
>    (put 'pulse-highlight-face :startface (or face
>  					    'pulse-highlight-start-face))
>    (put 'pulse-highlight-face :iteration 0))

Yes, it seems to behave as expected.  Thanks.

> P.S. And *please* in the future report bugs with report-emacs-bug.  It
> is tedious to have to mention the URL of emacs-devel discussion in the
> log message, instead of just mentioning the bug number.

Understood, thanks.

Re: Emacs 27 face :extend attribute not working with pulse.el?

[Eli Zaretskii]

> From: Adam Porter <adam@alphapapa.net>
> Date: Thu, 09 Apr 2020 09:46:40 -0500
> 
> >> This seems like an obvious, easy fix to restore the previous behavior,
> >> but I don't know if it's the "correct" one, because it appears to have
> >> the side effect of forcing extension even when
> >> pulse-momentary-highlight-one-line is called with a face argument which
> >> does not have ":extend t" set.
> >
> > Does the patch below produce good results?
> >
> > diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
> > index 16243e1..8649254 100644
> > --- a/lisp/cedet/pulse.el
> > +++ b/lisp/cedet/pulse.el
> > @@ -161,6 +161,9 @@ pulse-reset-face
> >  			   (face-background face nil t)
> >  			 (face-background 'pulse-highlight-start-face)
> >  			 ))
> > +  (and face
> > +       (set-face-extend 'pulse-highlight-face
> > +                        (face-extend-p face nil t)))
> >    (put 'pulse-highlight-face :startface (or face
> >  					    'pulse-highlight-start-face))
> >    (put 'pulse-highlight-face :iteration 0))
> 
> Yes, it seems to behave as expected.  Thanks.

Thanks, installed on the emacs-27 branch.