bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Hello,

`force-mode-line-update' has the following FIXME:

  if (!NILP (all))
    {
      update_mode_lines = 10;
      /* FIXME: This can't be right.  */
      current_buffer->prevent_redisplay_optimizations_p = true;
    }
  else if (buffer_window_count (current_buffer))
    {
      bset_update_mode_line (current_buffer);
      current_buffer->prevent_redisplay_optimizations_p = true;
    }

This FIXME has been introduced in 655ab9a3800, shortly after
ecda65d4f7e, which moved this code from `set-buffer-modified-p'.

AFAIU, the purpose of disabling redisplay optimizations is avoiding the
situation when the modification flag is unset in the buffer, but the
buffer was actually modified, and has to be redrawn.

If my understanding is correct,
current_buffer->prevent_redisplay_optimizations_p = true does not belong
to `force-mode-line-update', but rather to `restore-buffer-modified-p'.

I also grepped through src/display.c looking at the usage of
update_mode_lines, and there seems to be no obvious situation where
update_mode_lines being non-nil is ignored.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-07-06 built on localhost
Repository revision: d97b77e6c66db46b198c696f83458aa141794727
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Gentoo Linux


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Thu, 13 Jul 2023 13:00:26 +0000
> 
> `force-mode-line-update' has the following FIXME:
> 
>   if (!NILP (all))
>     {
>       update_mode_lines = 10;
>       /* FIXME: This can't be right.  */
>       current_buffer->prevent_redisplay_optimizations_p = true;
>     }
>   else if (buffer_window_count (current_buffer))
>     {
>       bset_update_mode_line (current_buffer);
>       current_buffer->prevent_redisplay_optimizations_p = true;
>     }
> 
> This FIXME has been introduced in 655ab9a3800, shortly after
> ecda65d4f7e, which moved this code from `set-buffer-modified-p'.

Yes, it was part of Stefan's effort to make redisplay less expensive
by avoiding too thorough redisplay in as many use cases as possible.

> AFAIU, the purpose of disabling redisplay optimizations is avoiding the
> situation when the modification flag is unset in the buffer, but the
> buffer was actually modified, and has to be redrawn.

No, it's more subtle.  In a nutshell, it is meant to prevent redisplay
from applying certain optimizations (search xdisp.c for the uses of
this flag).  These optimizations are based on various assumptions,
such as that the current glyph matrix of the window is up-to-date.
Setting the prevent_redisplay_optimizations_p flag tells the display
engine that those assumptions are (or might be) false.

Updating the mode line usually requires more thorough redisplay,
especially when the only reason for that is that some Lisp called
force-mode-line-update -- without setting that flag, the display
engine could easily decide that the window doesn't need to be redrawn.

> If my understanding is correct,
> current_buffer->prevent_redisplay_optimizations_p = true does not belong
> to `force-mode-line-update', but rather to `restore-buffer-modified-p'.

The purpose of force-mode-line-update is to do what its name says,
regardless of whether the buffer was modified or not, and how it was
modified.  The idea is that Lisp programs which change something that
they know must affect the mode line call this function to make sure
the mode line is redrawn with up-to-date information.  By contrast,
buffer modifications could be such that don't affect redisplay at all,
or allow redisplay to use some shortcuts and redraw only a very small
portion of the window.

> I also grepped through src/display.c looking at the usage of
> update_mode_lines, and there seems to be no obvious situation where
> update_mode_lines being non-nil is ignored.

Stefan will tell, but I'm quite sure he wrote that FIXME because
removing that line caused regression in some situation.  I see that
this flag is tested without also testing update_mode_lines in
window-lines-pixel-dimensions, window-line-height, window-end,
redisplay_window, try_window_id, and display_line.  So I don't think I
agree with your conclusion.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> If my understanding is correct,
>> current_buffer->prevent_redisplay_optimizations_p = true does not belong
>> to `force-mode-line-update', but rather to `restore-buffer-modified-p'.
>
> The purpose of force-mode-line-update is to do what its name says,
> regardless of whether the buffer was modified or not, and how it was
> modified.  The idea is that Lisp programs which change something that
> they know must affect the mode line call this function to make sure
> the mode line is redrawn with up-to-date information.

I do not claim that I fully understand, but what is confusing is that a
number of other places in code simply use bset_update_mode_line without
disabling optimizations. In particular:

1. kill-all-local-variables
2. rename-buffer

Also, `force-window-update' disable optimizations for a given window,
but not when all windows should be updated - in contrast with the code
in OP.

So, `force-window-update' and `force-mode-line-update' are at least
inconsistent.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 64596@debbugs.gnu.org
> Date: Thu, 13 Jul 2023 17:19:29 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The purpose of force-mode-line-update is to do what its name says,
> > regardless of whether the buffer was modified or not, and how it was
> > modified.  The idea is that Lisp programs which change something that
> > they know must affect the mode line call this function to make sure
> > the mode line is redrawn with up-to-date information.
> 
> I do not claim that I fully understand

Who does, when it comes to the display code?

> but what is confusing is that a number of other places in code
> simply use bset_update_mode_line without disabling optimizations. In
> particular:
> 
> 1. kill-all-local-variables
> 2. rename-buffer

bset_update_mode_line is for a single buffer, whereas the FIXME is for
the case where force-mode-line-update is called with a non-nil ALL
argument.

> Also, `force-window-update' disable optimizations for a given window,
> but not when all windows should be updated - in contrast with the code
> in OP.

When all the windows need to be updated, we force that via
windows_or_buffers_changed, which is a somewhat stronger flag, as far
as preventing optimizations goes.

> So, `force-window-update' and `force-mode-line-update' are at least
> inconsistent.

Why should they be entirely consistent?  They do different jobs.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> AFAIU, the purpose of disabling redisplay optimizations is avoiding the
>> situation when the modification flag is unset in the buffer, but the
>> buffer was actually modified, and has to be redrawn.
>
> No, it's more subtle.  In a nutshell, it is meant to prevent redisplay
> from applying certain optimizations (search xdisp.c for the uses of
> this flag).  These optimizations are based on various assumptions,
> such as that the current glyph matrix of the window is up-to-date.
> Setting the prevent_redisplay_optimizations_p flag tells the display
> engine that those assumptions are (or might be) false.

I'm afraid that the very existence of prevent_redisplay_optimizations_p
flag is a mistake hiding bugs with redisplay optimizations logic.

Currently, redisplay_internal has a number of conditions used to
determine if one or another optimization is safe to use + assertion that
!prevent_redisplay_optimizations_p.

When some code outside xdisp.c sets this flag, it is nothing but a
statement: "I was lazy enough to update xdisp.c properly, so I just
force bypassing optimizations".

> Updating the mode line usually requires more thorough redisplay,
> especially when the only reason for that is that some Lisp called
> force-mode-line-update -- without setting that flag, the display
> engine could easily decide that the window doesn't need to be redrawn.

I have explored xdisp a bit and AFAIU, once we are in redisplay_window,
mode line will be updated as long as update_mode_line is set (at least
to UPDATE_SOME), except when current window is MINI_WINDOW_P (this code
branch is not prevented by prevent_redisplay_optimizations_p though) or
when we give up redisplaying.

Calling redisplay_window may be prevented via needs_no_redisplay, unless
we have buffer->text->redisplay, which is set in bset_update_mode_line.
[ setting buffer->text->redisplay is rather awkward way to single mode
line update though ]

redisplay_window may also be postponed for frames that are not visible
or that are obscured, but it appears to be by design and not influenced
by prevent_redisplay_optimizations_p anyway 😕

So, I see not why calling bset_update_mode_line is not sufficient to
force mode line update in all windows associated with a single buffer.

>> If my understanding is correct,
>> current_buffer->prevent_redisplay_optimizations_p = true does not belong
>> to `force-mode-line-update', but rather to `restore-buffer-modified-p'.
>
> The purpose of force-mode-line-update is to do what its name says,
> regardless of whether the buffer was modified or not, and how it was
> modified.  The idea is that Lisp programs which change something that
> they know must affect the mode line call this function to make sure
> the mode line is redrawn with up-to-date information.  By contrast,
> buffer modifications could be such that don't affect redisplay at all,
> or allow redisplay to use some shortcuts and redraw only a very small
> portion of the window.

Then, why do we need to call Fforce_mode_line_update in
set-buffer-modified-p? Wouldn't calling bset_redisplay be better?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 11:53:02 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > No, it's more subtle.  In a nutshell, it is meant to prevent redisplay
> > from applying certain optimizations (search xdisp.c for the uses of
> > this flag).  These optimizations are based on various assumptions,
> > such as that the current glyph matrix of the window is up-to-date.
> > Setting the prevent_redisplay_optimizations_p flag tells the display
> > engine that those assumptions are (or might be) false.
> 
> I'm afraid that the very existence of prevent_redisplay_optimizations_p
> flag is a mistake hiding bugs with redisplay optimizations logic.

I think you should avoid making such comments until you have a better
understanding of the redisplay logic, or else I will stop taking your
posts in this matter seriously.

> Currently, redisplay_internal has a number of conditions used to
> determine if one or another optimization is safe to use + assertion that
> !prevent_redisplay_optimizations_p.
> 
> When some code outside xdisp.c sets this flag, it is nothing but a
> statement: "I was lazy enough to update xdisp.c properly, so I just
> force bypassing optimizations".

No.  The optimization in question, which is disabled when
prevent_redisplay_optimizations_p flag is set for the current buffer,
is described in the comment before the condition:

  /* Optimize the case that only the line containing the cursor in the
     selected window has changed.

The prevent_redisplay_optimizations_p flag says, among other things,
that we are not in that case, and it is tested early enough in order
to prevent us from examining additional conditions which are just
waste of CPU cycles (and some of them are relatively expensive).

> > Updating the mode line usually requires more thorough redisplay,
> > especially when the only reason for that is that some Lisp called
> > force-mode-line-update -- without setting that flag, the display
> > engine could easily decide that the window doesn't need to be redrawn.
> 
> I have explored xdisp a bit and AFAIU, once we are in redisplay_window,
> mode line will be updated as long as update_mode_line is set (at least
> to UPDATE_SOME), except when current window is MINI_WINDOW_P (this code
> branch is not prevented by prevent_redisplay_optimizations_p though) or
> when we give up redisplaying.
> 
> Calling redisplay_window may be prevented via needs_no_redisplay, unless
> we have buffer->text->redisplay, which is set in bset_update_mode_line.
> [ setting buffer->text->redisplay is rather awkward way to single mode
> line update though ]
> 
> redisplay_window may also be postponed for frames that are not visible
> or that are obscured, but it appears to be by design and not influenced
> by prevent_redisplay_optimizations_p anyway 😕
> 
> So, I see not why calling bset_update_mode_line is not sufficient to
> force mode line update in all windows associated with a single buffer.

The mode line displays quite a lot of information.  It could also
display information we don't know about (on the level of the redisplay
code), because mode-line-format supports :eval, which can execute any
Lisp.  Therefore, there could be changes to the buffer that affect the
mode line indirectly.

One problem of this kind which we had relatively recently is when the
changes in mode-line-format or some of its :eval forms yields a mode
line that is significantly taller or smaller than the previously
displayed one.  Such changes in the mode-line height generally affect
the window(s) as well, not just the mode line itself.

Moreover, you will see in the wild that force-mode-line-update is used
not just to update the mode line, but also to force a more thorough
redisplay of one or more windows.

Thus, this is not as simple a problem as you seem to think, and we
need deeper analysis and more significant changes than simply deleting
the code that you didn't understand and think is redundant.  Please
keep in mind that people who wrote that code (and I don't mean myself)
were not stupid at all, and generally knew what they were doing!

> >> If my understanding is correct,
> >> current_buffer->prevent_redisplay_optimizations_p = true does not belong
> >> to `force-mode-line-update', but rather to `restore-buffer-modified-p'.
> >
> > The purpose of force-mode-line-update is to do what its name says,
> > regardless of whether the buffer was modified or not, and how it was
> > modified.  The idea is that Lisp programs which change something that
> > they know must affect the mode line call this function to make sure
> > the mode line is redrawn with up-to-date information.  By contrast,
> > buffer modifications could be such that don't affect redisplay at all,
> > or allow redisplay to use some shortcuts and redraw only a very small
> > portion of the window.
> 
> Then, why do we need to call Fforce_mode_line_update in
> set-buffer-modified-p? Wouldn't calling bset_redisplay be better?

You have read the large comment there which attempts to answer this
question, didn't you?

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> I'm afraid that the very existence of prevent_redisplay_optimizations_p
>> flag is a mistake hiding bugs with redisplay optimizations logic.
>
> I think you should avoid making such comments until you have a better
> understanding of the redisplay logic, or else I will stop taking your
> posts in this matter seriously.

Your answer indicates that I missed something important after reading
the top comment in xdisp.c, redisplay_internal, redisplay_window, and
skimmed through a couple of other functions. But I still cannot figure
out what.

Note that I did not mean that previous committers had bad intentions. I
just think that the idea of having "disable all optimizations" flag is
causing the code quality that could be otherwise somewhat better (at
least, more explicit about why redisplay should take long path).

I think that replacing all the instances of setting this flag to more
explicit alternatives would improve the display performance and also
readability.

>> Currently, redisplay_internal has a number of conditions used to
>> determine if one or another optimization is safe to use + assertion that
>> !prevent_redisplay_optimizations_p.
>> 
>> When some code outside xdisp.c sets this flag, it is nothing but a
>> statement: "I was lazy enough to update xdisp.c properly, so I just
>> force bypassing optimizations".
>
> No.  The optimization in question, which is disabled when
> prevent_redisplay_optimizations_p flag is set for the current buffer,
> is described in the comment before the condition:
>
>   /* Optimize the case that only the line containing the cursor in the
>      selected window has changed.
>
> The prevent_redisplay_optimizations_p flag says, among other things,
> that we are not in that case, and it is tested early enough in order
> to prevent us from examining additional conditions which are just
> waste of CPU cycles (and some of them are relatively expensive).

Sure, but it is a very generic way to say this. It provides no detailed
reason what exactly changed in the buffer/window/frame that makes us
bypass this optimization.

Is it not always possible to use the other, more specific, existing
flags for redisplay to indicate that something important has been changed?

>> So, I see not why calling bset_update_mode_line is not sufficient to
>> force mode line update in all windows associated with a single buffer.
>
> The mode line displays quite a lot of information.  It could also
> display information we don't know about (on the level of the redisplay
> code), because mode-line-format supports :eval, which can execute any
> Lisp.  Therefore, there could be changes to the buffer that affect the
> mode line indirectly.

But isn't :eval processed by display_mode_lines? Once we take care to call
bset_update_mode_line, display_mode_lines is guaranteed to be executed,
AFAIU.

> One problem of this kind which we had relatively recently is when the
> changes in mode-line-format or some of its :eval forms yields a mode
> line that is significantly taller or smaller than the previously
> displayed one.  Such changes in the mode-line height generally affect
> the window(s) as well, not just the mode line itself.

Sure, and redisplay_window accounts for this when calling display_mode_lines:

      display_mode_lines (w);
      unbind_to (count1, Qnil);

      /* If mode line height has changed, arrange for a thorough
	 immediate redisplay using the correct mode line height.  */
      if (window_wants_mode_line (w)
	  && CURRENT_MODE_LINE_HEIGHT (w) != DESIRED_MODE_LINE_HEIGHT (w))
	{
	  f->fonts_changed = true;
	  w->mode_line_height = -1;
	  MATRIX_MODE_LINE_ROW (w->current_matrix)->height
	    = DESIRED_MODE_LINE_HEIGHT (w);
	}

> Moreover, you will see in the wild that force-mode-line-update is used
> not just to update the mode line, but also to force a more thorough
> redisplay of one or more windows.

Why not `force-window-update'? 

> Thus, this is not as simple a problem as you seem to think, and we
> need deeper analysis and more significant changes than simply deleting
> the code that you didn't understand and think is redundant.  Please
> keep in mind that people who wrote that code (and I don't mean myself)
> were not stupid at all, and generally knew what they were doing!

I understand, which is why I looked into git history and found that the
code in question is carried over during refactoring. Into a new function
with different meaning.

And I now looked deeper into the code, and see no obvious downsides of
removing current_buffer->prevent_redisplay_optimizations_p = true; from
force-mode-line-update specifically.
`set-buffer-modified-p' may need to be re-considered though as it is the
original place where setting prevent_redisplay_optimizations_p was done
(for different reasons).

>> > The purpose of force-mode-line-update is to do what its name says,
>> ... 
>> Then, why do we need to call Fforce_mode_line_update in
>> set-buffer-modified-p? Wouldn't calling bset_redisplay be better?
>
> You have read the large comment there which attempts to answer this
> question, didn't you?

Yup, I did. I thought bset_redisplay as an alternative, because it is
smarter than bset_update_mode_line when the buffer is displayed in
selected window (it is not necessary to assign DISPLAY_SOME flags).

However, I did miss the docstring that explicitly says that
`set-buffer-modified-p' forces mode line update. So, forcing mode line
update here is simply following the docstring and should not be
disputed.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 13:48:43 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I'm afraid that the very existence of prevent_redisplay_optimizations_p
> >> flag is a mistake hiding bugs with redisplay optimizations logic.
> >
> > I think you should avoid making such comments until you have a better
> > understanding of the redisplay logic, or else I will stop taking your
> > posts in this matter seriously.
> 
> Your answer indicates that I missed something important after reading
> the top comment in xdisp.c, redisplay_internal, redisplay_window, and
> skimmed through a couple of other functions. But I still cannot figure
> out what.

Keep studying the code, is all I can suggest.  There's much more there
than meets the eye, IME.

> Note that I did not mean that previous committers had bad intentions. I
> just think that the idea of having "disable all optimizations" flag is
> causing the code quality that could be otherwise somewhat better (at
> least, more explicit about why redisplay should take long path).

I agree that the underlying logic is somewhat subtle, and sometimes
even mysterious, and I suggested a way to make this logic more
explicit and better documented.  But removing parts of display code
because we don't always understand it is a bad idea, IME, and usually
leads to bugs.  So it is a non-starter, as far as I'm concerned.

> I think that replacing all the instances of setting this flag to more
> explicit alternatives would improve the display performance and also
> readability.

It's possible.  But to consider such suggestions, I'd need to see a
detailed proposal backed up by serious analysis.  In any case, whether
it will make performance better is not yet clear; the original
motivation for this was not a performance issue at all.

> >   /* Optimize the case that only the line containing the cursor in the
> >      selected window has changed.
> >
> > The prevent_redisplay_optimizations_p flag says, among other things,
> > that we are not in that case, and it is tested early enough in order
> > to prevent us from examining additional conditions which are just
> > waste of CPU cycles (and some of them are relatively expensive).
> 
> Sure, but it is a very generic way to say this. It provides no detailed
> reason what exactly changed in the buffer/window/frame that makes us
> bypass this optimization.

What detailed reasons would you like to see?

> Is it not always possible to use the other, more specific, existing
> flags for redisplay to indicate that something important has been changed?

Which other flags?  And why do you think they are more specific?  At
least some of them are actually _less_ specific.

> >> So, I see not why calling bset_update_mode_line is not sufficient to
> >> force mode line update in all windows associated with a single buffer.
> >
> > The mode line displays quite a lot of information.  It could also
> > display information we don't know about (on the level of the redisplay
> > code), because mode-line-format supports :eval, which can execute any
> > Lisp.  Therefore, there could be changes to the buffer that affect the
> > mode line indirectly.
> 
> But isn't :eval processed by display_mode_lines? Once we take care to call
> bset_update_mode_line, display_mode_lines is guaranteed to be executed,
> AFAIU.

Executed, but with what data?  redisplay_window is a large function,
and employs quite a few of shortcuts.  If one of those shortcuts is
taken when it shouldn't be, the data shown on the mode line might be
inaccurate, because some of the variables affecting it were not
updated.

But let me turn the table and ask you: suppose that the
prevent_redisplay_optimizations_p flag is not needed for updating the
mode line per se -- what damage could it possibly do to redisplaying
the mode line? why are you so eager to remove that setting from
force-mode-line-update?

> > One problem of this kind which we had relatively recently is when the
> > changes in mode-line-format or some of its :eval forms yields a mode
> > line that is significantly taller or smaller than the previously
> > displayed one.  Such changes in the mode-line height generally affect
> > the window(s) as well, not just the mode line itself.
> 
> Sure, and redisplay_window accounts for this when calling display_mode_lines:
> 
>       display_mode_lines (w);
>       unbind_to (count1, Qnil);
> 
>       /* If mode line height has changed, arrange for a thorough
> 	 immediate redisplay using the correct mode line height.  */
>       if (window_wants_mode_line (w)
> 	  && CURRENT_MODE_LINE_HEIGHT (w) != DESIRED_MODE_LINE_HEIGHT (w))
> 	{
> 	  f->fonts_changed = true;
> 	  w->mode_line_height = -1;
> 	  MATRIX_MODE_LINE_ROW (w->current_matrix)->height
> 	    = DESIRED_MODE_LINE_HEIGHT (w);
> 	}

And if you've read the relevant bug reports, this doesn't always work.

But this is just an example, so even if you consider it to be solved,
it tells us that there can be other issues like that.  We are trying,
slowly, to go from general flags to more specific ones, but we are not
there yet, and removing code in this situation will just produce more
buggy redisplay.  I cannot agree to that.

> > Moreover, you will see in the wild that force-mode-line-update is used
> > not just to update the mode line, but also to force a more thorough
> > redisplay of one or more windows.
> 
> Why not `force-window-update'? 

Why does it matter? they both do almost the same (when the argument to
force-window-update is a window).

> And I now looked deeper into the code, and see no obvious downsides of
> removing current_buffer->prevent_redisplay_optimizations_p = true; from
> force-mode-line-update specifically.
> `set-buffer-modified-p' may need to be re-considered though as it is the
> original place where setting prevent_redisplay_optimizations_p was done
> (for different reasons).

Again, removing this is a non-starter, except if you want to do it
locally.  If we want to make any progress in this area in general, we
should take one of the two approaches I described in my previous
message.  Anything else is simply irresponsible, and won't happen on
my watch, sorry.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Moreover, you will see in the wild that force-mode-line-update is used
> not just to update the mode line, but also to force a more thorough
> redisplay of one or more windows.

AFAIK these are bugs and we should not hide them by making
`force-mode-line-update` do more than what it's designed to do.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I'm afraid that the very existence of prevent_redisplay_optimizations_p
> flag is a mistake hiding bugs with redisplay optimizations logic.

It's not that simple: the reliable way to do redisplay is to recompute
the whole glyph matrix anew for all windows every time any change occurs.
But that's unrealistic  Instead, we decouple the redisplay from the
buffer modifications.  In addition we try to keep track of which parts
may need to be redisplayed.  For that we use various auxiliary variables
which the code performing modifications can set to tell the redisplay
about which parts may need to be redisplayed.

Such variables include the `redisplay` and the `update_mode_line` bits,
buffer vars that keep track of a buffer-interval outside of which
there has not been any buffer changes, ...

`prevent_redisplay_optimizations_p` is just another of those vars.
The problem with that var is not its existence but the fact that by now
noone knows what it means, so we can't touch that code, basically :-(

> So, I see not why calling bset_update_mode_line is not sufficient to
> force mode line update in all windows associated with a single buffer.

I suspect the `prevent_redisplay_optimizations_p` is/was not meant to make
sure the modeline is updated, but to make sure some other part of the
window is/was also updated, but the code that decides whether to redraw
that other part never looked at `update_mode_line`.

E.g. maybe some code caused the mode-line or header-line to
appear/disappear and thus called `force-mode-line-update` for that
purpose, but the redisplay failed to notice that it needed to redraw
parts of the buffer text in response o that change, so the "quick fix"
was to set `prevent_redisplay_optimizations_p`?
[ This is a completely hypothetical scenario, I have no idea how this
  code came about.  ]

> Then, why do we need to call Fforce_mode_line_update in
> set-buffer-modified-p?

I don't know if it's still needed, or (if it is) whether it's the best
way to get the result nowadays, but I know why it's done: it's simply
because Fforce_mode_line_update used not to exist and instead the
standard way to force a mode-line update was to call
`set-buffer-modified-p` (yeah, I know it's weird, but that was simply
the cheapest operation exposed to ELisp that happened to set the needed
flags for the redisplay).  `force-mode-line-update` was introduced as
a cleaner API (but still implemented by calling
`set-buffer-modified-p`), so when I "straightened things out" by making
`force-mode-line-update` set the redisplay flags directly, I made
`set-buffer-modified-p` call `Fforce_mode_line_update` to be sure that
any old code out there calling `set-buffer-modified-p` to force a mode
line update would keep working as well as before.

> Wouldn't calling bset_redisplay be better?

Maybe (depends whether the redisplay code on its own checks stores the
old modified-p value to detect when it has changed), tho I doubt it
matters very much.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 10:20:32 -0400
> 
> `prevent_redisplay_optimizations_p` is just another of those vars.
> The problem with that var is not its existence but the fact that by now
> noone knows what it means, so we can't touch that code, basically :-(

I think we can touch that code, but we need to do it carefully, and we
need to leave behind a "fire escape".

If you look at its uses (not where the flag is set, but where it is
checked), you will see that it either disables some optimization
(example: in try_window_id), or sets other variables, like
current_matrix_up_to_date_p.  In the first case, we need to go over
the places where the flag is set to true and think whether those
setters indeed need to disable each particular optimization.

In the second case, I think the flag is basically used to quickly
disable optimizations conditioned by those other variables, in a way
that prevents us to make more expensive tests.

If we understand better each of these cases -- and there aren't so
many of them -- we then could discuss whether to set the flag or not
and whether to replace it with something else, whether those are
other, more descriptive, flags or nothing.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> `force-mode-line-update' has the following FIXME:
>
>   if (!NILP (all))
>     {
>       update_mode_lines = 10;
>       /* FIXME: This can't be right.  */
>       current_buffer->prevent_redisplay_optimizations_p = true;
>     }
>   else if (buffer_window_count (current_buffer))
>     {
>       bset_update_mode_line (current_buffer);
>       current_buffer->prevent_redisplay_optimizations_p = true;
>     }
>
> This FIXME has been introduced in 655ab9a3800, shortly after
> ecda65d4f7e, which moved this code from `set-buffer-modified-p'.

ecda65d4f7e changed `force-mode-line-update` from

    (if all (with-current-buffer (other-buffer)))
    (set-buffer-modified-p (buffer-modified-p)))

to

    if (!NILP (all) || buffer_window_count (current_buffer))
      {
        update_mode_lines = 10;
        current_buffer->prevent_redisplay_optimizations_p = 1;
      }

That new code was (to the best of my knowledge) doing the same as what
the old ELisp code did (I got to that code basically by taking the sum
of the C code run by the original ELisp code and then simplifying it by
eliminating those things which canceled each other).

That code already had the same problem as the one I flagged with the
FIXME: when we do (force-mode-line-update t) the effect should be
global, so if `prevent_redisplay_optimizations_p` is needed in
`current_buffer` it should be needed in other displayed buffers as well.

> AFAIU, the purpose of disabling redisplay optimizations is avoiding the
> situation when the modification flag is unset in the buffer, but the
> buffer was actually modified, and has to be redrawn.
> If my understanding is correct,
> current_buffer->prevent_redisplay_optimizations_p = true does not belong
> to `force-mode-line-update', but rather to `restore-buffer-modified-p'.

I strongly suspect it doesn't belong in `force-mode-line-update`, indeed.

> Stefan will tell, but I'm quite sure he wrote that FIXME because
> removing that line caused regression in some situation.

Nope.  I put the FIXME simply because I realized that the code doesn't
make sense: if that line is sometimes necessary, then I'm pretty sure it's
not always sufficient.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> Cc: 64596@debbugs.gnu.org
> Date: Thu, 13 Jul 2023 13:20:23 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > AFAIU, the purpose of disabling redisplay optimizations is avoiding the
> > situation when the modification flag is unset in the buffer, but the
> > buffer was actually modified, and has to be redrawn.
> > If my understanding is correct,
> > current_buffer->prevent_redisplay_optimizations_p = true does not belong
> > to `force-mode-line-update', but rather to `restore-buffer-modified-p'.
> 
> I strongly suspect it doesn't belong in `force-mode-line-update`, indeed.

To the ALL branch or to both of them?

> > Stefan will tell, but I'm quite sure he wrote that FIXME because
> > removing that line caused regression in some situation.
> 
> Nope.  I put the FIXME simply because I realized that the code doesn't
> make sense: if that line is sometimes necessary, then I'm pretty sure it's
> not always sufficient.

Then I guess you or Ihor (or both) should try removing that line and
run with that for a while.

Alternatively, maybe in the case of ALL non-nil the code should set
the prevent_redisplay_optimizations_p flag of all the buffers that are
displayed in some window, since some redisplay optimizations are not
eligible when the mode-line is about to be updated.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Then I guess you or Ihor (or both) should try removing that line and
> run with that for a while.

Yup. That's what I am already doing. See the attached diff.

Beware though that I still haven't looked closely into your and Stefan's
emails (it is late here) and I have no idea what I am doing in this
diff.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test.diff --]
[-- Type: text/x-patch, Size: 1023 bytes --]

diff --git a/src/buffer.c b/src/buffer.c
index 0c46b201586..db7dc5c3c68 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1476,16 +1476,9 @@ DEFUN ("force-mode-line-update", Fforce_mode_line_update,
      (Lisp_Object all)
 {
   if (!NILP (all))
-    {
       update_mode_lines = 10;
-      /* FIXME: This can't be right.  */
-      current_buffer->prevent_redisplay_optimizations_p = true;
-    }
   else if (buffer_window_count (current_buffer))
-    {
       bset_update_mode_line (current_buffer);
-      current_buffer->prevent_redisplay_optimizations_p = true;
-    }
   return all;
 }
 
@@ -1502,6 +1495,8 @@ DEFUN ("set-buffer-modified-p", Fset_buffer_modified_p, Sset_buffer_modified_p,
 {
   Frestore_buffer_modified_p (flag);
 
+  if (!flag) current_buffer->prevent_redisplay_optimizations_p = true;
+
   /* Set update_mode_lines only if buffer is displayed in some window.
      Packages like jit-lock or lazy-lock preserve a buffer's modified
      state by recording/restoring the state around blocks of code.

[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Then I guess you or Ihor (or both) should try removing that line and
> run with that for a while.

FWIW, I've been running without those two lines ever since I put this FIXME.

> Alternatively, maybe in the case of ALL non-nil the code should set
> the prevent_redisplay_optimizations_p flag of all the buffers that are
> displayed in some window, since some redisplay optimizations are not
> eligible when the mode-line is about to be updated.

Any reason why the corresponding optimizations don't consult the
`update_mode_lines` var (or the `update_mode_line` buffer flag), or
maybe have redisplay start by propagating the `update_mode_line` buffer
flag to `prevent_redisplay_optimizations_p`?


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Thu, 13 Jul 2023 18:02:26 -0400
> 
> > Then I guess you or Ihor (or both) should try removing that line and
> > run with that for a while.
> 
> FWIW, I've been running without those two lines ever since I put this FIXME.

I'd be happier with the alternative I proposed, which you cite below.
Because bitter experience taught me that your, Stefan, usage patterns
evidently exclude some use cases, because bug reports pop up after
your similar changes where the display is not updated in some cases.
I'm sure the same is true for the usage patterns of anyone of us, so
doing this on a small number of systems is not enough.

> > Alternatively, maybe in the case of ALL non-nil the code should set
> > the prevent_redisplay_optimizations_p flag of all the buffers that are
> > displayed in some window, since some redisplay optimizations are not
> > eligible when the mode-line is about to be updated.
> 
> Any reason why the corresponding optimizations don't consult the
> `update_mode_lines` var (or the `update_mode_line` buffer flag), or
> maybe have redisplay start by propagating the `update_mode_line` buffer
> flag to `prevent_redisplay_optimizations_p`?

Once again, prevent_redisplay_optimizations_p is NOT (just) about
updating mode lines, it affects more optimizations.  There's also
windows_or_buffers_changed, which is even "stronger".  For example,
try_window_id checks the prevent_redisplay_optimizations_p flag and
punts if it is set, although that optimization method AFAIK has
nothing to do with redisplaying mode lines, it is only about updating
the text area.

It would be nice to analyze all those flags, make them more selective,
and understand/document better what optimizations and optional
processing are affected by each one of them.  It is a large and
somewhat ungrateful job, so if someone wants to do it by
systematically examining the situations where we set each one of those
flags and their effects on redisplay, I can offer my best help (though
I cannot afford doing this job myself).

Failing that, we could try disabling some portions of the code.  This
is not the best method of collecting such systematic information, IME,
certainly not if just a couple of people do that, because experience
teaches us that usage patterns differ wildly, and different
window-systems and different window managers sometimes have
significant effect on this stuff.  So if we want to use this
"phenomenological" approach, my suggestion is to allow disabling the
relevant parts of the display-related code at run time, controlled by
variables exposed to Lisp.  We already have a bunch of inhibit-*
variables that inhibit specific optimizations in xdisp.c; we could add
more.  Then we could set the defaults of these variables according to
our best understanding, and if and when redisplay problems are
reported, ask people to flip one or more of these inhibit-* variables
and see if the problems go away.

Again, this is not the best way, so if someone here is willing to do a
thorough job regarding this, I encourage him or her to take the first
way, although it is harder.  The advantage is that we will have a much
better understanding and documentation of these flags, and will
probably be able to improve the ad-hoc set of flags we have today and
make it easier to use and maintain.  (Ideally, these flags should be a
single bitmap with individual reasons representing the causes for
disabling optimizations, and each optimization should examine the bits
it cares about.)

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > Then I guess you or Ihor (or both) should try removing that line and
>> > run with that for a while.
>> FWIW, I've been running without those two lines ever since I put this FIXME.
> I'd be happier with the alternative I proposed, which you cite below.

I assume you assumed that my lack of problems would make me think
removing those two lines is safe :-)

I'm fully aware that when it comes to redisplay, one or two testers are
definitely far from sufficient, don't worry.

>> > Alternatively, maybe in the case of ALL non-nil the code should set
>> > the prevent_redisplay_optimizations_p flag of all the buffers that are
>> > displayed in some window, since some redisplay optimizations are not
>> > eligible when the mode-line is about to be updated.
>> 
>> Any reason why the corresponding optimizations don't consult the
>> `update_mode_lines` var (or the `update_mode_line` buffer flag), or
>> maybe have redisplay start by propagating the `update_mode_line` buffer
>> flag to `prevent_redisplay_optimizations_p`?
>
> Once again, prevent_redisplay_optimizations_p is NOT (just) about
> updating mode lines, it affects more optimizations.  There's also

I know.  I didn't mean "consult `update_mode_lines` instead of
`prevent_redisplay_optimizations_p`" but "consult `update_mode_lines` in
addition to `prevent_redisplay_optimizations_p`".

> It would be nice to analyze all those flags, make them more selective,
> and understand/document better what optimizations and optional
> processing are affected by each one of them.  It is a large and
> somewhat ungrateful job, so if someone wants to do it by
> systematically examining the situations where we set each one of those
> flags and their effects on redisplay, I can offer my best help (though
> I cannot afford doing this job myself).

I can't see it happening ever in such a systematic way.
A more pragmatic approach is the one you propose afterwards: based on
our vague understanding of how things work, make a few simplifications,
expose them to our users and then see what bug reports we get in return.

I suspect a single boolean variable (which we could call
`internal--use-old-slow-redisplay`) to control those simplifications
would be enough.

We could set it to nil on `master` and to t in the release branch.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 10:31:23 -0400
> 
> > It would be nice to analyze all those flags, make them more selective,
> > and understand/document better what optimizations and optional
> > processing are affected by each one of them.  It is a large and
> > somewhat ungrateful job, so if someone wants to do it by
> > systematically examining the situations where we set each one of those
> > flags and their effects on redisplay, I can offer my best help (though
> > I cannot afford doing this job myself).
> 
> I can't see it happening ever in such a systematic way.

I still hope it will.  It's a good way of getting familiar with the
display code, so maybe someone will step forward.

> A more pragmatic approach is the one you propose afterwards: based on
> our vague understanding of how things work, make a few simplifications,
> expose them to our users and then see what bug reports we get in return.
> 
> I suspect a single boolean variable (which we could call
> `internal--use-old-slow-redisplay`) to control those simplifications
> would be enough.

No, one variable is not enough -- it will never tell us which of the
potential flags or settings of the flag requires to be reinstated.  We
need to be able to investigate this at a finer granularity.

And the variables should be called something like
use-old-and-correct-redisplay-for-mode-line.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> No, one variable is not enough -- it will never tell us which of the
> potential flags or settings of the flag requires to be reinstated.  We
> need to be able to investigate this at a finer granularity.

I doubt it.

What we need in order to investigate is a somewhat reproducible test
case and for that we need the bug to be exposed to as many users as
possible to increase the chance that one of them bumps into
a good recipe.

The variable is not used to investigate, but to make it ethical to
expose users to those potential bugs because they can set the var to
recover the old behavior as soon as they're tired of playing the
guinea pig.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> What we need in order to investigate is a somewhat reproducible test
> case and for that we need the bug to be exposed to as many users as
> possible to increase the chance that one of them bumps into
> a good recipe.
>
> The variable is not used to investigate, but to make it ethical to
> expose users to those potential bugs because they can set the var to
> recover the old behavior as soon as they're tired of playing the
> guinea pig.

+1.
Somewhat orthogonal, but related: if there are people who want to live
dangerously (even more dangerously than on master), it might be
interesting to have some kind of flip like "give me all the cutting edge
experimental features enabled, even if not backward-compatible".

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 17:46:09 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > What we need in order to investigate is a somewhat reproducible test
> > case and for that we need the bug to be exposed to as many users as
> > possible to increase the chance that one of them bumps into
> > a good recipe.
> >
> > The variable is not used to investigate, but to make it ethical to
> > expose users to those potential bugs because they can set the var to
> > recover the old behavior as soon as they're tired of playing the
> > guinea pig.
> 
> +1.

 -1000

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 22:06:38 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Ihor Radchenko <yantar92@posteo.net>
> > Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> > Date: Fri, 14 Jul 2023 17:46:09 +0000
> > 
> > Stefan Monnier <monnier@iro.umontreal.ca> writes:
> > 
> > > What we need in order to investigate is a somewhat reproducible test
> > > case and for that we need the bug to be exposed to as many users as
> > > possible to increase the chance that one of them bumps into
> > > a good recipe.
> > >
> > > The variable is not used to investigate, but to make it ethical to
> > > expose users to those potential bugs because they can set the var to
> > > recover the old behavior as soon as they're tired of playing the
> > > guinea pig.
> > 
> > +1.
> 
>  -1000

Let me explain once again my position on this.

We currently have quite a few flags that are used by various Emacs
commands to give redisplay hints about what should be done:

  . redisplay flag of a buffer
  . redisplay flag of a window
  . redisplay flag of a frame
  . prevent_redisplay_optimizations_p flag of a buffer
  . clip_changed flag of a buffer
  . update_mode_line flag of a window
  . must_be_updated_p flag of a window
  . cursor_type_changed flag of a frame
  . face_change flag of a frame
  . update_mode_lines variable
  . windows_or_buffers_changed variable

(There are a few more, but these are the bulk.)  The 3 redisplay flags
at the beginning of this list did not exist before Emacs 24, they were
added in the hope to make the flags more selective and
self-explanatory.  But just knowing, for example, that a buffer's text
was changed says almost nothing about which optimizations can and
cannot be used, whether or not the mode line needs to be updated, nor
even if the window(s) showing the buffer need to be redrawn (since the
change could be in a portion of the buffer that is not visible in any
of the windows).

Anyway, we now have this dozen of flags and variables, and one of them
is prevent_redisplay_optimizations_p.  It makes absolutely no sense to
me to try to "solve" the "problem" of this single flag in the single
case that started this thread.  Even if this flag should not be set in
the particular place with a FIXME -- which was not convincingly
demonstrated here -- so what? what harm can that possibly make in this
specific case?  None.  What we have now might look untidy at best, but
it does work, and works well.  It took an effort to get here, but the
current situation has been stable for several years: I don't remember
any significant redisplay problems reported for the last several
years.

What we do need is to make some better order out of these flags,
understand better what does each one of them tell the display engine,
and perhaps make some of them more selective.  And we need to document
these understandings.

So this is the deal: if someone wants to try to do this job, or even
some part of it, you will have my full support, my attention, and any
form of help I can practically provide.  But I will not agree to
random poking at this or that particular flag, unless there's
convincing evidence that it causes a display bug or a significant
performance problem.  Because otherwise making changes in code that we
don't sufficiently understand can only cause bugs, and guess who gets
to work on fixing them.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> What we do need is to make some better order out of these flags,
> understand better what does each one of them tell the display engine,
> and perhaps make some of them more selective.  And we need to document
> these understandings.

I agree.
From my recent reading of the commentary in xdisp.c, one apparently
missing summary is which window and frame components are considered by
the redisplay code.

"Glyph rows." section in the top comment talk about margins + text area,
but the real redisplay_internal appears to consider (1) title bars; (2)
menu bars; (3) header line; (4) tab line; (5) mode-line; (6) window
text; (7) echo area (which is treated specially).

> ....  But I will not agree to
> random poking at this or that particular flag, unless there's
> convincing evidence that it causes a display bug or a significant
> performance problem.  Because otherwise making changes in code that we
> don't sufficiently understand can only cause bugs, and guess who gets
> to work on fixing them.

That's understandable.
But the general idea of having some kind of "experimental" flag might be
still useful in other situations. Not necessarily redisplay.
Of course, such experiments should be still weighed carefully.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 07:22:59 +0000
> 
> >From my recent reading of the commentary in xdisp.c, one apparently
> missing summary is which window and frame components are considered by
> the redisplay code.

Basically, all of them, I'd say.

> "Glyph rows." section in the top comment talk about margins + text area,
> but the real redisplay_internal appears to consider (1) title bars; (2)
> menu bars; (3) header line; (4) tab line; (5) mode-line; (6) window
> text; (7) echo area (which is treated specially).

That section indeed mentions the text area and the margins, but only
because the glyph rows distinguish between them.  The other areas you
mention either don't use our glyphs (title bar), or have no margin
areas (mode line, header line, menu bar, etc.), only the text area.

Echo area is displayed in a mini-window, which is just a window,
albeit a special one.

> > ....  But I will not agree to
> > random poking at this or that particular flag, unless there's
> > convincing evidence that it causes a display bug or a significant
> > performance problem.  Because otherwise making changes in code that we
> > don't sufficiently understand can only cause bugs, and guess who gets
> > to work on fixing them.
> 
> That's understandable.
> But the general idea of having some kind of "experimental" flag might be
> still useful in other situations. Not necessarily redisplay.
> Of course, such experiments should be still weighed carefully.

I'm okay with such experimental flags, and we did use them in the past
(still have some of them in the current sources).  I'm just saying
those should be selective enough to allow enabling/disabling a small
set of features, not all of them together.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> We currently have quite a few flags that are used by various Emacs
> commands to give redisplay hints about what should be done:
>
>   . redisplay flag of a buffer
>   . redisplay flag of a window
>   . redisplay flag of a frame
>   . update_mode_line flag of a window
>   . update_mode_lines variable
>   . windows_or_buffers_changed variable

I can explain what these are supposed to represent (IOW, if there's
a bug linked to them, I should be able to tell you whether the blame
goes to the redisplay code (which "reads" these vars) or in the
non-redisplay code (which sets these vars)).

If you read their docs and have questions, I'd love to hear them so
I can improve their doc.

>   . clip_changed flag of a buffer
    . beg_unchanged of a buffer_text
    . end_unchanged of a buffer_text

I think their intended meaning is fairly clear.  I haven't really
played with the corresponding part of the code, so I don't know if the
code really matches my expectations, but I have the impression that
there shouldn't be too many surprises.

>   . cursor_type_changed flag of a frame
>   . face_change flag of a frame

I can see what these are supposed to mean as well, but I'm less sure
that that intended meaning really matches the code (e.g., I wouldn't be
surprised to discover that one of them is also (ab)used for something
else).

>   . prevent_redisplay_optimizations_p flag of a buffer
>   . must_be_updated_p flag of a window

No idea what these mean.
The name `must_be_updated_p` makes it sound to me like it's redundant
with the `redisplay` flag above.

> (There are a few more, but these are the bulk.)  The 3 redisplay flags
> at the beginning of this list did not exist before Emacs 24, they were
> added in the hope to make the flags more selective and
> self-explanatory.

They were added exclusively to refine:

    . update_mode_lines variable
    . windows_or_buffers_changed variable

since these have a global meaning whereas in most cases only a small
number of the windows should be affected.

[ IOW, the refinement targeted use cases where there are many windows.
  I often have a hundred windows or more :-)  ]

> performance problem.  Because otherwise making changes in code that we
> don't sufficiently understand can only cause bugs,

It can introduce bugs, but in my experience when dealing with code
I don't understand, it's by breaking the code that you learn how it
works, so saying "can only cause bugs" doesn't make sense.  The change
can bring more clarity to the code, which is beneficial in the long
term, and if it introduces bugs then presumably some users will see
them, report them, and that will bring us better understanding of
the code.

That's eactly what happened when I introduced the 3 `redisplay` bits
above: it did introduce a few bugs, but overall it made the code
more clear.  My experience has been very positive there, and I hope you
wouldn't veto a similar change in the future.

> and guess who gets to work on fixing them.

AFAIK, for the bugs introduced by those `redisplay` bit: I did, as
it should.  And it wasn't terribly hard (largely because I indeed knew
(because I had decided it myself) what meant what and hence where
a problem should be fixed).


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 10:49:37 -0400
> 
> > performance problem.  Because otherwise making changes in code that we
> > don't sufficiently understand can only cause bugs,
> 
> It can introduce bugs, but in my experience when dealing with code
> I don't understand, it's by breaking the code that you learn how it
> works, so saying "can only cause bugs" doesn't make sense.

It makes a lot of sense to me.  So let's agree to disagree here.  To
reiterate my firm opinion: either someone steps forward to work on
this seriously and systematically, or we should leave this code alone,
for better and for worse.

> The change can bring more clarity to the code, which is beneficial
> in the long term, and if it introduces bugs then presumably some
> users will see them, report them, and that will bring us better
> understanding of the code.

If someone wants to lead such a project for the next 10 years or so,
maybe.  But what happens in reality is that the breaking changes are
made, and then the "guilty parties" disappear into thin air, or lose
interest.  And we are left with a broken redisplay and an unfinished
project.

> That's eactly what happened when I introduced the 3 `redisplay` bits
> above: it did introduce a few bugs, but overall it made the code
> more clear.

No, that's not "exactly" what happened.  Some of the bugs popped up
much later, among other things.  I agree that the added comments made
the situation better, but you know as well as I do that some of the
aspects of those variables are still somewhat mysterious: specifically
the meaning of windows_or_buffers_changed and update_mode_lines for
disabling optimizations and shortcuts.  Which is exactly the focus of
the current discussion.

> > and guess who gets to work on fixing them.
> 
> AFAIK, for the bugs introduced by those `redisplay` bit: I did, as
> it should.

Not all of them.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> But what happens in reality is that the breaking changes are made, and
> then the "guilty parties" disappear into thin air, or lose interest.
> And we are left with a broken redisplay and an unfinished project.

I didn't disappear into thin air, did I?

Maybe I shouldn't take this personally, but my `redisplay-bit` changes
are the only changes I'm aware of in this kind of vicinity, so I can't
help but feel that this experience is an important point of reference
for both of us.

>> That's eactly what happened when I introduced the 3 `redisplay` bits
>> above: it did introduce a few bugs, but overall it made the code
>> more clear.
> No, that's not "exactly" what happened.  Some of the bugs popped up
> much later, among other things.  I agree that the added comments made
> the situation better, but you know as well as I do that some of the
> aspects of those variables are still somewhat mysterious: specifically
> the meaning of windows_or_buffers_changed and update_mode_lines for
> disabling optimizations and shortcuts.  Which is exactly the focus of
> the current discussion.

Those aspects are not due to my changes.  If they are mysterious it's
because the remained mysterious, i.e. because I did not change them.

[ Actually, I did change them a bit to help track them down: I changed
  those vars from booleans to integers where the integer is a "code" for
  the place where it has been "mysteriously set".  And you can check
  `redisplay--all-windows-cause` and `redisplay--mode-lines-cause` to
  see how many times each of those "mysterious causes" has been
  used :-)  ]

This said, the meaning of those vars is clear, I think (they mean, that
all the windows/modelines need to be updated).
What still isn't clear at some places is the reason why they're set.

>> > and guess who gets to work on fixing them.
>> AFAIK, for the bugs introduced by those `redisplay` bit: I did, as
>> it should.
> Not all of them.

Admittedly, sometimes it's difficult to know beforehand whether the bug
may be due to those changes, so you may end up wasting your time before
you can redirect the bug to the appropriate person, but this is another
place where having that `internal--use-old-slow-redisplay` variable
would help.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 12:01:50 -0400
> 
> > But what happens in reality is that the breaking changes are made, and
> > then the "guilty parties" disappear into thin air, or lose interest.
> > And we are left with a broken redisplay and an unfinished project.
> 
> I didn't disappear into thin air, did I?

Let's not make this too personal.  Suffice it to say that I saw more
than once or twice changes in these areas which caused subtle
redisplay problems several years later, and I did have to fix a few of
them.  So what I wrote is based on facts and actual experience.

> >> That's eactly what happened when I introduced the 3 `redisplay` bits
> >> above: it did introduce a few bugs, but overall it made the code
> >> more clear.
> > No, that's not "exactly" what happened.  Some of the bugs popped up
> > much later, among other things.  I agree that the added comments made
> > the situation better, but you know as well as I do that some of the
> > aspects of those variables are still somewhat mysterious: specifically
> > the meaning of windows_or_buffers_changed and update_mode_lines for
> > disabling optimizations and shortcuts.  Which is exactly the focus of
> > the current discussion.
> 
> Those aspects are not due to my changes.  If they are mysterious it's
> because the remained mysterious, i.e. because I did not change them.

Indeed.  The above wasn't written to accuse, but to convey the fact
that we are not yet where we want to be in this regard.

> This said, the meaning of those vars is clear, I think (they mean, that
> all the windows/modelines need to be updated).

Not entirely true, AFAIU.  For example, what does update_mode_lines
have to do with preparing the menu bar?

  static void
  prepare_menu_bars (void)
  {
    bool all_windows = windows_or_buffers_changed || update_mode_lines;
    bool some_windows = REDISPLAY_SOME_P ();

or a similar reference in update_menu_bar.

Or why redisplay_internal does this:

  consider_all_windows_p = (update_mode_lines
			    || windows_or_buffers_changed);

And a couple of more "questionable" uses of that variable.

> What still isn't clear at some places is the reason why they're set.

Yes, that too.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Not entirely true, AFAIU.  For example, what does update_mode_lines
> have to do with preparing the menu bar?
>
>   static void
>   prepare_menu_bars (void)
>   {
>     bool all_windows = windows_or_buffers_changed || update_mode_lines;
>     bool some_windows = REDISPLAY_SOME_P ();

Indeed `update_mode_lines` means more than just mode-lines.  It includes
of course header-lines (of course), frame names (less obvious), and also
the menu-bar, and some of those links are indeed not 100% clear.

WDYT about the patch below?
I'll try to update the docs to clarify this.


        Stefan


diff --git a/src/window.h b/src/window.h
index 2f793ebe438..48d3fd3dc82 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1114,9 +1114,12 @@ #define WINDOW_TEXT_TO_FRAME_PIXEL_X(W, X)	\
 
 extern Lisp_Object echo_area_window;
 
-/* Non-zero if we should redraw the mode lines on the next redisplay.
+/* Non-zero if we should redraw the mode line*s* on the next redisplay.
    Usually set to a unique small integer so we can track the main causes of
-   full redisplays in `redisplay--mode-lines-cause'.  */
+   full redisplays in `redisplay--mode-lines-cause'.
+   Here "mode lines" includes also header-lines and frame names, and
+   apparently also menu-bars.  The link with header-lines is clear,
+   but a bit less so for frame names and the menu-bar.  */
 
 extern int update_mode_lines;
 
@@ -1134,6 +1137,11 @@ #define WINDOW_TEXT_TO_FRAME_PIXEL_X(W, X)	\
 extern void wset_redisplay (struct window *w);
 extern void fset_redisplay (struct frame *f);
 extern void bset_redisplay (struct buffer *b);
+
+/* Routines to indicate that the mode-lines might need to be redisplayed.
+   Just as for `update_mode_lines`, this includes header-lines, frame names
+   and menu-bars, and the link with frame names and menu-bars is still
+   unclear.   */
 extern void bset_update_mode_line (struct buffer *b);
 extern void wset_update_mode_line (struct window *w);
 /* Call this to tell redisplay to look for other windows than selected-window

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 13:15:39 -0400
> 
> > Not entirely true, AFAIU.  For example, what does update_mode_lines
> > have to do with preparing the menu bar?
> >
> >   static void
> >   prepare_menu_bars (void)
> >   {
> >     bool all_windows = windows_or_buffers_changed || update_mode_lines;
> >     bool some_windows = REDISPLAY_SOME_P ();
> 
> Indeed `update_mode_lines` means more than just mode-lines.  It includes
> of course header-lines (of course), frame names (less obvious), and also
> the menu-bar, and some of those links are indeed not 100% clear.
> 
> WDYT about the patch below?

That's an improvement, thanks.  But it still isn't everything.  For
example, what about resize_echo_area_exactly: why does it set this
variable?  Also, this variable is tested in update_tool_bar and
update_tab_bar, so the comment you suggest should mention tool bar and
tab bar as well.  Unless you know the answers to these questions, I
guess more digging is in order.

And I'd be happier if, instead of a single variable for so many
purposes we would have either several separate variables or make this
one a bitmap, where each bit says something very specific about the
part of the display that needs updating.  Any takers?

> -/* Non-zero if we should redraw the mode lines on the next redisplay.
> +/* Non-zero if we should redraw the mode line*s* on the next redisplay.
>     Usually set to a unique small integer so we can track the main causes of
> -   full redisplays in `redisplay--mode-lines-cause'.  */
> +   full redisplays in `redisplay--mode-lines-cause'.
> +   Here "mode lines" includes also header-lines and frame names, and
> +   apparently also menu-bars.  The link with header-lines is clear,
> +   but a bit less so for frame names and the menu-bar.  */

Btw, we need some function to show the information in
redisplay--mode-lines-cause in human-readable format, or at least a
documentation for how to examine it.  Without that, this variable is
probably useful only to a very small group of people.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> -/* Non-zero if we should redraw the mode lines on the next redisplay.
> +/* Non-zero if we should redraw the mode line*s* on the next redisplay.
>     Usually set to a unique small integer so we can track the main causes of
> -   full redisplays in `redisplay--mode-lines-cause'.  */
> +   full redisplays in `redisplay--mode-lines-cause'.
> +   Here "mode lines" includes also header-lines and frame names, and
> +   apparently also menu-bars.  The link with header-lines is clear,
> +   but a bit less so for frame names and the menu-bar.  */

It would be even better if *-mode-lines functions were renamed to
something more explicit. It is very disorienting that "mode-line" may
mean actual mode-line, but sometimes more than that.
A clarifying comment is an improvement, but does not make the code in
other places more readable.

Maybe use a term like "info-lines" to avoid confusion of using the same
term for multiple different meanings?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:38:14 +0000
> 
> It would be even better if *-mode-lines functions were renamed to
> something more explicit. It is very disorienting that "mode-line" may
> mean actual mode-line, but sometimes more than that.
> A clarifying comment is an improvement, but does not make the code in
> other places more readable.

If we want to do this, the job is larger yet.  We also have this:

  struct glyph_row
  {
    [...]
    /* True means row is a mode or header/tab-line.  */
    bool_bf mode_line_p : 1;

    /* True means row is a tab-line.  */
    bool_bf tab_line_p : 1;

  struct it
  {
    [...]
    /* True means window has a tab line at its top.  */
    bool_bf tab_line_p : 1;

    /* True means window has a mode line at its top.  */
    bool_bf header_line_p : 1;

  struct glyph_matrix
  {
    [...]
    /* True means window displayed in this matrix has a tab line.  */
    bool_bf tab_line_p : 1;

    /* True means window displayed in this matrix has a header
       line.  */
    bool_bf header_line_p : 1;


and the code which deals with these.  (Note that the bits are
inconsistent between glyph_row and the other two structures, and all
of them mention only 2 out of 3 attributes.)

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Or why redisplay_internal does this:
>
>   consider_all_windows_p = (update_mode_lines
> 			    || windows_or_buffers_changed);

Sorry, I skipped this part in my previous email.
I understand the above code, but I'm not sure what explanation might
be needed so you can understand it as well.  Maybe the problem is the
name `consider_all_windows_p` which suggests that all windows will be
updated, but it only says that we should loop through all the windows to
try and find those which need to be updated.

Would the patch below help?


        Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index a3464c2c375..385569d9309 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -16491,7 +16491,9 @@ redisplay_internal (void)
   int garbaged_frame_retries = 0;
 
   /* True means redisplay has to consider all windows on all
-     frames.  False, only selected_window is considered.  */
+     frames.  False, only selected_window is considered.
+     If true, the set of windows we try to update is further limited
+     according to the `redisplay` bits in buffers/windows/frames.  */
   bool consider_all_windows_p;
 
   /* True means redisplay has to redisplay the miniwindow.  */

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 14:15:41 -0400
> 
> > Or why redisplay_internal does this:
> >
> >   consider_all_windows_p = (update_mode_lines
> > 			    || windows_or_buffers_changed);
> 
> Sorry, I skipped this part in my previous email.
> I understand the above code, but I'm not sure what explanation might
> be needed so you can understand it as well.  Maybe the problem is the
> name `consider_all_windows_p` which suggests that all windows will be
> updated, but it only says that we should loop through all the windows to
> try and find those which need to be updated.
> 
> Would the patch below help?

It probably would.  (The meaning of the variable was clear to me
before this, but maybe it will help others.)

Thanks.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 14:15:41 -0400
> 
> > Or why redisplay_internal does this:
> >
> >   consider_all_windows_p = (update_mode_lines
> > 			    || windows_or_buffers_changed);
> 
> Sorry, I skipped this part in my previous email.
> I understand the above code, but I'm not sure what explanation might
> be needed so you can understand it as well.  Maybe the problem is the
> name `consider_all_windows_p` which suggests that all windows will be
> updated, but it only says that we should loop through all the windows to
> try and find those which need to be updated.

The actual issue with the above is different: it implies that
update_mode_lines _always_ indicates that more than a single mode line
should be updated (which is why we need to "consider all windows").
But is that actually true, i.e. does every place that assigns non-zero
to update_mode_lines indeed perform a change which makes it
_necessary_ to consider non-selected windows?

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> The actual issue with the above is different: it implies that
> update_mode_lines _always_ indicates that more than a single mode line
> should be updated (which is why we need to "consider all windows").
> But is that actually true, i.e. does every place that assigns non-zero
> to update_mode_lines indeed perform a change which makes it
> _necessary_ to consider non-selected windows?

At least, when the current buffer is only displayed in a single,
selected window, checking every window should not be necessary.

Compare bset_redisplay and bset_update_mode_line.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> The actual issue with the above is different: it implies that
>> update_mode_lines _always_ indicates that more than a single mode line
>> should be updated (which is why we need to "consider all windows").
>> But is that actually true, i.e. does every place that assigns non-zero
>> to update_mode_lines indeed perform a change which makes it
>> _necessary_ to consider non-selected windows?
>
> At least, when the current buffer is only displayed in a single,
> selected window, checking every window should not be necessary.

Indeed, we have a special case for that.  Before my change, it was
either "just the selected window" or "all windows".

> Compare bset_redisplay and bset_update_mode_line.

Indeed the old code updated all windows after a call to
`force-mode-line-update` even if the current buffer is only displayed in
the selected window and I somewhat preserved that :-(


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 19:43:17 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The actual issue with the above is different: it implies that
> > update_mode_lines _always_ indicates that more than a single mode line
> > should be updated (which is why we need to "consider all windows").
> > But is that actually true, i.e. does every place that assigns non-zero
> > to update_mode_lines indeed perform a change which makes it
> > _necessary_ to consider non-selected windows?
> 
> At least, when the current buffer is only displayed in a single,
> selected window, checking every window should not be necessary.

The code which sets update_mode_lines doesn't know whether the current
buffer will be displayed in a single window by the time redisplay
kicks in.  It doesn't even know whether it will still be the current
buffer by that time.  Because the Lisp program that is running and
making the changes which cause update_mode_lines to be set can change
these things before it finishes.

These global variables come from the time when the Emacs redisplay was
much less selective.  It basically has only two modes: either
redisplay only the selected window, or redisplay all the windows on
all the frames.  (Here "redisplay" means "examine for redisplay and
decide whether and which parts need that", not necessarily "actually
redraw".)  When the various 'redisplay' flags were added, the intent
was to make redisplay more selective, so that it could completely
refrain from examining windows on a certain frame, for example.  The
right way to keep advancing in that direction is to extend these flags
and maybe introduce more flags that will describe the need to
redisplay in more detail.  Global variables such as update_mode_lines
can do that only if they provide specific values to describe each
required aspect of redisplay.  But in any case, the code which sets
the variable cannot make decisions for redisplay, it can only describe
the changes for redisplay to consider.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> At least, when the current buffer is only displayed in a single,
>> selected window, checking every window should not be necessary.
>
> The code which sets update_mode_lines doesn't know whether the current
> buffer will be displayed in a single window by the time redisplay
> kicks in.  It doesn't even know whether it will still be the current
> buffer by that time.  Because the Lisp program that is running and
> making the changes which cause update_mode_lines to be set can change
> these things before it finishes.

If the selected window changes for any reason, redisplaying the
previously selected window will be queued by select_window:

  if (NILP (norecord) || EQ (norecord, Qmark_for_redisplay))
    { /* Mark the window for redisplay since the selected-window has
	 a different mode-line.  */
      wset_redisplay (XWINDOW (selected_window)); 
      wset_redisplay (w);
    }
  else
    redisplay_other_windows ();

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 05:49:45 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> At least, when the current buffer is only displayed in a single,
> >> selected window, checking every window should not be necessary.
> >
> > The code which sets update_mode_lines doesn't know whether the current
> > buffer will be displayed in a single window by the time redisplay
> > kicks in.  It doesn't even know whether it will still be the current
> > buffer by that time.  Because the Lisp program that is running and
> > making the changes which cause update_mode_lines to be set can change
> > these things before it finishes.
> 
> If the selected window changes for any reason, redisplaying the
> previously selected window will be queued by select_window:

Yes, but what does this have to do with what I wrote?

My point is that where we set update_mode_lines we cannot yet know
whether only the selected window will have to be redisplayed.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[martin rudalics]

 > These global variables come from the time when the Emacs redisplay was
 > much less selective.  It basically has only two modes: either
 > redisplay only the selected window, or redisplay all the windows on
 > all the frames.  (Here "redisplay" means "examine for redisplay and
 > decide whether and which parts need that", not necessarily "actually
 > redraw".)  When the various 'redisplay' flags were added, the intent
 > was to make redisplay more selective, so that it could completely
 > refrain from examining windows on a certain frame, for example.  The
 > right way to keep advancing in that direction is to extend these flags
 > and maybe introduce more flags that will describe the need to
 > redisplay in more detail.  Global variables such as update_mode_lines
 > can do that only if they provide specific values to describe each
 > required aspect of redisplay.  But in any case, the code which sets
 > the variable cannot make decisions for redisplay, it can only describe
 > the changes for redisplay to consider.

 From the POV of window management the situation is simple: When the size
of a window or a decoration changes, it would like to tell redisplay
about it.  But redisplay would have to tell it first which kind of
changes it's able to accept.  For example, I don't even know whether,
when running without window matrices, redisplay is capable of doing
something reasonable with the fact that the border between two adjacent
windows shifted and a third window not affected by that shift can be
spared by redisplay.  So from the POV of window management, redisplay
would have to offer a list of such flags it is able to handle and do
something reasonable about.

As an example: one of the worst things the window code does is saving a
window configuration, reading something from the minibuffer and
restoring the window configuration when done.  In nine out of ten cases
the restored configuration will not have changed and redisplay wouldn't
have to do anything.  Now Fset_window_configuration is awful in the
regard that it deletes all windows on the frame regardless of whether
the configuration changed only to "restore" them later (including that
n_leaf_windows allocation hack to find out which glyph matrices can be
safely freed).  While it is fairly easy to fix that, it's by no means
clear what and how to tell redisplay that some window did change in one
of those rare cases where it did.

Try the following simple scenario which currently breaks redisplay:

(let (config)
   (set-frame-parameter nil 'left-fringe 50)
   (y-or-n-p "?")
   (setq config (current-window-configuration))
   (setq left-fringe-width 0)
   (set-window-buffer nil (window-buffer))
   (y-or-n-p "?")
   (set-window-configuration config))

Here 'set-window-configuration' should be able to tell redisplay that it
should redisplay the window but it should be able to tell that iff the
fringe really changed which is fairly easy to find out.  Once it found
out, which kind of flag would it set?

martin

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

martin rudalics <rudalics@gmx.at> writes:

> Here 'set-window-configuration' should be able to tell redisplay that it
> should redisplay the window but it should be able to tell that iff the
> fringe really changed which is fairly easy to find out.  Once it found
> out, which kind of flag would it set?

May someone list all the possible displayed elements Emacs considers?

I think that it might be a good new API to have a single function that
marks things for redisplay. That function will accept arguments
indicating what exactly may need to be redisplayed: CURRENT_TEXT_LINE,
FRINGE, LINE_NUMBERS, CURRENT_BUFFER_TEXT, CURRENT_BUFFER_MODELINES,
ALL_MODELINES, CURRENT_FRAME, etc.

Then, every place where we request redisplay will call that single API
function, while being very explicit what kind of redisplay it has in
mind.

That API function may be implemented in backwards-compatible way - we
can just hide the existing logic inside for now and simplify later
instead of directly manipulating global and local flag variables.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca,
>  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 08:40:38 +0000
> 
> martin rudalics <rudalics@gmx.at> writes:
> 
> > Here 'set-window-configuration' should be able to tell redisplay that it
> > should redisplay the window but it should be able to tell that iff the
> > fringe really changed which is fairly easy to find out.  Once it found
> > out, which kind of flag would it set?
> 
> May someone list all the possible displayed elements Emacs considers?

What do you mean by "display element" here?  The Emacs display engine
already has a term "display element", but I think it means something
very different: character glyphs, images, compositions, etc.  See the
function get_next_display_element in xdisp.c.

So we had better agreed on clear terminology here to avoid terrible
confusion.

> I think that it might be a good new API to have a single function that
> marks things for redisplay. That function will accept arguments
> indicating what exactly may need to be redisplayed: CURRENT_TEXT_LINE,
> FRINGE, LINE_NUMBERS, CURRENT_BUFFER_TEXT, CURRENT_BUFFER_MODELINES,
> ALL_MODELINES, CURRENT_FRAME, etc.

The first step is to identify the changes in which the display engine
is interested, and design the flag(s) to communicate this information
to it.  The actual API, whether a single function or a set of macros
or something else, comes later, and is not a complicated decision to
make.

Please note that whether current text line needs to be redisplayed, or
if line numbers need to be redisplayed, or if mode line needs to be
redisplayed, is in many cases a decision that only the display engine
can make.  So you seem to be thinking in terms that are not entirely
correct.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> May someone list all the possible displayed elements Emacs considers?
>
> What do you mean by "display element" here?  The Emacs display engine
> already has a term "display element", but I think it means something
> very different: character glyphs, images, compositions, etc.  See the
> function get_next_display_element in xdisp.c.
>
> So we had better agreed on clear terminology here to avoid terrible
> confusion.

Fair point.
The top commentary in xdisp.c defines "display elements" as elements in
glyph matrix. These include characters (possibly composed) and images.

However, not everything drawn in Emacs frame is represented by display
elements :

1. Title bar is not using glyphs at all
2. Same for scroll bars
3. and menu bars
4. and tool bars
5. and window boundaries
6. and things like fill-column-indicator (AFAIU)

Further, some display elements are grouped:
1. per frame
2. per window
3. within fringes
4. within mode-line
5. within buffer text area
6. within a line of text
7. within header line
8. within tab-line

I think that the flags should provide a way to mark each of the groups
or individual glyphs for future redisplay.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: rudalics@gmx.at, monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 09:41:28 +0000
> 
> The top commentary in xdisp.c defines "display elements" as elements in
> glyph matrix. These include characters (possibly composed) and images.

They include more than that, see enum glyph_type in dispextern.h.

> However, not everything drawn in Emacs frame is represented by display
> elements :
> 
> 1. Title bar is not using glyphs at all
> 2. Same for scroll bars

These two are window-system/toolkit widgets.

> 3. and menu bars

This is either a toolkit widget or an Emacs-produced text; in the
latter case it does use our glyphs.

> 4. and tool bars

Depending on whether the tool bar is "external" or not, this could be
a special kind of window with our glyphs.

> 5. and window boundaries

On TTY frames, these are character glyphs.

> 6. and things like fill-column-indicator (AFAIU)

Those are fringe bitmaps, so they belong to the fringe.

> Further, some display elements are grouped:
> 1. per frame
> 2. per window
> 3. within fringes
> 4. within mode-line
> 5. within buffer text area
> 6. within a line of text
> 7. within header line
> 8. within tab-line

The latter 5 are in display-engine realm, and some of them are only
calculated as part of redisplay, so other code should not try to
handle that.  But yes, we definitely need (and already have) buffer
flags, window flags, and frame flags.

> I think that the flags should provide a way to mark each of the groups
> or individual glyphs for future redisplay.

Nothing knows (or should know) about glyphs except the display code.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> Date: Sun, 16 Jul 2023 10:26:04 +0200
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  From the POV of window management the situation is simple: When the size
> of a window or a decoration changes, it would like to tell redisplay
> about it.  But redisplay would have to tell it first which kind of
> changes it's able to accept.

I don't think this is required.  The information flow is
unidirectional: the functions which change the windows should tell
what they changed, and redisplay will then decide what that requires.
For example, if some windows changed their dimensions, redisplay would
need to know which dimension changed.

> For example, I don't even know whether, when running without window
> matrices, redisplay is capable of doing something reasonable with
> the fact that the border between two adjacent windows shifted and a
> third window not affected by that shift can be spared by redisplay.

If the third window displays a buffer that didn't change in any way,
then yes, the display engine should be capable of doing the above.  On
TTY frames, the display engine considers the entire frame, so it
should see that the portion corresponding to that third window didn't
change, and refrain from redrawing it.

> As an example: one of the worst things the window code does is saving a
> window configuration, reading something from the minibuffer and
> restoring the window configuration when done.  In nine out of ten cases
> the restored configuration will not have changed and redisplay wouldn't
> have to do anything.  Now Fset_window_configuration is awful in the
> regard that it deletes all windows on the frame regardless of whether
> the configuration changed only to "restore" them later (including that
> n_leaf_windows allocation hack to find out which glyph matrices can be
> safely freed).  While it is fairly easy to fix that, it's by no means
> clear what and how to tell redisplay that some window did change in one
> of those rare cases where it did.

We need to add flags that convey the information about which windows
changed what dimensions.

> (let (config)
>    (set-frame-parameter nil 'left-fringe 50)
>    (y-or-n-p "?")
>    (setq config (current-window-configuration))
>    (setq left-fringe-width 0)
>    (set-window-buffer nil (window-buffer))
>    (y-or-n-p "?")
>    (set-window-configuration config))
> 
> Here 'set-window-configuration' should be able to tell redisplay that it
> should redisplay the window but it should be able to tell that iff the
> fringe really changed which is fairly easy to find out.  Once it found
> out, which kind of flag would it set?

I'm not even sure it must find that out.  It could simply tell
redisplay that the fringe really changed.  The rest is up to the
display engine to decide.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[martin rudalics]

 > The information flow is
 > unidirectional: the functions which change the windows should tell
 > what they changed, and redisplay will then decide what that requires.
 > For example, if some windows changed their dimensions, redisplay would
 > need to know which dimension changed.

The window code should have all necessary information at hand (but for
the final height of the mode lines) when redisplay starts but would have
to know what redisplay wants it to tell.  It would be easy to reserve
one bit on each window to tell that its width, left scroll bar, left
margin, left fringe, body width, right fringe, right margin, right
scroll bar or the height, header, tab, mode line, body and scroll bar
height changed.  And one bit for each of font, line spacing, start and
point position and the scrolling statuses of the window.  Maybe also one
bit for the left and top edge within the frame and the left and top body
edge of the window.

Once it has been established what redisplay wants to know, it would be
easy to transfer practically all notifications currently performed by
frame.c to window.c.  That is, gui_set_left_fringe would no longer
SET_FRAME_GARBAGED because the window code would decide whether the
width of any left fringe of any window affected should really change and
set the left_fringe_width_changed, the body_width_changed and maybe also
the left_body_edge_changed bit accordingly.

In addition there would probably be one window_changed bit per window to
tell redisplay that one of the bits above changed and one frame_changed
bit to tell redisplay that at least one window has its window_changed
bit set.  Then we probably should try to remove any special treatment of
the selected window in this regard, since with all our excursions, code
can never tell anyway which window will be the selected one at the time
redisplay starts.

Finally, redisplay would have to merge in all buffer text and position
changes for each window showing that buffer mostly as it does now and
separately consider a few frame changes like visibility.

Information flow would still be bidirectional for the mode lines: As
soon as redisplay has calculated their respective heights and these
should change (a rare case), the window code would have to be informed
about that, store the new heights, adjust the body height, recalculate
scroll bar dimensions and leave it to redisplay to decide whether to
calculate mode line heights again to avoid infinite recursion.

martin

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Sorry, I skipped this part in my previous email.
>> I understand the above code, but I'm not sure what explanation might
>> be needed so you can understand it as well.  Maybe the problem is the
>> name `consider_all_windows_p` which suggests that all windows will be
>> updated, but it only says that we should loop through all the windows to
>> try and find those which need to be updated.
>
> The actual issue with the above is different: it implies that
> update_mode_lines _always_ indicates that more than a single mode line
> should be updated (which is why we need to "consider all windows").

How 'bout the wording below, then?


        Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index a3464c2c375..13f4fbca074 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -16490,8 +16490,10 @@ redisplay_internal (void)
   enum {MAX_GARBAGED_FRAME_RETRIES = 2 };
   int garbaged_frame_retries = 0;
 
-  /* True means redisplay has to consider all windows on all
-     frames.  False, only selected_window is considered.  */
+  /* False, means that only the selected_window needs to be updated.
+     True means that other windows may need to be updated as well,
+     so we need to consult the `redisplay` bits of
+     all windows/buffer/frames.  */
   bool consider_all_windows_p;
 
   /* True means redisplay has to redisplay the miniwindow.  */

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 18:53:19 -0400
> 
> > The actual issue with the above is different: it implies that
> > update_mode_lines _always_ indicates that more than a single mode line
> > should be updated (which is why we need to "consider all windows").
> 
> How 'bout the wording below, then?

OK, provided that you remove that redundant comma

> +  /* False, means that only the selected_window needs to be updated.
             ^
there.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> -  /* True means redisplay has to consider all windows on all
> -     frames.  False, only selected_window is considered.  */
> +  /* False, means that only the selected_window needs to be updated.
> +     True means that other windows may need to be updated as well,
> +     so we need to consult the `redisplay` bits of
> +     all windows/buffer/frames.  */
>    bool consider_all_windows_p;

BTW, is there any particular reason why windows_or_buffers_changed is
not a queue of windows/buffer to be re-displayed?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 05:52:04 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > -  /* True means redisplay has to consider all windows on all
> > -     frames.  False, only selected_window is considered.  */
> > +  /* False, means that only the selected_window needs to be updated.
> > +     True means that other windows may need to be updated as well,
> > +     so we need to consult the `redisplay` bits of
> > +     all windows/buffer/frames.  */
> >    bool consider_all_windows_p;
> 
> BTW, is there any particular reason why windows_or_buffers_changed is
> not a queue of windows/buffer to be re-displayed?

Why do we need such a queue, when the redisplay flags of buffers and
windows are supposed to tell us that already?

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> BTW, is there any particular reason why windows_or_buffers_changed is
>> not a queue of windows/buffer to be re-displayed?
>
> Why do we need such a queue, when the redisplay flags of buffers and
> windows are supposed to tell us that already?

To not loop over all the existing windows and frames in xdisp.c:506
redisplay branch.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 07:28:09 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> BTW, is there any particular reason why windows_or_buffers_changed is
> >> not a queue of windows/buffer to be re-displayed?
> >
> > Why do we need such a queue, when the redisplay flags of buffers and
> > windows are supposed to tell us that already?
> 
> To not loop over all the existing windows and frames in xdisp.c:506
> redisplay branch.

Someone will have to demonstrate that this is worth our while.
Testing a single boolean flag is not an expensive operation, while
maintaining that queue also takes CPU.  Also, some changes in a window
can affect other windows on the same frame, for example if the window
is resized.

IOW, maybe it's a good idea and maybe it isn't, but it definitely
doesn't affect the most expensive parts of redisplay.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> -  /* True means redisplay has to consider all windows on all
>> -     frames.  False, only selected_window is considered.  */
>> +  /* False, means that only the selected_window needs to be updated.
>> +     True means that other windows may need to be updated as well,
>> +     so we need to consult the `redisplay` bits of
>> +     all windows/buffer/frames.  */
>>    bool consider_all_windows_p;
>
> BTW, is there any particular reason why windows_or_buffers_changed is
> not a queue of windows/buffer to be re-displayed?

FWIW, I suspect that the loop over all windows is always
insignificant, so (in theory) we could get rid of the optimization that
looks only at the selected window in most cases and we wouldn't notice
any slowdown or measurable increase in CPU use.

So, replacing the `redisplay` bits with a queue is probably not a great
idea (especially since setting a bit is much faster than adding an
element to a queue where you need to try and avoid adding the same
element a hundred times).


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:04:30 -0400
> 
> >> -  /* True means redisplay has to consider all windows on all
> >> -     frames.  False, only selected_window is considered.  */
> >> +  /* False, means that only the selected_window needs to be updated.
> >> +     True means that other windows may need to be updated as well,
> >> +     so we need to consult the `redisplay` bits of
> >> +     all windows/buffer/frames.  */
> >>    bool consider_all_windows_p;
> >
> > BTW, is there any particular reason why windows_or_buffers_changed is
> > not a queue of windows/buffer to be re-displayed?
> 
> FWIW, I suspect that the loop over all windows is always
> insignificant, so (in theory) we could get rid of the optimization that
> looks only at the selected window in most cases and we wouldn't notice
> any slowdown or measurable increase in CPU use.

True, but with one caveat: the loop over all windows will not
trivially return from redisplay_window if the window's frame has its
redisplay flag set.  So it is enough for the frame to have this flag
set, for redisplay to have to consider all of its windows, even if the
redisplay flag of each and every one of that frame's windows is reset.
And once we decided not to return immediately after entering
redisplay_window, the processing is not insignificant, since it
involves making the window's buffer the current one, and as result
setting all the values of buffer-local variables.

So it sounds like we should make sure we don't set the frame's
redisplay flag unless most or all of its windows actually need to be
considered.  Is this so with the current code?

> So, replacing the `redisplay` bits with a queue is probably not a great
> idea (especially since setting a bit is much faster than adding an
> element to a queue where you need to try and avoid adding the same
> element a hundred times).

Agreed.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> So it sounds like we should make sure we don't set the frame's
> redisplay flag unless most or all of its windows actually need to be
> considered.  Is this so with the current code?

Indeed, the frame's `redisplay` bit means "*all* windows in that frame
need to be redisplayed", so it should be set with care.  But since the
code before that only had a single boolean for "redisplay all windows on
all frames", I was often happy enough to replace the global redisplay with
`fset_redisplay` even though it might be possible to be more specific.

`fset_redisplay` is typically used when windows are added/deleted/resized,
because it's hard at that point to know which windows really need to
be updated.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>   . prevent_redisplay_optimizations_p flag of a buffer
>>   . must_be_updated_p flag of a window
>
> No idea what these mean.
> The name `must_be_updated_p` makes it sound to me like it's redundant
> with the `redisplay` flag above.

I agree about must_be_updated_p. I had exactly same though that it is
redundant with redisplay flag when reading the code.

prevent_redisplay_optimizations_p is intuitively logical, but I still
feel confused because it appears to be redundant upon seeing redisplay
flag in the buffer. It raises a question what redisplay flag really
means - it is causing redisplay of a window/buffer or not necessarily?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:22:38 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >>   . prevent_redisplay_optimizations_p flag of a buffer
> >>   . must_be_updated_p flag of a window
> >
> > No idea what these mean.
> > The name `must_be_updated_p` makes it sound to me like it's redundant
> > with the `redisplay` flag above.
> 
> I agree about must_be_updated_p. I had exactly same though that it is
> redundant with redisplay flag when reading the code.

Look closer, please.  The name of the flag might suggest what you say,
but its usage suggests otherwise.

> prevent_redisplay_optimizations_p is intuitively logical, but I still
> feel confused because it appears to be redundant upon seeing redisplay
> flag in the buffer. It raises a question what redisplay flag really
> means - it is causing redisplay of a window/buffer or not necessarily?

Neither.

The redisplay flag means that a window or a buffer or a frame need to
be _considered_ for potential redisplay.  (Whether the consideration
will conclude there's a need to actually redraw the corresponding part
of the Emacs display is another matter -- it could conclude that all,
some parts of, or none of the objects marked for redisplay actually
need it.)

prevent_redisplay_optimizations_p says that while considering which
parts of the buffer need to cause redisplay, certain shortcuts and
optimizations designed to make redisplay faster and less expensive
must not be taken/used.

IOW, prevent_redisplay_optimizations_p must come with the redisplay
flag set on the buffer/window/frame, but after that it says a
different thing.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> I agree about must_be_updated_p. I had exactly same though that it is
>> redundant with redisplay flag when reading the code.
>
> Look closer, please.  The name of the flag might suggest what you say,
> but its usage suggests otherwise.

Do I understand correctly that prevent_display_optimizations_p in
buffer, must_be_updated_p in window, and garbaged in frames all serve
the same purpose of forcing the redisplay?

>> prevent_redisplay_optimizations_p is intuitively logical, but I still
>> feel confused because it appears to be redundant upon seeing redisplay
>> flag in the buffer. It raises a question what redisplay flag really
>> means - it is causing redisplay of a window/buffer or not necessarily?
>
> Neither.
>
> The redisplay flag means that a window or a buffer or a frame need to
> be _considered_ for potential redisplay.  (Whether the consideration
> will conclude there's a need to actually redraw the corresponding part
> of the Emacs display is another matter -- it could conclude that all,
> some parts of, or none of the objects marked for redisplay actually
> need it.)
>
> prevent_redisplay_optimizations_p says that while considering which
> parts of the buffer need to cause redisplay, certain shortcuts and
> optimizations designed to make redisplay faster and less expensive
> must not be taken/used.
>
> IOW, prevent_redisplay_optimizations_p must come with the redisplay
> flag set on the buffer/window/frame, but after that it says a
> different thing.

Then, why not use uniform naming scheme and have the buffer/window/frame
flags names as maybe_redisplay and must_redisplay instead of different
flag name for different object type?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:47:55 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I agree about must_be_updated_p. I had exactly same though that it is
> >> redundant with redisplay flag when reading the code.
> >
> > Look closer, please.  The name of the flag might suggest what you say,
> > but its usage suggests otherwise.
> 
> Do I understand correctly that prevent_display_optimizations_p in
> buffer, must_be_updated_p in window, and garbaged in frames all serve
> the same purpose of forcing the redisplay?

They all force redisplay, yes, but in different ways.  For example,
the frame's garbaged flag means the entire frame with all its windows
and decorations might need to be redrawn.  By contrast,
prevent_redisplay_optimizations_p just means "don't use some of the
redisplay optimizations that we otherwise could be tempted to try".
IOW, not redisplaying a window at all is not an "optimization".

> > IOW, prevent_redisplay_optimizations_p must come with the redisplay
> > flag set on the buffer/window/frame, but after that it says a
> > different thing.
> 
> Then, why not use uniform naming scheme and have the buffer/window/frame
> flags names as maybe_redisplay and must_redisplay instead of different
> flag name for different object type?

Historical reasons.  But such naming convention is of secondary
importance, IME.  As long as the names are not completely misleading,
and their effects are well documented, 90% of the job is done.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> I agree about must_be_updated_p. I had exactly same though that it is
>>> redundant with redisplay flag when reading the code.
>> Look closer, please.  The name of the flag might suggest what you say,
>> but its usage suggests otherwise.
> Do I understand correctly that prevent_display_optimizations_p in
> buffer, must_be_updated_p in window, and garbaged in frames all serve
> the same purpose of forcing the redisplay?

My understanding of the redisplay code is that it's split into 3 part:

1- decide which windows may need to be updated.
2- update the glyph matrix of a window.
3- update the glass by comparing the old glyph matrix and the new one.

[ The point between 1 and 2 is made visible to ELisp via
  `pre-redisplay-function`.  ]

The `redisplay` bits belong to step (1).
The `prevent_display_optimizations_p` OTOH belong to step 2, AFAIU.

BTW, I wish those 3 steps were exposed to ELisp, so the top-level of
redisplay could be moved to ELisp.  This would allow for example
`follow-mode` to pick a more appropriate order in which to process the
windows at step 2.

> Then, why not use uniform naming scheme and have the buffer/window/frame
> flags names as maybe_redisplay and must_redisplay instead of different
> flag name for different object type?

For that someone first needs to have a clear idea of what these things
do and how they relate to each other :-)


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:26:10 -0400
> 
> My understanding of the redisplay code is that it's split into 3 part:
> 
> 1- decide which windows may need to be updated.

More accurately:

  1a - decide whether all the windows need to be considered, or just
       the selected window
  1b - if all the windows need to be considered, decide for each frame
       whether it needs to be considered, and if so, consider all of
       that frame's windows

Considering a window can sometimes decide very quickly that the window
doesn't need to be redisplayed, but, as I mentioned elsewhere, when
its frame's redisplay flag is set, we never decide that, and the
frame's redisplay flag is what causes us to consider a frame in the
first place.

> 2- update the glyph matrix of a window.

This update can be partial -- that's one of the redisplay
optimizations.  That is, we update only a very small part of the glyph
matrix, as small as a single glyph row (= screen line).

> 3- update the glass by comparing the old glyph matrix and the new one.
> 
> [ The point between 1 and 2 is made visible to ELisp via
>   `pre-redisplay-function`.  ]
> 
> The `redisplay` bits belong to step (1).
> The `prevent_display_optimizations_p` OTOH belong to step 2, AFAIU.

Yes.

> BTW, I wish those 3 steps were exposed to ELisp, so the top-level of
> redisplay could be moved to ELisp.  This would allow for example
> `follow-mode` to pick a more appropriate order in which to process the
> windows at step 2.

follow-mode needs much more than that.  But if you want to be able to
tell redisplay "update only this window and nothing else", I think it
should be easy to accomplish by adding a single function and no other
changes: all you need is to call 'redisplay' (which is already
exposed) after setting the redisplay flag of a single window.

> > Then, why not use uniform naming scheme and have the buffer/window/frame
> > flags names as maybe_redisplay and must_redisplay instead of different
> > flag name for different object type?
> 
> For that someone first needs to have a clear idea of what these things
> do and how they relate to each other :-)

Right.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> 1- decide which windows may need to be updated.
>
> More accurately:
>
>   1a - decide whether all the windows need to be considered, or just
>        the selected window
>   1b - if all the windows need to be considered, decide for each frame
>        whether it needs to be considered, and if so, consider all of
>        that frame's windows
>
> Considering a window can sometimes decide very quickly that the window
> doesn't need to be redisplayed, but, as I mentioned elsewhere, when
> its frame's redisplay flag is set, we never decide that, and the
> frame's redisplay flag is what causes us to consider a frame in the
> first place.

Hmm... I'm not sure why you're saying

    frame's redisplay flag is what causes us to consider a frame in the
    first place.

AFAICT the main place where we check a frame's `redisplay` bit is in
`needs_no_redisplay` which operates on a window.

In most cases a frame's `redisplay` bit should not be set, tho some of
its windows' `redisplay` bits may be set, and we will consider this
frame and all its windows (to find those that need to be updated).

>> BTW, I wish those 3 steps were exposed to ELisp, so the top-level of
>> redisplay could be moved to ELisp.  This would allow for example
>> `follow-mode` to pick a more appropriate order in which to process the
>> windows at step 2.
>
> follow-mode needs much more than that.

If the redisplay toplevel could be hacked in ELisp, follow-mode could
call the `update-window-glyph-matrix` on the "main window" of its
window-set first, then get the resulting window-end/start and use that
to adjust/scroll its next/prev windows, and then call
`update-window-glyph-matrix` on them, thus propagating the information
in the right direction and avoiding unnecessary redisplay computations.

> But if you want to be able to tell redisplay "update only this window
> and nothing else", I think it should be easy to accomplish by adding
> a single function and no other changes: all you need is to call
> 'redisplay' (which is already exposed) after setting the redisplay
> flag of a single window.

That won't avoid redisplaying the other windows whose `redisplay` bits
had been set already for other reasons.

I guess we could try to "save the redisplay bit" of other
windows/buffers/frames, and restore them afterwards.  Also, it would be
good to be able to compute the glyph matrices of the various affected
windows before we do the actual glass update (especially if we want to
handle iteration where some part of the redisplay (e.g. jit-lock,
evaluation of mode-lines and menu bars, point moving out of scope
forcing a scroll, you name it) causes changes which require
recomputing a glyph matrix we just computed).


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 12:44:08 -0400
> 
> >   1a - decide whether all the windows need to be considered, or just
> >        the selected window
> >   1b - if all the windows need to be considered, decide for each frame
> >        whether it needs to be considered, and if so, consider all of
> >        that frame's windows
> >
> > Considering a window can sometimes decide very quickly that the window
> > doesn't need to be redisplayed, but, as I mentioned elsewhere, when
> > its frame's redisplay flag is set, we never decide that, and the
> > frame's redisplay flag is what causes us to consider a frame in the
> > first place.
> 
> Hmm... I'm not sure why you're saying
> 
>     frame's redisplay flag is what causes us to consider a frame in the
>     first place.

Because fset_redisplay does this:

  void
  fset_redisplay (struct frame *f)
  {
    redisplay_other_windows ();
    f->redisplay = true;
  }

and redisplay_other_windows sets windows_or_buffers_changed, which
then causes consider_all_windows_p to be true upon the next redisplay
cycle:

  consider_all_windows_p = (update_mode_lines
			    || windows_or_buffers_changed);

> If the redisplay toplevel could be hacked in ELisp, follow-mode could
> call the `update-window-glyph-matrix` on the "main window" of its
> window-set first, then get the resulting window-end/start and use that
> to adjust/scroll its next/prev windows, and then call
> `update-window-glyph-matrix` on them, thus propagating the information
> in the right direction and avoiding unnecessary redisplay computations.
> 
> > But if you want to be able to tell redisplay "update only this window
> > and nothing else", I think it should be easy to accomplish by adding
> > a single function and no other changes: all you need is to call
> > 'redisplay' (which is already exposed) after setting the redisplay
> > flag of a single window.
> 
> That won't avoid redisplaying the other windows whose `redisplay` bits
> had been set already for other reasons.

Yes, it will: when a window is redisplayed by redisplay_window, its
redisplay flag is reset, and it will not be redisplayed during this
cycle.  So if you have a way to redisplay a single window, you can do
that in any order you want, one window at a time, and those windows
which you redisplayed in this way will not be redisplayed until the
next cycle.

As for update-window-glyph-matrix, I don't see why that would help,
because updating the matrix without calling update_window fairly soon
after that will not produce the effect that you want.

> I guess we could try to "save the redisplay bit" of other
> windows/buffers/frames, and restore them afterwards.

What for?

> Also, it would be good to be able to compute the glyph matrices of
> the various affected windows before we do the actual glass update
> (especially if we want to handle iteration where some part of the
> redisplay (e.g. jit-lock, evaluation of mode-lines and menu bars,
> point moving out of scope forcing a scroll, you name it) causes
> changes which require recomputing a glyph matrix we just computed).

Sounds like an unnecessary complication to me, and for some situations
I'm not even sure how you can handle them, unless you thoroughly
redesign the display code.

IOW, this is not just a matter of exposing a small number of functions
to Lisp, IMO.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> Cc: yantar92@posteo.net, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 20:11:24 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > > But if you want to be able to tell redisplay "update only this window
> > > and nothing else", I think it should be easy to accomplish by adding
> > > a single function and no other changes: all you need is to call
> > > 'redisplay' (which is already exposed) after setting the redisplay
> > > flag of a single window.

Another easy-to-implement idea: give redisplay_internal a list of
windows in the order you want them redisplayed, in which case it will
follow that order instead of the current depth-first order implemented
by redisplay_windows.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Hmm... I'm not sure why you're saying
>> 
>>     frame's redisplay flag is what causes us to consider a frame in the
>>     first place.
>
> Because fset_redisplay does this:
>
>   void
>   fset_redisplay (struct frame *f)
>   {
>     redisplay_other_windows ();
>     f->redisplay = true;
>   }
>
> and redisplay_other_windows sets windows_or_buffers_changed, which
> then causes consider_all_windows_p to be true upon the next redisplay
> cycle:
>
>   consider_all_windows_p = (update_mode_lines
> 			    || windows_or_buffers_changed);

Yes, most calls to `[fbw]set_redisplay` will call
`redisplay_other_windows` and once that function has been called, all
the frames will be "considered", but that happens regardless of the
`redisplay` bit of any particular frame.

The `redisplay` bit is consulted later, once we loop over all the
windows in all those frames, where we decide which of those windows are
updated depending on the `redisplay` bits of the window, the window's
buffer, and the window's frame.

Are you maybe confused by the name of the `FRAME_REDISPLAY_P` macro,
which does *not* pay attention to the `redisplay` bit?

> IOW, this is not just a matter of exposing a small number of functions
> to Lisp, IMO.

I'm sorry if I made you think I suggested it would be a simple matter.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 14:53:03 -0400
> 
> >> Hmm... I'm not sure why you're saying
> >> 
> >>     frame's redisplay flag is what causes us to consider a frame in the
> >>     first place.
> >
> > Because fset_redisplay does this:
> >
> >   void
> >   fset_redisplay (struct frame *f)
> >   {
> >     redisplay_other_windows ();
> >     f->redisplay = true;
> >   }
> >
> > and redisplay_other_windows sets windows_or_buffers_changed, which
> > then causes consider_all_windows_p to be true upon the next redisplay
> > cycle:
> >
> >   consider_all_windows_p = (update_mode_lines
> > 			    || windows_or_buffers_changed);
> 
> Yes, most calls to `[fbw]set_redisplay` will call
> `redisplay_other_windows` and once that function has been called, all
> the frames will be "considered", but that happens regardless of the
> `redisplay` bit of any particular frame.

If we always set these flags by calling these functions, then we will
end up doing more in redisplay than needed, because update_mode_lines
and windows_or_buffers_changed also disable certain optimizations.
When someone writes code that calls fset_redisplay, he or she are
likely to think that all this does is set the redisplay flag.  Which
is not true.  If every call to fset_redisplay indeed needs to disable
those optimizations, we should have more flags, and should probably
not call the function fset_redisplay, but something else.

> The `redisplay` bit is consulted later, once we loop over all the
> windows in all those frames, where we decide which of those windows are
> updated depending on the `redisplay` bits of the window, the window's
> buffer, and the window's frame.

That's not all of the uses of this flag.  Here's one example of other
uses:

	  if (some_windows
	      && !f->redisplay
	      && !w->redisplay
	      && !XBUFFER (w->contents)->text->redisplay)
	    continue;

(This avoids updating the tool bar and the tab bar of the frame, and
there's a similar condition that avoids updating the frame's title.)

> Are you maybe confused by the name of the `FRAME_REDISPLAY_P` macro,
> which does *not* pay attention to the `redisplay` bit?

No, I don't think I'm confused about that.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> That's not all of the uses of this flag.  Here's one example of other
> uses:
>
> 	  if (some_windows
> 	      && !f->redisplay
> 	      && !w->redisplay
> 	      && !XBUFFER (w->contents)->text->redisplay)
> 	    continue;
>
> (This avoids updating the tool bar and the tab bar of the frame, and
> there's a similar condition that avoids updating the frame's title.)

Right, but notice that what we're fundamentally checking is whether `w`
(which is the frame's selected window here) needs to be updated, because
that's the one currently "occupying" the tool-bar.

FWIW, I don't know why we don't also check
`XBUFFER(w->contents)->redisplay` here.

Also, I still don't see why that makes you think:

    frame's redisplay flag is what causes us to consider a frame in the
    first place.

Indeed, a frame's redisplay flag may be such a reason, but a window's
flag or its text buffer's may be the reason instead.  A frame's
`redisplay` flag does not need to be set for us to consider it.


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 18:19:57 -0400
> 
> > That's not all of the uses of this flag.  Here's one example of other
> > uses:
> >
> > 	  if (some_windows
> > 	      && !f->redisplay
> > 	      && !w->redisplay
> > 	      && !XBUFFER (w->contents)->text->redisplay)
> > 	    continue;
> >
> > (This avoids updating the tool bar and the tab bar of the frame, and
> > there's a similar condition that avoids updating the frame's title.)
> 
> Right, but notice that what we're fundamentally checking is whether `w`
> (which is the frame's selected window here) needs to be updated, because
> that's the one currently "occupying" the tool-bar.

But testing the frame's redisplay flag will produce false positives,
because many changes that require to redraw the frame's windows don't
affect the tool bar or the frame's title.

> FWIW, I don't know why we don't also check
> `XBUFFER(w->contents)->redisplay` here.

We do:

 	  if (some_windows
 	      && !f->redisplay
 	      && !w->redisplay
 	      && !XBUFFER (w->contents)->text->redisplay)  <<<<<<<<<<<
 	    continue;

> Also, I still don't see why that makes you think:
> 
>     frame's redisplay flag is what causes us to consider a frame in the
>     first place.
> 
> Indeed, a frame's redisplay flag may be such a reason, but a window's
> flag or its text buffer's may be the reason instead.  A frame's
> `redisplay` flag does not need to be set for us to consider it.

No, my point is that fset_redisplay shouldn't set
windows_or_buffers_changed.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > That's not all of the uses of this flag.  Here's one example of other
>> > uses:
>> >
>> > 	  if (some_windows
>> > 	      && !f->redisplay
>> > 	      && !w->redisplay
>> > 	      && !XBUFFER (w->contents)->text->redisplay)
>> > 	    continue;
>> >
>> > (This avoids updating the tool bar and the tab bar of the frame, and
>> > there's a similar condition that avoids updating the frame's title.)
>> Right, but notice that what we're fundamentally checking is whether `w`
>> (which is the frame's selected window here) needs to be updated, because
>> that's the one currently "occupying" the tool-bar.
> But testing the frame's redisplay flag will produce false positives,
> because many changes that require to redraw the frame's windows don't
> affect the tool bar or the frame's title.

Of course, we have loads of false positives.  The point of the
`redisplay` flags is just to rule out things we can easily rule out.
It's very coarse.  It could be made finer, of course.

> No, my point is that fset_redisplay shouldn't set
> windows_or_buffers_changed.

Setting `windows_or_buffers_changed` there is currently necessary in
order to make sure we look at the `redisplay` bits (as opposed to
updating only the selected window).


        Stefan

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Mon, 17 Jul 2023 08:53:00 -0400
> 
> > No, my point is that fset_redisplay shouldn't set
> > windows_or_buffers_changed.
> 
> Setting `windows_or_buffers_changed` there is currently necessary in
> order to make sure we look at the `redisplay` bits (as opposed to
> updating only the selected window).

I understand; I was suggesting a possible cleanup, since this
discussion is mainly about such cleanups.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 10:22:38 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >>   . prevent_redisplay_optimizations_p flag of a buffer
> >>   . must_be_updated_p flag of a window
> >
> > No idea what these mean.
> > The name `must_be_updated_p` makes it sound to me like it's redundant
> > with the `redisplay` flag above.
> 
> I agree about must_be_updated_p. I had exactly same though that it is
> redundant with redisplay flag when reading the code.

Actually, the must_be_updated_p flag is not for xdisp.c, i.e. not for
redisplay_window and its subroutines.  It is for update_window, which
is the last, 3rd, stage of a redisplay cycle, most of the code of
which is in dispnew.c.  That's where the newly computed glyph matrix
of a window is compared to its current matrix, and Emacs decides what,
if anything, should be actually written to the glass.  What happens
with this flag is that redisplay_window and friends _set_ this flag
for windows that dispnew.c _must_ update.  The flag is tested in
dispnew.c, see there for its use and comments.

So it was wrong for me to include that flag in the list which started
this sub-thread.  Sorry about that.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ihor Radchenko <yantar92@posteo.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>, 64596@debbugs.gnu.org
>> Date: Sun, 16 Jul 2023 10:22:38 +0000
>> 
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 
>> >>   . prevent_redisplay_optimizations_p flag of a buffer
>> >>   . must_be_updated_p flag of a window
>> >
>> > No idea what these mean.
>> > The name `must_be_updated_p` makes it sound to me like it's redundant
>> > with the `redisplay` flag above.
>> 
>> I agree about must_be_updated_p. I had exactly same though that it is
>> redundant with redisplay flag when reading the code.
>
> Actually, the must_be_updated_p flag is not for xdisp.c, i.e. not for
> redisplay_window and its subroutines.  It is for update_window, which
> is the last, 3rd, stage of a redisplay cycle, most of the code of
> which is in dispnew.c.  That's where the newly computed glyph matrix
> of a window is compared to its current matrix, and Emacs decides what,
> if anything, should be actually written to the glass.  What happens
> with this flag is that redisplay_window and friends _set_ this flag
> for windows that dispnew.c _must_ update.  The flag is tested in
> dispnew.c, see there for its use and comments.

I see.

I have to say that these flag names are very disorienting.
I have first seen this flag while reading xdisp.c, and it was not obvious
at all what it does.

The name must_be_updated has no link with glyph matrix terminology, and
it is only explained in the commentary in window.h. But then, at the
point of reading commentary I was not yet familiar with xdisp...

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Sun, 16 Jul 2023 20:12:34 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Actually, the must_be_updated_p flag is not for xdisp.c, i.e. not for
> > redisplay_window and its subroutines.  It is for update_window, which
> > is the last, 3rd, stage of a redisplay cycle, most of the code of
> > which is in dispnew.c.  That's where the newly computed glyph matrix
> > of a window is compared to its current matrix, and Emacs decides what,
> > if anything, should be actually written to the glass.  What happens
> > with this flag is that redisplay_window and friends _set_ this flag
> > for windows that dispnew.c _must_ update.  The flag is tested in
> > dispnew.c, see there for its use and comments.
> 
> I see.
> 
> I have to say that these flag names are very disorienting.
> I have first seen this flag while reading xdisp.c, and it was not obvious
> at all what it does.

Well, update_frame and update_window are the relevant functions in
dispnew.c, so "must_be_updated_p" kind-of points to them.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Actually, the must_be_updated_p flag is not for xdisp.c, i.e. not for
>> > redisplay_window and its subroutines.  It is for update_window, which
>> > is the last, 3rd, stage of a redisplay cycle, most of the code of
>> > which is in dispnew.c...
>> 
>> I have to say that these flag names are very disorienting.
>> I have first seen this flag while reading xdisp.c, and it was not obvious
>> at all what it does.
>
> Well, update_frame and update_window are the relevant functions in
> dispnew.c, so "must_be_updated_p" kind-of points to them.

This does not help the confusion, unfortunately.
Or maybe the terms "redisplay" and "update" should be explained better
somewhere near the top of xdisp.c. They clearly have different meaning,
but not for untrained eye.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Mon, 17 Jul 2023 09:22:07 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Actually, the must_be_updated_p flag is not for xdisp.c, i.e. not for
> >> > redisplay_window and its subroutines.  It is for update_window, which
> >> > is the last, 3rd, stage of a redisplay cycle, most of the code of
> >> > which is in dispnew.c...
> >> 
> >> I have to say that these flag names are very disorienting.
> >> I have first seen this flag while reading xdisp.c, and it was not obvious
> >> at all what it does.
> >
> > Well, update_frame and update_window are the relevant functions in
> > dispnew.c, so "must_be_updated_p" kind-of points to them.
> 
> This does not help the confusion, unfortunately.
> Or maybe the terms "redisplay" and "update" should be explained better
> somewhere near the top of xdisp.c. They clearly have different meaning,
> but not for untrained eye.

"Update" is the general name of the 3rd step of "redisplay cycle".  It
takes its name from the function which enters this step: update_frame.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> This does not help the confusion, unfortunately.
>> Or maybe the terms "redisplay" and "update" should be explained better
>> somewhere near the top of xdisp.c. They clearly have different meaning,
>> but not for untrained eye.
>
> "Update" is the general name of the 3rd step of "redisplay cycle".  It
> takes its name from the function which enters this step: update_frame.

May someone then detail these steps in the top commentary in xdisp.c +
highlight that redisplay term is generally used when deciding if we need
to update. And update is an actual process of putting desired matrix
onto the glass.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Mon, 17 Jul 2023 12:00:02 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This does not help the confusion, unfortunately.
> >> Or maybe the terms "redisplay" and "update" should be explained better
> >> somewhere near the top of xdisp.c. They clearly have different meaning,
> >> but not for untrained eye.
> >
> > "Update" is the general name of the 3rd step of "redisplay cycle".  It
> > takes its name from the function which enters this step: update_frame.
> 
> May someone then detail these steps in the top commentary in xdisp.c +
> highlight that redisplay term is generally used when deciding if we need
> to update.

It's already in the large commentary at the beginning of xdisp.c
(search for "update_window" to find the description of "update").  It
just "drowns" in the sea of the information there

> And update is an actual process of putting desired matrix onto the
> glass.

That's a very simplistic description of what "update" does.  Even the
comment in xdisp.c says more.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> May someone then detail these steps in the top commentary in xdisp.c +
>> highlight that redisplay term is generally used when deciding if we need
>> to update.
>
> It's already in the large commentary at the beginning of xdisp.c
> (search for "update_window" to find the description of "update").  It
> just "drowns" in the sea of the information there

I saw this. But "update" is not used consistently across that
commentary.

If we read at the very beginning:

    /* New redisplay written by Gerd Moellmann <gerd@gnu.org>.
    
       Redisplay.
    
       Emacs separates the task of updating the display from code
       modifying global state, e.g. buffer text. ...
    
       Updating the display is triggered by the Lisp interpreter when it
       decides it's time to do it....

Which immediately creates an impression that "redisplay" and "update"
are referring to the same thing.
It would be nice the highlight the distinction rightaway there.

Further,

       You will find a lot of redisplay optimizations when you start
       looking at the innards of redisplay.  The overall goal of all these
       optimizations is to make redisplay fast because it is done
       frequently.  Some of these optimizations are implemented by the
       following functions:

focuses on optimizations related to window, but says nothing about
checking frames. It talks not about buffers displayed in multiple
windows either.

The rest of the commentary is diving deep into the "update" part.
Redisplay and its optimizations are not described in sufficient detail.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Tue, 18 Jul 2023 09:52:28 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> May someone then detail these steps in the top commentary in xdisp.c +
> >> highlight that redisplay term is generally used when deciding if we need
> >> to update.
> >
> > It's already in the large commentary at the beginning of xdisp.c
> > (search for "update_window" to find the description of "update").  It
> > just "drowns" in the sea of the information there
> 
> I saw this. But "update" is not used consistently across that
> commentary.

Tough.  "Update" is too general a word to limit is use.  Sorry.

> If we read at the very beginning:
> 
>     /* New redisplay written by Gerd Moellmann <gerd@gnu.org>.
>     
>        Redisplay.
>     
>        Emacs separates the task of updating the display from code
>        modifying global state, e.g. buffer text. ...
>     
>        Updating the display is triggered by the Lisp interpreter when it
>        decides it's time to do it....
> 
> Which immediately creates an impression that "redisplay" and "update"
> are referring to the same thing.

There's no practical way to avoid all the possible misinterpretations
of this large and complex text, especially when it is read for the
first time.

> It would be nice the highlight the distinction rightaway there.

If someone wants to work on making this commentary more clear, please
do.  But in general, I think we are splitting hair here.  This
commentary is intended as an introduction to reading the code and some
kind of general guidelines.  It doesn't pretend to be a detailed and
comprehensive documentation of everything that happens in the display
engine.  Rest assured that there are important aspects and algorithms
of the display engine that are completely absent from this commentary.

>        You will find a lot of redisplay optimizations when you start
>        looking at the innards of redisplay.  The overall goal of all these
>        optimizations is to make redisplay fast because it is done
>        frequently.  Some of these optimizations are implemented by the
>        following functions:
> 
> focuses on optimizations related to window, but says nothing about
> checking frames. 

Because nothing interesting happens on frames that can be described as
"redisplay optimizations".  The only situation where some important
optimizations do happen there is on TTY frames, and that _is_ covered:
see the section headed "Frame matrices".

>                   It talks not about buffers displayed in multiple
> windows either.

Again, no redisplay optimizations consider multiple windows; they all
work on a single window.  And the text doesn't describe many more
other things.  Like I said: this is intended to be an introduction and
a "guide for the perplexed", not a full and detailed documentation --
that would be a much larger job and a much longer text.

Informative additions to this commentary are welcome, of course.

> The rest of the commentary is diving deep into the "update" part.
> Redisplay and its optimizations are not described in sufficient detail.

This is a misunderstanding: where that part talks about "updating the
display", it actually means updating the window's glyph matrix.  The
only "optimization" in the "update" part is that we write to the glass
only the portions where the current and the desired glyph matrices
differ, and cannot be made identical by moving pixels on the screen
(as opposed to actually displaying new pixels).

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> I saw this. But "update" is not used consistently across that
>> commentary.
>
> Tough.  "Update" is too general a word to limit is use.  Sorry.

May it be replaced by another, more precise, term then?

>> If we read at the very beginning:
>> 
>>     /* New redisplay written by Gerd Moellmann <gerd@gnu.org>.
>>     
>>        Redisplay.
>>     
>>        Emacs separates the task of updating the display from code
>>        modifying global state, e.g. buffer text. ...
>>     
>>        Updating the display is triggered by the Lisp interpreter when it
>>        decides it's time to do it....
>> 
>> Which immediately creates an impression that "redisplay" and "update"
>> are referring to the same thing.
>
> There's no practical way to avoid all the possible misinterpretations
> of this large and complex text, especially when it is read for the
> first time.

What about the attached patch?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-xdisp.c-Clarify-the-meaning-of-update-term-in-co.patch --]
[-- Type: text/x-patch, Size: 1331 bytes --]

From 2be256093307246f8a3a62a5613467ceb6e7e9b8 Mon Sep 17 00:00:00 2001
Message-ID: <2be256093307246f8a3a62a5613467ceb6e7e9b8.1689761362.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Wed, 19 Jul 2023 13:08:54 +0300
Subject: [PATCH] * src/xdisp.c: Clarify the meaning of "update" term in
 commentary

See bug#64596.
---
 src/xdisp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/xdisp.c b/src/xdisp.c
index a3464c2c375..b306159c2f9 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -75,6 +75,13 @@ Copyright (C) 1985-2023 Free Software Foundation, Inc.
    and to make these changes visible.  Preferably it would do that in
    a moderately intelligent way, i.e. fast.
 
+   To be as fast as possible, redisplay code tries hard to avoid
+   updating unchanged parts of the display on the glass.  Redisplay
+   process starts from examining what has been changed since the
+   previous redisplay and only running the glass _update_ action when
+   necessary.  In the code below, functions and variables related to
+   updating on the glass will have _update_ in their names.
+
    Changes in buffer text can be deduced from window and buffer
    structures, and from some global variables like `beg_unchanged' and
    `end_unchanged'.  The contents of the display are additionally
-- 
2.41.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Wed, 19 Jul 2023 10:11:19 +0000
> 
> >> I saw this. But "update" is not used consistently across that
> >> commentary.
> >
> > Tough.  "Update" is too general a word to limit is use.  Sorry.
> 
> May it be replaced by another, more precise, term then?

Already done.

> What about the attached patch?

Thanks, I installed a more detailed overview of the redisplay cycle,
which I hope will make the commentary more clear.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I installed a more detailed overview of the redisplay cycle,
> which I hope will make the commentary more clear.

Thanks!
Several nitpicks:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-29&id=86f2d6d62fce90d19815503e3e99ac9c4d4585af
+    . decide which windows on which frames need their windows
                   ^buffers?
+      considered for redisplay

+   For buffer parts that have been changed since the last redisplay,
+   `redisplay_window' __constructs__ a second glyph matrix ___is constructed___,

+   part of the display by scrolling lines.  The actual update of the
+   display of each window ___by comparing the desired and the current
+   matrix___ is done by `update_window', which calls functions which draw

maybe put that clause after by `update_window'.

+   to the glass (those functions are specific to the type of the
+   window's frame: X, w32, NS, etc.).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Wed, 19 Jul 2023 15:50:20 +0000
> 
> +    . decide which windows on which frames need their windows
>                    ^buffers?
> +      considered for redisplay

Not buffers, windows.  Redisplay works by windows, not by buffers.
When it decides to work on a window, it temporarily makes that
window's buffer current.

> +   For buffer parts that have been changed since the last redisplay,
> +   `redisplay_window' __constructs__ a second glyph matrix ___is constructed___,

Fixed.

> +   part of the display by scrolling lines.  The actual update of the
> +   display of each window ___by comparing the desired and the current
> +   matrix___ is done by `update_window', which calls functions which draw
> 
> maybe put that clause after by `update_window'.

I added commas instead.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> +    . decide which windows on which frames need their windows
>>                    ^buffers?
>> +      considered for redisplay
>
> Not buffers, windows.  Redisplay works by windows, not by buffers.
> When it decides to work on a window, it temporarily makes that
> window's buffer current.

Then, the sentence is a big confusing.

. decide which windows ... need their windows considered ...

How can windows have "their windows"?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Thu, 20 Jul 2023 09:40:19 +0000
> 
> Then, the sentence is a big confusing.
> 
> . decide which windows ... need their windows considered ...
> 
> How can windows have "their windows"?

Fixed, thanks.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Which immediately creates an impression that "redisplay" and "update"
  > are referring to the same thing.
  > It would be nice the highlight the distinction rightaway there.

For me, they are two ways of thinking about the same action.
Redisplay is a little more terse and a little more technical in feel,
but concretely it means "updating the screen".

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: eliz@gnu.org, monnier@iro.umontreal.ca, 64596@debbugs.gnu.org
> Date: Thu, 20 Jul 2023 22:41:01 -0400
> 
>   > Which immediately creates an impression that "redisplay" and "update"
>   > are referring to the same thing.
>   > It would be nice the highlight the distinction rightaway there.
> 
> For me, they are two ways of thinking about the same action.
> Redisplay is a little more terse and a little more technical in feel,
> but concretely it means "updating the screen".

Yes, it does.  "Redisplay" is actually the name of the entire process,
which includes "update_frame" as its last step.

I have modified the commentary in xdisp.c to make this "30KFt view" of
redisplay more clear at first reading.

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I have modified the commentary in xdisp.c to make this "30KFt view" of
  > redisplay more clear at first reading.

Thanks for clarifying it.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  64596@debbugs.gnu.org
> Date: Fri, 14 Jul 2023 13:38:11 -0400
> 
> > No, one variable is not enough -- it will never tell us which of the
> > potential flags or settings of the flag requires to be reinstated.  We
> > need to be able to investigate this at a finer granularity.
> 
> I doubt it.
> 
> What we need in order to investigate is a somewhat reproducible test
> case and for that we need the bug to be exposed to as many users as
> possible to increase the chance that one of them bumps into
> a good recipe.
> 
> The variable is not used to investigate, but to make it ethical to
> expose users to those potential bugs because they can set the var to
> recover the old behavior as soon as they're tired of playing the
> guinea pig.

That's not what I had in mind when I proposed this approach.

What I had in mind was to investigate the need for every one of the
variables we use to disable redisplay optimizations in the various
cases: the prevent_redisplay_optimizations_p flag, the
update_mode_lines variable, the windows_or_buffers_changed variable,
etc. -- in the places in xdisp.c where these are heeded.  The purpose
was to understand better which ones should be used where and why.
This cannot be done with a single variable.

So if you want a single variable for some "ethical"
pseudo-investigation, I guess we deeply disagree here.  I'm not
interested in such "investigation".