Package: emacs;
Reported by: Konrad Podczeck <konrad.podczeck <at> univie.ac.at>
Date: Fri, 17 Jul 2020 15:37:02 UTC
Severity: normal
View this message in rfc822 format
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: alan <at> idiocy.org, konrad.podczeck <at> univie.ac.at, 42406 <at> debbugs.gnu.org Subject: bug#42406: Mouse-wheel scrolling can be flickering 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;
}
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.