bug#75291: Redisplay not updating fringe when face filter changes

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Sometimes we don't redraw when its effective face changes indirectly due
to a face filter condition evaluating differently.

An explicit redraw-display works around the problem, but it shouldn't be
necessary and is an efficiency hit.

See the code below:

;; -*- lexical-binding: t -*-

;; emacs -Q

(setf my-window (selected-window))

(dolist (face '(default fringe))
  (face-remap-add-relative
   face `(:filtered (:window foo t) (:background "green"))))

(set-window-parameter my-window 'foo t)

;; During this sit-for, background and fringe should be green
(sit-for 1)
(set-window-parameter my-window 'foo nil)

;; Enable to work around bug
(when nil
  (redraw-display))

;; During this sit-for, background and fringe should be their original
;; color (probably white), but only the background changes.
;; Without redraw-display, the fringe remains green.
(sit-for 1)

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Date: Thu, 02 Jan 2025 12:30:16 -0500
> 
> Sometimes we don't redraw when its effective face changes indirectly due
> to a face filter condition evaluating differently.
> 
> An explicit redraw-display works around the problem, but it shouldn't be
> necessary and is an efficiency hit.
> 
> See the code below:
> 
> ;; -*- lexical-binding: t -*-
> 
> ;; emacs -Q
> 
> (setf my-window (selected-window))
> 
> (dolist (face '(default fringe))
>   (face-remap-add-relative
>    face `(:filtered (:window foo t) (:background "green"))))
> 
> (set-window-parameter my-window 'foo t)
> 
> ;; During this sit-for, background and fringe should be green
> (sit-for 1)
> (set-window-parameter my-window 'foo nil)
> 
> ;; Enable to work around bug
> (when nil
>   (redraw-display))
> 
> ;; During this sit-for, background and fringe should be their original
> ;; color (probably white), but only the background changes.
> ;; Without redraw-display, the fringe remains green.
> (sit-for 1)

I think this is bug#74876 again.  I tried to explain there why the way
the fringe drawing is implemented doesn't work well with
face-remapping (or with changes in the fringe face in general).

Maybe I'm missing something, but supporting what this and that bug
want would need a rewrite of update_window_fringes (and possibly also
the way we record fringes' attributes in the glyph row).

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Thu, 02 Jan 2025 12:30:16 -0500
>> 
>> Sometimes we don't redraw when its effective face changes indirectly due
>> to a face filter condition evaluating differently.
>> 
>> An explicit redraw-display works around the problem, but it shouldn't be
>> necessary and is an efficiency hit.
>> 
>> See the code below:
>> 
>> ;; -*- lexical-binding: t -*-
>> 
>> ;; emacs -Q
>> 
>> (setf my-window (selected-window))
>> 
>> (dolist (face '(default fringe))
>>   (face-remap-add-relative
>>    face `(:filtered (:window foo t) (:background "green"))))
>> 
>> (set-window-parameter my-window 'foo t)
>> 
>> ;; During this sit-for, background and fringe should be green
>> (sit-for 1)
>> (set-window-parameter my-window 'foo nil)
>> 
>> ;; Enable to work around bug
>> (when nil
>>   (redraw-display))
>> 
>> ;; During this sit-for, background and fringe should be their original
>> ;; color (probably white), but only the background changes.
>> ;; Without redraw-display, the fringe remains green.
>> (sit-for 1)
>
> I think this is bug#74876 again.  I tried to explain there why the way
> the fringe drawing is implemented doesn't work well with
> face-remapping (or with changes in the fringe face in general).
>
> Maybe I'm missing something, but supporting what this and that bug
> want would need a rewrite of update_window_fringes (and possibly also
> the way we record fringes' attributes in the glyph row).

That's amazing. Seven years ago, I implemented
https://github.com/dcolascione/emacs-window-highlight/blob/master/window-highlight.el.
I worked around the bug we're discussing using redraw-display. I see
that Michal (++) implemented the same feature independently and reported
the same bug just a few weeks before I got around to reporting the same
bug from seven years ago!  Pure serendipity.

(BTW: my code uses pre-redisplay-function, not a command hook. I've
tried it both ways and found the redisplay hook to be more reliable.)

Anyway, given that the feature has been implemented twice now, maybe we
can find some way to make it work efficiently?  I haven't looked into
how exactly we do fringe invalidation, but isn't there a way we can
limit the blast radius of the redraw by providing a lisp-level function
to invalidate extra GUI parts of specific windows rather than forcing a
redraw-frame?  It's not clear to me why we couldn't skip redrawing every
row's content but redraw every row's fringe anyway.

Ideally, a change to a face in which the change couldn't possibly affect
layout (e.g. changing a background color) would be pretty efficient from
a redisplay POV, since we wouldn't have to even try to reflow any text.

bug#75291: Redisplay not updating fringe when face filter changes

[Michal Nazarewicz]

> Eli Zaretskii <eliz@gnu.org> writes:
>> I think this is bug#74876 again.  I tried to explain there why the way
>> the fringe drawing is implemented doesn't work well with
>> face-remapping (or with changes in the fringe face in general).
>>
>> Maybe I'm missing something, but supporting what this and that bug
>> want would need a rewrite of update_window_fringes (and possibly also
>> the way we record fringes' attributes in the glyph row).

On Thu, Jan 02 2025, Daniel Colascione wrote:
> (BTW: my code uses pre-redisplay-function, not a command hook. I've
> tried it both ways and found the redisplay hook to be more reliable.)

FYI, the actual auto-dim-other-buffers code doesn’t use
pre-command-hook.  It uses hooks for window, buffer or focus changes.

> Anyway, given that the feature has been implemented twice now, maybe we
> can find some way to make it work efficiently?

Since I managed to work-around the issue, I haven’t looked closely at
the low-level details of the redrawing code.  From afar it certainly
looks like it should be easy to force redrawing of fringes only, but
I also trust Eli’s opinion on complexity of this task more than my
impressions.

While I’d like to have a function indicating that particular window’s
fringes need to be updated (maybe something like ‘(force-window-update
window t)’, I won’t have time to work on this any time soon.

> Ideally, a change to a face in which the change couldn't possibly affect
> layout (e.g. changing a background color) would be pretty efficient from
> a redisplay POV, since we wouldn't have to even try to reflow any
> text.

I don’t think it’s that simple.  Even if all you’re changing is colour,
you still need to redraw the text.  At best, without anti-aliasing there
may be some way to optimise it but I doubt complicating the code to
support that would be worth it.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

merge 75291 74876
thanks

> From: Daniel Colascione <dancol@dancol.org>
> Cc: 75291@debbugs.gnu.org, Michal Nazarewicz <mina86@mina86.com>
> Date: Thu, 02 Jan 2025 13:36:34 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think this is bug#74876 again.  I tried to explain there why the way
> > the fringe drawing is implemented doesn't work well with
> > face-remapping (or with changes in the fringe face in general).
> >
> > Maybe I'm missing something, but supporting what this and that bug
> > want would need a rewrite of update_window_fringes (and possibly also
> > the way we record fringes' attributes in the glyph row).
> 
> That's amazing. Seven years ago, I implemented
> https://github.com/dcolascione/emacs-window-highlight/blob/master/window-highlight.el.
> I worked around the bug we're discussing using redraw-display. I see
> that Michal (++) implemented the same feature independently and reported
> the same bug just a few weeks before I got around to reporting the same
> bug from seven years ago!  Pure serendipity.

Yes, redrawing everything will work, but will also cause flicker, and
is generally expensive, thus undesirable.

> Anyway, given that the feature has been implemented twice now, maybe we
> can find some way to make it work efficiently?  I haven't looked into
> how exactly we do fringe invalidation, but isn't there a way we can
> limit the blast radius of the redraw by providing a lisp-level function
> to invalidate extra GUI parts of specific windows rather than forcing a
> redraw-frame?  It's not clear to me why we couldn't skip redrawing every
> row's content but redraw every row's fringe anyway.
> 
> Ideally, a change to a face in which the change couldn't possibly affect
> layout (e.g. changing a background color) would be pretty efficient from
> a redisplay POV, since we wouldn't have to even try to reflow any text.

AFAIR, the current implementation is the other way around: when some
glyph row changes, we consider redrawing its fringes.  E.g.,
draw_window_fringes only considers glyph rows that are enabled in the
glyph matrix, which means redisplay found that glyph row has changed.
Clearly, someone didn't think the fringe face will change during a
session!

Regarding your idea about Lisp function that would invalidate GUI
parts: it is not very easy, since a Lisp program cannot easily know
where on the screen a given region of buffer positions will be.
There is posn-at-point, of course, but (a) it is quite expensive, and
(b) when Lisp runs, display could be outdated, so what posn-at-point
returns could be inaccurate.

In any case, I don't think this will be needed in the case in point,
because when the fringe face changes, we'd want to redraw the fringes
of the entire window, right?  So redisplay_window would need to notice
the change in the face, and invoke update_window_fringes in a special
way, such that update_window_fringes marks fringes to be redrawn not
only when the glyph row is enabled.  Maybe that would work.

The way to "notice the change in the face" is not a simple problem to
solve, btw.  We currently don't know which faces change when some face
is modified.  So we have a frame-global flag that is set when any face
changes its attributes.  If we use that flag for detecting potential
changes in the fringe face, we'd start redrawing fringes
unnecessarily.  We could add a special flag for the fringe face, but
then we'd need to add a special mechanism in xfaces.c which would
analyze each change in some face's attribute and determine whether it
could possibly affect the fringe face.  Or maybe I'm missing some more
elegant/easier solution.

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Eli Zaretskii <eliz@gnu.org> writes:

> merge 75291 74876
> thanks
>
>> From: Daniel Colascione <dancol@dancol.org>
>> Cc: 75291@debbugs.gnu.org, Michal Nazarewicz <mina86@mina86.com>
>> Date: Thu, 02 Jan 2025 13:36:34 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I think this is bug#74876 again.  I tried to explain there why the way
>> > the fringe drawing is implemented doesn't work well with
>> > face-remapping (or with changes in the fringe face in general).
>> >
>> > Maybe I'm missing something, but supporting what this and that bug
>> > want would need a rewrite of update_window_fringes (and possibly also
>> > the way we record fringes' attributes in the glyph row).
>> 
>> That's amazing. Seven years ago, I implemented
>> https://github.com/dcolascione/emacs-window-highlight/blob/master/window-highlight.el.
>> I worked around the bug we're discussing using redraw-display. I see
>> that Michal (++) implemented the same feature independently and reported
>> the same bug just a few weeks before I got around to reporting the same
>> bug from seven years ago!  Pure serendipity.
>
> Yes, redrawing everything will work, but will also cause flicker, and
> is generally expensive, thus undesirable.

FWIW, it doesn't seem to cause flicker in practice.  I see flicker only
when walking through messages in mu4e --- we do redisplay and draw only
the background, and I haven't figured out why yet.  But in general, on a
modern window system, turning a given redisplay into a full redisplay
shouldn't cause flicker, even if it's inefficient.

>> Anyway, given that the feature has been implemented twice now, maybe we
>> can find some way to make it work efficiently?  I haven't looked into
>> how exactly we do fringe invalidation, but isn't there a way we can
>> limit the blast radius of the redraw by providing a lisp-level function
>> to invalidate extra GUI parts of specific windows rather than forcing a
>> redraw-frame?  It's not clear to me why we couldn't skip redrawing every
>> row's content but redraw every row's fringe anyway.
>> 
>> Ideally, a change to a face in which the change couldn't possibly affect
>> layout (e.g. changing a background color) would be pretty efficient from
>> a redisplay POV, since we wouldn't have to even try to reflow any text.
>
> AFAIR, the current implementation is the other way around: when some
> glyph row changes, we consider redrawing its fringes.  E.g.,
> draw_window_fringes only considers glyph rows that are enabled in the
> glyph matrix, which means redisplay found that glyph row has changed.
> Clearly, someone didn't think the fringe face will change during a
> session!

I came across overlay_arrows_changed_p --- isn't this function trying to
deal with exactly the case of something in the fringe changing outside
the changed text region?

> Regarding your idea about Lisp function that would invalidate GUI
> parts: it is not very easy, since a Lisp program cannot easily know
> where on the screen a given region of buffer positions will be.
> There is posn-at-point, of course, but (a) it is quite expensive, and
> (b) when Lisp runs, display could be outdated, so what posn-at-point
> returns could be inaccurate.

I was imagining a lisp function that would make the next redisplay of a
window do what you suggest in the next paragraph.

I'm not sure we'd even need an explicit Lisp function though.
Face filters with :window are defined to compare window parameter values
with eq, so couldn't we keep track of all the :filtered face
specifications we encounter during face resolution and have
set-window-parameter check whether the parameter it's setting is on the
list of possible face filters and, if so, force next redisplay to
evaluate faces? set-window-parameter wouldn't even have to do a deep
comparison, because it's just eq.

> In any case, I don't think this will be needed in the case in point,
> because when the fringe face changes, we'd want to redraw the fringes
> of the entire window, right?  So redisplay_window would need to notice
> the change in the face, and invoke update_window_fringes in a special
> way, such that update_window_fringes marks fringes to be redrawn not
> only when the glyph row is enabled.  Maybe that would work.

> The way to "notice the change in the face" is not a simple problem to
> solve, btw.  We currently don't know which faces change when some face
> is modified.  So we have a frame-global flag that is set when any face
> changes its attributes.  If we use that flag for detecting potential
> changes in the fringe face, we'd start redrawing fringes
> unnecessarily.

How many unnecessary face recalculations would we do? ISTM we could make
the invalidation pretty precise as long as we're just looking at
window parameters.

> We could add a special flag for the fringe face, but
> then we'd need to add a special mechanism in xfaces.c which would
> analyze each change in some face's attribute and determine whether it
> could possibly affect the fringe face.  Or maybe I'm missing some more
> elegant/easier solution.

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> Date: Thu, 02 Jan 2025 14:50:54 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, redrawing everything will work, but will also cause flicker, and
> > is generally expensive, thus undesirable.
> 
> FWIW, it doesn't seem to cause flicker in practice.  I see flicker only
> when walking through messages in mu4e --- we do redisplay and draw only
> the background, and I haven't figured out why yet.  But in general, on a
> modern window system, turning a given redisplay into a full redisplay
> shouldn't cause flicker, even if it's inefficient.

I think it depends on whether you use double-buffering (some people
don't or cannot) and whether you have the mouse pointer over an Emacs
frame.  Also, depending on the GUI toolkit, the decorations might
flicker.

> I came across overlay_arrows_changed_p --- isn't this function trying to
> deal with exactly the case of something in the fringe changing outside
> the changed text region?

So you want to add to display_line code that sets each glyph_row's
redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
probably work, provided that we disable redisplay optimizations which
might avoid calling display_line (you will see that we already disable
such optimizations when overlay_arrows_changed_p returns non-zero).
We might actually need to disable more of the optimizations, because
the overlay-arrow thing doesn't contradict the optimizations that
scroll the pixels, something that reaction to changes in the fringe
face cannot tolerate.

> > Regarding your idea about Lisp function that would invalidate GUI
> > parts: it is not very easy, since a Lisp program cannot easily know
> > where on the screen a given region of buffer positions will be.
> > There is posn-at-point, of course, but (a) it is quite expensive, and
> > (b) when Lisp runs, display could be outdated, so what posn-at-point
> > returns could be inaccurate.
> 
> I was imagining a lisp function that would make the next redisplay of a
> window do what you suggest in the next paragraph.

What does Lisp know about the fringe face that the display engine
doesn't?

> I'm not sure we'd even need an explicit Lisp function though.
> Face filters with :window are defined to compare window parameter values
> with eq, so couldn't we keep track of all the :filtered face
> specifications we encounter during face resolution and have
> set-window-parameter check whether the parameter it's setting is on the
> list of possible face filters and, if so, force next redisplay to
> evaluate faces? set-window-parameter wouldn't even have to do a deep
> comparison, because it's just eq.

First, the fringe face might not be window-specific (by default, it
isn't).  So I'm not sure how a window parameter will help.
face-remapping-alist is per-buffer, not per-window.

Next, the main problem with faces is face inheritance and face merging
(the latter is not relevant to fringe, I think).  Given that some
attribute of some face changes, how do you know whether such a change
causes the fringe face to change?  We'd probably need to realize it
anew, and then compare to the cached one?  And we'd need to do that
for each window, because of face-remapping-alist?

> > The way to "notice the change in the face" is not a simple problem to
> > solve, btw.  We currently don't know which faces change when some face
> > is modified.  So we have a frame-global flag that is set when any face
> > changes its attributes.  If we use that flag for detecting potential
> > changes in the fringe face, we'd start redrawing fringes
> > unnecessarily.
> 
> How many unnecessary face recalculations would we do? ISTM we could make
> the invalidation pretty precise as long as we're just looking at
> window parameters.

See above.

I think the proof of the proverbial pudding is in eating: maybe doing
the above will produce reasonable performance.

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Colascione <dancol@dancol.org>
>> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
>> Date: Thu, 02 Jan 2025 14:50:54 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Yes, redrawing everything will work, but will also cause flicker, and
>> > is generally expensive, thus undesirable.
>> 
>> FWIW, it doesn't seem to cause flicker in practice.  I see flicker only
>> when walking through messages in mu4e --- we do redisplay and draw only
>> the background, and I haven't figured out why yet.  But in general, on a
>> modern window system, turning a given redisplay into a full redisplay
>> shouldn't cause flicker, even if it's inefficient.
>
> I think it depends on whether you use double-buffering (some people
> don't or cannot) and whether you have the mouse pointer over an Emacs
> frame.  Also, depending on the GUI toolkit, the decorations might
> flicker.

TTY windows don't have fringes, and the most commonly-used window
systems all do atomic updates nowadays.

>> I came across overlay_arrows_changed_p --- isn't this function trying to
>> deal with exactly the case of something in the fringe changing outside
>> the changed text region?
>
> So you want to add to display_line code that sets each glyph_row's
> redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
> probably work, provided that we disable redisplay optimizations which
> might avoid calling display_line (you will see that we already disable
> such optimizations when overlay_arrows_changed_p returns non-zero).
> We might actually need to disable more of the optimizations, because
> the overlay-arrow thing doesn't contradict the optimizations that
> scroll the pixels, something that reaction to changes in the fringe
> face cannot tolerate.

That might work, but I don't think we even need anything that
complicated or low-level.  Not many are using :filtered now, and those
that do big redraws anyway.  How about this simpler code that gets us
correctness, albeit more conservatively?

diff --git a/src/window.c b/src/window.c
index 5a10c381eaf..6d135a67a66 100644
--- a/src/window.c
+++ b/src/window.c
@@ -2396,11 +2396,29 @@ DEFUN ("set-window-parameter", Fset_window_parameter,
   Lisp_Object old_alist_elt;
 
   old_alist_elt = Fassq (parameter, w->window_parameters);
+
+  /* If this window parameter has been used in a face remapping filter
+     expression and we changed its value, force a from-scratch redisplay
+     to make sure that everything that depends on the window parameter
+     value is up-to-date.  FIXME: instead of taking a sledgehammer to
+     redisplay, we could be more precise in tracking what display bits
+     depend on which remapped faces.  Skip redrawing TTY frames: they
+     don't have fringes or other graphical bits we might have to redraw
+     here.  */
+  if (SYMBOLP (parameter) &&
+      WINDOW_LIVE_P (window) &&
+      FRAME_WINDOW_P (WINDOW_XFRAME (w)) &&
+      !NILP (Fget (parameter, Qface_filter)) &&
+      !EQ (CDR_SAFE (old_alist_elt), value))
+    redraw_frame (WINDOW_XFRAME (w));
+
   if (NILP (old_alist_elt))
     wset_window_parameters
       (w, Fcons (Fcons (parameter, value), w->window_parameters));
   else
     Fsetcdr (old_alist_elt, value);
+
+
   return value;
 }
 
diff --git a/src/xfaces.c b/src/xfaces.c
index d1ca2e0d5d4..1f58bdbf6ae 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2512,6 +2512,9 @@ evaluate_face_filter (Lisp_Object filter, struct window *w,
     if (!NILP (filter))
       goto err;
 
+    if (NILP (Fget (parameter, Qface_filter)))
+      Fput (parameter, Qface_filter, Qt);
+
     bool match = false;
     if (w)
       {
@@ -7623,6 +7626,8 @@ syms_of_xfaces (void)
   Vface_remapping_alist = Qnil;
   DEFSYM (Qface_remapping_alist,"face-remapping-alist");
 
+  DEFSYM (Qface_filter, "face-filter");
+
   DEFVAR_LISP ("face-font-rescale-alist", Vface_font_rescale_alist,
 	       doc: /* Alist of fonts vs the rescaling factors.
 Each element is a cons (FONT-PATTERN . RESCALE-RATIO), where

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> Date: Fri, 03 Jan 2025 12:25:05 -0500
> 
> > I think it depends on whether you use double-buffering (some people
> > don't or cannot) and whether you have the mouse pointer over an Emacs
> > frame.  Also, depending on the GUI toolkit, the decorations might
> > flicker.
> 
> TTY windows don't have fringes, and the most commonly-used window
> systems all do atomic updates nowadays.

People still report flickering from time to time, so I don't think
this never happens.

> > So you want to add to display_line code that sets each glyph_row's
> > redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
> > probably work, provided that we disable redisplay optimizations which
> > might avoid calling display_line (you will see that we already disable
> > such optimizations when overlay_arrows_changed_p returns non-zero).
> > We might actually need to disable more of the optimizations, because
> > the overlay-arrow thing doesn't contradict the optimizations that
> > scroll the pixels, something that reaction to changes in the fringe
> > face cannot tolerate.
> 
> That might work, but I don't think we even need anything that
> complicated or low-level.  Not many are using :filtered now, and those
> that do big redraws anyway.  How about this simpler code that gets us
> correctness, albeit more conservatively?

Doesn't that only support face remapping with :filtered attribute?
What about the more general case where the fringe face is remapped in
a way that's independent of the windows?

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Colascione <dancol@dancol.org>
>> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
>> Date: Fri, 03 Jan 2025 12:25:05 -0500
>> 
>> > I think it depends on whether you use double-buffering (some people
>> > don't or cannot) and whether you have the mouse pointer over an Emacs
>> > frame.  Also, depending on the GUI toolkit, the decorations might
>> > flicker.
>> 
>> TTY windows don't have fringes, and the most commonly-used window
>> systems all do atomic updates nowadays.
>
> People still report flickering from time to time, so I don't think
> this never happens.
>
>> > So you want to add to display_line code that sets each glyph_row's
>> > redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
>> > probably work, provided that we disable redisplay optimizations which
>> > might avoid calling display_line (you will see that we already disable
>> > such optimizations when overlay_arrows_changed_p returns non-zero).
>> > We might actually need to disable more of the optimizations, because
>> > the overlay-arrow thing doesn't contradict the optimizations that
>> > scroll the pixels, something that reaction to changes in the fringe
>> > face cannot tolerate.
>> 
>> That might work, but I don't think we even need anything that
>> complicated or low-level.  Not many are using :filtered now, and those
>> that do big redraws anyway.  How about this simpler code that gets us
>> correctness, albeit more conservatively?
>
> Doesn't that only support face remapping with :filtered attribute?
> What about the more general case where the fringe face is remapped in
> a way that's independent of the windows?

That seems to work already. It's only in the fringe that I see problems
--- it just doesn't seem worth it to limit the redraw to the fringe.

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> Date: Fri, 03 Jan 2025 14:46:20 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Daniel Colascione <dancol@dancol.org>
> >> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> >> Date: Fri, 03 Jan 2025 12:25:05 -0500
> >> 
> >> > I think it depends on whether you use double-buffering (some people
> >> > don't or cannot) and whether you have the mouse pointer over an Emacs
> >> > frame.  Also, depending on the GUI toolkit, the decorations might
> >> > flicker.
> >> 
> >> TTY windows don't have fringes, and the most commonly-used window
> >> systems all do atomic updates nowadays.
> >
> > People still report flickering from time to time, so I don't think
> > this never happens.
> >
> >> > So you want to add to display_line code that sets each glyph_row's
> >> > redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
> >> > probably work, provided that we disable redisplay optimizations which
> >> > might avoid calling display_line (you will see that we already disable
> >> > such optimizations when overlay_arrows_changed_p returns non-zero).
> >> > We might actually need to disable more of the optimizations, because
> >> > the overlay-arrow thing doesn't contradict the optimizations that
> >> > scroll the pixels, something that reaction to changes in the fringe
> >> > face cannot tolerate.
> >> 
> >> That might work, but I don't think we even need anything that
> >> complicated or low-level.  Not many are using :filtered now, and those
> >> that do big redraws anyway.  How about this simpler code that gets us
> >> correctness, albeit more conservatively?
> >
> > Doesn't that only support face remapping with :filtered attribute?
> > What about the more general case where the fringe face is remapped in
> > a way that's independent of the windows?
> 
> That seems to work already. It's only in the fringe that I see problems
> --- it just doesn't seem worth it to limit the redraw to the fringe.

Sorry, I don't understand.  I _was_ talking about the fringe face.

But if redraw_frame solves the issue, and doesn't cause unpleasant or
expensive redraws, feel free to install on the master branch.  But
please change this:

+  if (SYMBOLP (parameter) &&
+      WINDOW_LIVE_P (window) &&
+      FRAME_WINDOW_P (WINDOW_XFRAME (w)) &&
+      !NILP (Fget (parameter, Qface_filter)) && <<<<<<<<<<<<<<<<<
+      !EQ (CDR_SAFE (old_alist_elt), value))
+    redraw_frame (WINDOW_XFRAME (w));

to say this instead:

+  if (SYMBOLP (parameter) &&
+      WINDOW_LIVE_P (window) &&
+      FRAME_WINDOW_P (WINDOW_XFRAME (w)) &&
+      EQ (Fget (parameter, Qface_filter), Qt) && <<<<<<<<<<<<<<<<<
+      !EQ (CDR_SAFE (old_alist_elt), value))
+    redraw_frame (WINDOW_XFRAME (w));

(A stylistic comment: our conventions is to put the && operator at the
beginning of a line, not at the end of a line.)

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> Cc: 75291@debbugs.gnu.org, mina86@mina86.com
> Date: Fri, 03 Jan 2025 22:10:45 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Daniel Colascione <dancol@dancol.org>
> > Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> > Date: Fri, 03 Jan 2025 14:46:20 -0500
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> From: Daniel Colascione <dancol@dancol.org>
> > >> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> > >> Date: Fri, 03 Jan 2025 12:25:05 -0500
> > >> 
> > >> > I think it depends on whether you use double-buffering (some people
> > >> > don't or cannot) and whether you have the mouse pointer over an Emacs
> > >> > frame.  Also, depending on the GUI toolkit, the decorations might
> > >> > flicker.
> > >> 
> > >> TTY windows don't have fringes, and the most commonly-used window
> > >> systems all do atomic updates nowadays.
> > >
> > > People still report flickering from time to time, so I don't think
> > > this never happens.
> > >
> > >> > So you want to add to display_line code that sets each glyph_row's
> > >> > redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
> > >> > probably work, provided that we disable redisplay optimizations which
> > >> > might avoid calling display_line (you will see that we already disable
> > >> > such optimizations when overlay_arrows_changed_p returns non-zero).
> > >> > We might actually need to disable more of the optimizations, because
> > >> > the overlay-arrow thing doesn't contradict the optimizations that
> > >> > scroll the pixels, something that reaction to changes in the fringe
> > >> > face cannot tolerate.
> > >> 
> > >> That might work, but I don't think we even need anything that
> > >> complicated or low-level.  Not many are using :filtered now, and those
> > >> that do big redraws anyway.  How about this simpler code that gets us
> > >> correctness, albeit more conservatively?
> > >
> > > Doesn't that only support face remapping with :filtered attribute?
> > > What about the more general case where the fringe face is remapped in
> > > a way that's independent of the windows?
> > 
> > That seems to work already. It's only in the fringe that I see problems
> > --- it just doesn't seem worth it to limit the redraw to the fringe.
> 
> Sorry, I don't understand.  I _was_ talking about the fringe face.
> 
> But if redraw_frame solves the issue, and doesn't cause unpleasant or
> expensive redraws, feel free to install on the master branch.  But
> please change this:

Btw: this is _really_ a sledgehammer solution, and it will affect
similar changes in any face, not just the fringe face.  I wonder how
long will it take for complaints to come in.

How about leaving behind a variable exposed to Lisp that could be used
to disable this sledgehammer?  Then we at least could tell people who
don't want this how to disable it.

bug#75291: Redisplay not updating fringe when face filter changes

[Daniel Colascione]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Colascione <dancol@dancol.org>
>> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
>> Date: Fri, 03 Jan 2025 14:46:20 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Daniel Colascione <dancol@dancol.org>
>> >> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
>> >> Date: Fri, 03 Jan 2025 12:25:05 -0500
>> >> 
>> >> > I think it depends on whether you use double-buffering (some people
>> >> > don't or cannot) and whether you have the mouse pointer over an Emacs
>> >> > frame.  Also, depending on the GUI toolkit, the decorations might
>> >> > flicker.
>> >> 
>> >> TTY windows don't have fringes, and the most commonly-used window
>> >> systems all do atomic updates nowadays.
>> >
>> > People still report flickering from time to time, so I don't think
>> > this never happens.
>> >
>> >> > So you want to add to display_line code that sets each glyph_row's
>> >> > redraw_fringe_bitmaps_p flag when the fringe face changes?  That could
>> >> > probably work, provided that we disable redisplay optimizations which
>> >> > might avoid calling display_line (you will see that we already disable
>> >> > such optimizations when overlay_arrows_changed_p returns non-zero).
>> >> > We might actually need to disable more of the optimizations, because
>> >> > the overlay-arrow thing doesn't contradict the optimizations that
>> >> > scroll the pixels, something that reaction to changes in the fringe
>> >> > face cannot tolerate.
>> >> 
>> >> That might work, but I don't think we even need anything that
>> >> complicated or low-level.  Not many are using :filtered now, and those
>> >> that do big redraws anyway.  How about this simpler code that gets us
>> >> correctness, albeit more conservatively?
>> >
>> > Doesn't that only support face remapping with :filtered attribute?
>> > What about the more general case where the fringe face is remapped in
>> > a way that's independent of the windows?
>> 
>> That seems to work already. It's only in the fringe that I see problems
>> --- it just doesn't seem worth it to limit the redraw to the fringe.
>
> Sorry, I don't understand.  I _was_ talking about the fringe face.

I misread your question. To answer it: what are the circumstances in
which the effective fringe face can change behind our backs?  Any change
to a face attribute proper will cause a redraw anyway.
The face-remap.el functions call force-mode-line-update to make sure
face remapping lists take effect, so we should have non-window-specific
remapping changes covered.  What's left is the window parameters.
Any other cases?

> But if redraw_frame solves the issue, and doesn't cause unpleasant or
> expensive redraws, feel free to install on the master branch.  But
> please change this:
>
> +  if (SYMBOLP (parameter) &&
> +      WINDOW_LIVE_P (window) &&
> +      FRAME_WINDOW_P (WINDOW_XFRAME (w)) &&
> +      !NILP (Fget (parameter, Qface_filter)) && <<<<<<<<<<<<<<<<<
> +      !EQ (CDR_SAFE (old_alist_elt), value))
> +    redraw_frame (WINDOW_XFRAME (w));
>
> to say this instead:
>
> +  if (SYMBOLP (parameter) &&
> +      WINDOW_LIVE_P (window) &&
> +      FRAME_WINDOW_P (WINDOW_XFRAME (w)) &&
> +      EQ (Fget (parameter, Qface_filter), Qt) && <<<<<<<<<<<<<<<<<
> +      !EQ (CDR_SAFE (old_alist_elt), value))
> +    redraw_frame (WINDOW_XFRAME (w));
>
> (A stylistic comment: our conventions is to put the && operator at the
> beginning of a line, not at the end of a line.)

Thanks for the reminder!

bug#75291: Redisplay not updating fringe when face filter changes

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Cc: 75291@debbugs.gnu.org,  mina86@mina86.com
> Date: Fri, 03 Jan 2025 15:57:37 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Doesn't that only support face remapping with :filtered attribute?
> >> > What about the more general case where the fringe face is remapped in
> >> > a way that's independent of the windows?
> >> 
> >> That seems to work already. It's only in the fringe that I see problems
> >> --- it just doesn't seem worth it to limit the redraw to the fringe.
> >
> > Sorry, I don't understand.  I _was_ talking about the fringe face.
> 
> I misread your question. To answer it: what are the circumstances in
> which the effective fringe face can change behind our backs?

For example, face remapping via face-remapping-alist.

> Any change to a face attribute proper will cause a redraw anyway.

Changes in face attributes set the frame's redisplay flag, but that
just tells the display engine to disable some radical optimizations,
like considering only the selected window and only the line where the
cursor is.  AFAIU, this will not necessarily cause all the fringes of
all the frame's windows to be redrawn, because we only redraw the
fringes of the glyph rows that we consider changed.

> The face-remap.el functions call force-mode-line-update to make sure
> face remapping lists take effect

force-mode-line-update sets a buffer-specific flag that disables some
redisplay optimizations, but I don't think that guarantees redrawing
of all the fringes of all the windows which show that buffer.

So I'm not sure we have that covered.  But if you have tested that and
this actually work, fine; I don't pretend to know for sure this won't
work.