Subject: bug#42406: Mouse-wheel scrolling can be flickering

From: Stefan Monnier <[email-masked]>
Date: Fri, 18 Dec 2020 11:22:40 -0500

>> > That probably just means abbrev-mode should be added to the list at
>> > the end of frame.el.  Or maybe that we need some new mechanism to
>> > trigger update of the lighter on the mode line when a mode is turned
>> > on or off.
>> Don't know about "new" but the old mechanism is that the standard
>> minor-mode code ends up calling `force-mode-line-update` (this now
>> mostly comes from `define-minor-mode`, but in the past it was present in
>> most "manual" definitions as well).
> IMO, that shouldn't be needed.

But it is, currently.

> My point is that we are dealing with a bunch of global and local flags
> with overlapping functionality about which we have no clear idea what
> each one is doing.  Removing a flag will not solve that basic problem,
> because I don't doubt that some of these flags is needed in some,
> perhaps subtle and rare, situation.  We won't find flags that could be
> removed without unwanted consequences.

IME a good to way to find out what a chunk of code is for is to remove
it and see what happens.  It's not the only way and not always the best
way either, of course.

> My point is that the documentation says REDISPLAY_SOME causes only the
> mode line(s) to be updated, but the idea that changes which affect the
> mode line could be handled by using REDISPLAY_SOME is incorrect
> because it assumes the windows above and below will never be affected
> by such changes.  So the idea itself is flawed, albeit in very rare
> and somewhat unusual use cases.

I can see how a mode-line update can reduce the height of the
mode-line and this require to display "more" of the contents of the
attached window.  But:
- I don't know how/where this is currently handled in the redisplay
  (I do have some ideas of how it *could* be handled, OTOH ;-)
- I don't see how this can also require display updates in the window
  that's below.
- more importantly, I don't see how this relates to REDISPLAY_SOME: it
  seems to be an issue of "update mode lines may require updates in
  window contents" and that issue is linked to the division between
  `update_mode_lines` and `windows_or_buffers_changed` but that is
  orthogonal to the REDISPLAY_SOME one.

> I hoped that this will lead to the conclusion that the current
> partition of possible use cases and its translation into specific sets
> of values of the flags and variables we have is at least inaccurate
> and incomplete, if not worse.  With the implied realization that we
> need to rethink this and then reimplement it.

In case you're curious, here's my idea of how I think the above problem
could be attacked.

    bool redisplay_bits_set;

    fset_redisplay ()
    { ...; redisplay_bits_set = true; }
    [...]

    redisplay ()
    {
       if (redisplay_bits_set)
         redisplay_internal ();
       if (redisplay_bits_set)
         /* Redisplay itself caused further changes.  So try again.
            This second redisplay could potentially cause yet more changes,
            but it *really* should not.
            If it does, then tough luck: we won't take the risk of
            inf-looping for it so the result will only be seen at the
            next redisplay.  */
         redisplay_internal ();
    }

>> >   consider_all_windows_p = (update_mode_lines
>> > 			    || windows_or_buffers_changed);
>> >   [...]
>> >   if (consider_all_windows_p)
>> >     {
>> >       FOR_EACH_FRAME (tail, frame)
>> > 	XFRAME (frame)->updated_p = false;
>> >
>> >       propagate_buffer_redisplay ();
>> >
>> >       FOR_EACH_FRAME (tail, frame)
>> > 	{
>> >    [...]
>> >
>> > If the redisplay flag is all we need, how come we must also set
>> > update_mode_lines or windows_or_buffers_changed to get Emacs to
>> > consider anything beyond the selected window?
>> The `redisplay` bits were designed to reduce the set of windows that we
>> consider at each redisplay.
> Then why do we need the consider_all_windows_p condition, which is
> based on 2 other variables?

That's because I kept the special case where redisplay only considers
the selected window as explained earlier.

> It should have been enough to simply go over all the redisplay flags
> of all the frames and windows and buffers, and see if any of them are
> set for any window other than the selected window of the selected
> frame.  Right?

Yes.  I explained yesterday why I didn't do that back then:

    Similarly, I kept the special case where we only consider the selected
    window.  We could get rid of it and only rely on the `redisplay` bits
    instead, but it could make things marginally slower in some cases, and
    it would have a required more work to try and better understand what
    that "selected window only" code path does to make sure I wasn't
    introducing any regression.

But if someone wants to go ahead and do that, I'd welcome it.

>> > Why does it have to be so complicated to say "this frame needs to have
>> > all of its windows reconsidered for redisplay"?
>> Is it?  AFAIK `fset_redisplay (f)` is all it takes, which doesn't seem
>> particularly complex (and neither does its code).
> So you are saying that wset_update_mode_line should only set the
> frame's redisplay flag?  If so, why didn't your patch to do just that
> work?

AFAIK that is what my patch does.

> And that's because the flags we use, however numerous, are too blunt
> for selectively specifying which parts to redisplay.  Which AFAIU is
> the crux of this bug report.

Since the performance is good enough in the single-frame case (even
though it does perform a lot of unnecessary work), I think the
`redisplay` bits are sufficient for the needs of this bug-report (since
they are perfectly sufficient to reduce the many-frames case down to the
single-frame case).

>> >> > I'm not against experimenting with replacing 42 by 32 or by
>> >> > REDISPLAY_SOME etc., but I don't think we should install anything
>> >> > along these lines, except if we need to fix a clear bug (i.e. a
>> >> > redisplay glitch), which this one isn't.
>> >> I don't know what you mean by "along these lines".
>> > "Along these lines" means playing more games with "special" values of
>> > update_mode_lines and windows_or_buffers_changed.
>> I don't know what you mean by "special values".
>> And I'm not playing any games here.
> This is not useful: you are responding to the specific words I used
> instead of responding to what I meant (which I think is fairly
> obvious).

I'm afraid it's not obvious enough for me.  I'm not playing with words,
I was only quoting the specific words which I think are the source of my
lack of understanding of what you meant.

>> The meaning of those vars is as follows:
>>
>> - update_mode_lines == 0 means that none of the mode lines (and
>>   relatives) needs to be updated.
>> - update_mode_lines > 2 means that all the mode lines in all windows
>>   need to be updated.
>> - update_mode_lines == 2 means that all the mode lines need to be
>>   updated in the set designated by the `redisplay` bits (where the
>>   `redisplay` on a frame means that all of its windows are also part opf
>>   the set, and where the `redisplay` bit of a buffer means that all the
>>   windows that display this buffer are also part of the set).
>>
>> - windows_or_buffers_changed == 0 means that only the selected window's
>>   content may need to be updated.
>
> Yes, I know.  The comments you provided tell this much.  The problem
> is, the reality is subtly and annoyingly different, and that is not
> good for maintainability.

When it's different, please consider it as a bug rather than as "the doc
doesn't match reality".  For that reason I believe the patch below is
*right*.

Maybe it'll introduce regressions, in which case it should indicate that
we have a bug elsewhere, and maybe it won't improve the original problem
(e.g. because 42 is only one of the causes why the redisplay has to
reconsider all the windows/frames), but it is a step in the right direction.

To be clear: I have no intention to push this to `emacs-27`, but
I can't see any good reason not to push it to master (after fixing its
FIXME, obviously).

>> - update_mode_lines > 2 means that the contents in all windows
>>   may need to be updated.
>> - update_mode_lines == 2 means that the contents in all windows in the
>>   set designated by the `redisplay` bits may need to be updated.
>
> Copy/paste? did you mean windows_or_buffers_changed?

Indeed, sorry.

        Stefan

diff --git a/src/window.c b/src/window.c
index bcc989b5a7..9b88c18142 100644
--- a/src/window.c
+++ b/src/window.c
@@ -224,7 +224,13 @@ wset_update_mode_line (struct window *w)
   Lisp_Object fselected_window = XFRAME (WINDOW_FRAME (w))->selected_window;
 
   if (WINDOWP (fselected_window) && XWINDOW (fselected_window) == w)
-    update_mode_lines = 42;
+    {
+      /* FIXME: This should be in xdisp.c, next to fset_redisplay
+         and friends!  */
+      if (!update_mode_lines)
+        update_mode_lines = 2;    /* FIXME: REDISPLAY_SOME  */
+      XFRAME (WINDOW_FRAME (w))->redisplay = true;
+    }
   else
     w->update_mode_line = true;
 }