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

[Daniel Colascione <dancol@dancol.org>]

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.