bug#38181: Actual height of mode-line not taken into account

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

If the height of the mode-line is increased by inserting an image into
it, then that height is not taken into account in a buffer/window until
`redisplay' has been called at least once since the buffer was created.

This function can be used for testing:

(defun test-popup ()
  (interactive)
  (with-current-buffer (generate-new-buffer "*test*")
    (save-excursion
      (insert "one\ntwo\nthree\nfour\nfive"))
    (let ((win (display-buffer (current-buffer)
                               '(display-buffer-in-side-window
                                 (side . bottom)))))
      ;; (redisplay)
      (fit-window-to-buffer win))))

First we can see that if we increase the height by modifying the
`mode-line' and `mode-line-inactive' faces, then that IS taken into
account.

(set-face-attribute 'mode-line nil :height 250)
(set-face-attribute 'mode-line-inactive nil :height 250)

Now decrease the height again and try this instead:

(setq-default
 mode-line-format
 (cons (propertize " " 'display
                   (create-image "/* XPM */ static char * image[] = {
\"3 60 1 1\",
\"0 c #00aaff\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\"
};" 'xpm t :ascent 'center))
       mode-line-format))

When you call the command now, then the lower parts of the buffer are
not visible because the window is not high enough.

A work-around is to call `redisplay' before calling
`fit-window-to-buffer', which could be done by advising that function.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > (setq-default
 >   mode-line-format
 >   (cons (propertize " " 'display
 >                     (create-image "/* XPM */ static char * image[] = {
 > \"3 60 1 1\",
 > \"0 c #00aaff\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
 > \"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\"
 > };" 'xpm t :ascent 'center))
 >         mode-line-format))


When I evaluate that specification, the scroll bars are screwed up for
all windows and remain so for all unselected windows.  So this problem
is not pertinent to 'fit-window-to-buffer' only.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 13 Nov 2019 09:03:56 +0100
> 
> When I evaluate that specification, the scroll bars are screwed up for
> all windows and remain so for all unselected windows.

Doesn't happen here, FWIW.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Tue, 12 Nov 2019 17:52:59 +0100
> 
> If the height of the mode-line is increased by inserting an image into
> it, then that height is not taken into account in a buffer/window until
> `redisplay' has been called at least once since the buffer was created.

AFAIR, the mode line display is done lazily, because otherwise the
mode line would flicker.  When you change the face, that triggers a
thorough redisplay, because any change in faces does that (it could
mean many faces now have a different appearance).  Adding an image to
the mode line doesn't have that effect, which is why you need to force
redisplay manually.

So much for theory.  Now for the practical aspects: how bad is it to
have to invoke redisplay by hand in these cases?  After all, they
don't sound like something a Lisp program would do frequently.

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> So much for theory.  Now for the practical aspects: how bad is it to
> have to invoke redisplay by hand in these cases?  After all, they
> don't sound like something a Lisp program would do frequently.

Currently I do the manual redisplay by advising fit-window-to-buffer,
which I don't think can be avoided.  This results in some flickering,
so I make sure it is only done once per buffer.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 38181@debbugs.gnu.org
> Date: Fri, 15 Nov 2019 15:24:11 +0100
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So much for theory.  Now for the practical aspects: how bad is it to
> > have to invoke redisplay by hand in these cases?  After all, they
> > don't sound like something a Lisp program would do frequently.
> 
> Currently I do the manual redisplay by advising fit-window-to-buffer,
> which I don't think can be avoided.  This results in some flickering,
> so I make sure it is only done once per buffer.

Why the need to use advising?  Your recipe shows that calling
redisplay before fit-window-to-buffer also solves the problem.  Can't
you do something like that only when you add such tall images to the
mode line?

An alternative would be to scale the image so that it doesn't enlarge
the mode line, btw.  Is that possible in your use cases?

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> Why the need to use advising?  Your recipe shows that calling
> redisplay before fit-window-to-buffer also solves the problem.  Can't
> you do something like that only when you add such tall images to the
> mode line?

This is about packages like powerline, spaceline, doom-modeline and my
moody.  These packages add images to the global mode-line-format to make
them prettier.  These packages do not create any buffers of their own
and they never call fit-window-to-buffer themselves, but whenever that
function is called for a new buffer redisplay has to be run first, so
the advice is needed.

> An alternative would be to scale the image so that it doesn't enlarge
> the mode line, btw.  Is that possible in your use cases?

No because enlarging the mode-line is one of the things I did in order
to make it prettier (imo).  It's a goal not a means or side-effect.

You can see a screenshot on https://github.com/tarsius/moody.
Many other "mode-line prettifiers" also increase its height.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 38181@debbugs.gnu.org
> Date: Sat, 16 Nov 2019 00:51:19 +0100
> 
> > An alternative would be to scale the image so that it doesn't enlarge
> > the mode line, btw.  Is that possible in your use cases?
> 
> No because enlarging the mode-line is one of the things I did in order
> to make it prettier (imo).  It's a goal not a means or side-effect.

If this is to make the mode line prettier, then it should be done
once, at the beginning of a session, right?  In that case, why calling
redisplay after loading the package or enabling a feature is not a
solution?

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Cc: 38181@debbugs.gnu.org
>> Date: Sat, 16 Nov 2019 00:51:19 +0100
>>
>> > An alternative would be to scale the image so that it doesn't enlarge
>> > the mode line, btw.  Is that possible in your use cases?
>>
>> No because enlarging the mode-line is one of the things I did in order
>> to make it prettier (imo).  It's a goal not a means or side-effect.
>
> If this is to make the mode line prettier, then it should be done
> once, at the beginning of a session, right?  In that case, why calling
> redisplay after loading the package or enabling a feature is not a
> solution?

No that won't work.  Each buffer has its own mode-line so when
a new buffer is created, then its height has to be calculated.

In practice all mode-lines have the same height, so if Emacs could
use the height "that all the other buffers/windows are using" when
it does not know the actual height, then that would help.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 38181@debbugs.gnu.org
> Date: Sat, 16 Nov 2019 15:54:53 +0100
> 
> > If this is to make the mode line prettier, then it should be done
> > once, at the beginning of a session, right?  In that case, why calling
> > redisplay after loading the package or enabling a feature is not a
> > solution?
> 
> No that won't work.  Each buffer has its own mode-line so when
> a new buffer is created, then its height has to be calculated.

Ouch!

> In practice all mode-lines have the same height, so if Emacs could
> use the height "that all the other buffers/windows are using" when
> it does not know the actual height, then that would help.

fit-window-to-buffer doesn't use (or know, really) the height of the
mode line in the scenario you presented.  Instead, it gets the height
of the window's text area (which excludes the mode line and the header
line) from the data stored in the window object.  The problem is that
this data is recalculated only when the window is redisplayed, so
without the call to 'redisplay' we use stale data until the next
redisplay cycle.

However, if all the mode lines have the same height, then the problem
shouldn't have happened, and so now I wonder what am I missing.  If
you simulate the situation where the mode line changes, while keeping
its height (even if that height is unusually large), does the problem
with fit-window-to-buffer still happen?

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> The problem is that
> this data is recalculated only when the window is redisplayed, so
> without the call to 'redisplay' we use stale data until the next
> redisplay cycle.
>
> However, if all the mode lines have the same height, then the problem
> shouldn't have happened, and so now I wonder what am I missing.

I think the culprit is that we are not reusing some existing window to
display a new buffer but actually split an existing window in two and
then display the new buffer in the new window which never had its mode-
line height calculated.

> If
> you simulate the situation where the mode line changes, while keeping
> its height (even if that height is unusually large), does the problem
> with fit-window-to-buffer still happen?

No, if the mode-line (height) can be "reused", then there is no problem.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Date: Fri, 15 Nov 2019 15:48:53 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 38181@debbugs.gnu.org
> 
> > From: Jonas Bernoulli <jonas@bernoul.li>
> > Date: Tue, 12 Nov 2019 17:52:59 +0100
> > 
> > If the height of the mode-line is increased by inserting an image into
> > it, then that height is not taken into account in a buffer/window until
> > `redisplay' has been called at least once since the buffer was created.
> 
> AFAIR, the mode line display is done lazily, because otherwise the
> mode line would flicker.  When you change the face, that triggers a
> thorough redisplay, because any change in faces does that (it could
> mean many faces now have a different appearance).  Adding an image to
> the mode line doesn't have that effect, which is why you need to force
> redisplay manually.

Btw, I was wrong above: enlarging the mode-line faces also causes a
similar problem.  Try this slightly modified recipe:

  (defun test-popup ()
    (interactive)
    (set-face-attribute 'mode-line nil :height 350)
    (set-face-attribute 'mode-line-inactive nil :height 350)
    (with-current-buffer (generate-new-buffer "*test*")
      (save-excursion
	(insert "one\ntwo\nthree\nfour\nfive"))
      (let ((win (display-buffer (current-buffer)
				 '(display-buffer-in-side-window
				   (side . bottom)))))
	(fit-window-to-buffer win))))

and you will see that the buffer *test* isn't shown in its entirety,
either.

I think fit-window-to-buffer relies on window's metrics (like the
number of lines in the text area) to be up to date, and that is only
true after a window was redisplayed once since changing the mode-line
height.  Martin, is this correct?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

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

 > I think fit-window-to-buffer relies on window's metrics (like the
 > number of lines in the text area) to be up to date, and that is only
 > true after a window was redisplayed once since changing the mode-line
 > height.  Martin, is this correct?

Yes.  But, as I mentioned earlier, the problem shows up with the
scroll bar immediately when Jonas' form is evaluated, see the attached
screenshot.  Code like 'fit-window-to-buffer' works on the text area
only, it doesn't care whether a scroll bar or mode line changed size
since last redisplay.

martin

[-- Attachment #2: mode-line-vs-scroll-bar.png --]
[-- Type: image/png, Size: 18762 bytes --]

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 16 Nov 2019 09:20:27 +0100
> 
>  > I think fit-window-to-buffer relies on window's metrics (like the
>  > number of lines in the text area) to be up to date, and that is only
>  > true after a window was redisplayed once since changing the mode-line
>  > height.  Martin, is this correct?
> 
> Yes.

Then I'm not sure we can fix such use cases at all, without causing
display flickering in much more popular use cases.  Do you have any
ideas for a possible fix?  Could fit-window-to-buffer invoke
'redisplay' internally, perhaps?

> But, as I mentioned earlier, the problem shows up with the
> scroll bar immediately when Jonas' form is evaluated, see the attached
> screenshot.

That just means we (or a Lisp program which makes these mode-line
modifications) need to recreate and redisplay the scroll bars anew in
these cases, right?  It's a separate problem, right?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >>   > I think fit-window-to-buffer relies on window's metrics (like the
 >>   > number of lines in the text area) to be up to date, and that is only
 >>   > true after a window was redisplayed once since changing the mode-line
 >>   > height.  Martin, is this correct?
 >>
 >> Yes.
 >
 > Then I'm not sure we can fix such use cases at all, without causing
 > display flickering in much more popular use cases.  Do you have any
 > ideas for a possible fix?  Could fit-window-to-buffer invoke
 > 'redisplay' internally, perhaps?

It could but it's called way too often to warrant such behavior by
default.  But we could give it a separate optional argument so users
can avoid the advice.  I think Jonas could easily write and a test a
patch along this idea.

 >> But, as I mentioned earlier, the problem shows up with the
 >> scroll bar immediately when Jonas' form is evaluated, see the attached
 >> screenshot.
 >
 > That just means we (or a Lisp program which makes these mode-line
 > modifications) need to recreate and redisplay the scroll bars anew in
 > these cases, right?

Right.  But that same program could also redisplay all windows in
these cases, right?

 > It's a separate problem, right?

For some value of right ...

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 16 Nov 2019 09:57:40 +0100
> 
>  > Then I'm not sure we can fix such use cases at all, without causing
>  > display flickering in much more popular use cases.  Do you have any
>  > ideas for a possible fix?  Could fit-window-to-buffer invoke
>  > 'redisplay' internally, perhaps?
> 
> It could but it's called way too often to warrant such behavior by
> default.

What are the frequent calls?  Help and completions windows come to
mind, but those are interactive, and so I'm not sure I see why calling
redisplay would be a bad idea there.  Are there other callers which
I'm missing?

> But we could give it a separate optional argument so users can avoid
> the advice.

That's possible, of course.

>  >> But, as I mentioned earlier, the problem shows up with the
>  >> scroll bar immediately when Jonas' form is evaluated, see the attached
>  >> screenshot.
>  >
>  > That just means we (or a Lisp program which makes these mode-line
>  > modifications) need to recreate and redisplay the scroll bars anew in
>  > these cases, right?
> 
> Right.  But that same program could also redisplay all windows in
> these cases, right?

I meant that calling redisplay should do this automatically.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > What are the frequent calls?  Help and completions windows come to
 > mind, but those are interactive, and so I'm not sure I see why calling
 > redisplay would be a bad idea there.  Are there other callers which
 > I'm missing?

The functions for displaying temporary buffers when
'temp-buffer-resize-mode' is on.  I do not think that the overhead for
calculating the mode line height is excessive.  It's just that IIUC
even Jonas changes the mode line height only once per window/buffer
assignment so even for him this overhead is practically always lost.

 >> Right.  But that same program could also redisplay all windows in
 >> these cases, right?
 >
 > I meant that calling redisplay should do this automatically.

By calculating the mode line before drawing the scroll bars?

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 16 Nov 2019 20:28:10 +0100
> 
>  > What are the frequent calls?  Help and completions windows come to
>  > mind, but those are interactive, and so I'm not sure I see why calling
>  > redisplay would be a bad idea there.  Are there other callers which
>  > I'm missing?
> 
> The functions for displaying temporary buffers when
> 'temp-buffer-resize-mode' is on.  I do not think that the overhead for
> calculating the mode line height is excessive.  It's just that IIUC
> even Jonas changes the mode line height only once per window/buffer
> assignment so even for him this overhead is practically always lost.

But I thought we'd established that mode-line height calculation is
not the culprit, the culprit is the fact that a window's height is not
recomputed when the mode-line height changes.  Did I misunderstand?

>  >> Right.  But that same program could also redisplay all windows in
>  >> these cases, right?
>  >
>  > I meant that calling redisplay should do this automatically.
> 
> By calculating the mode line before drawing the scroll bars?

No, by triggering redrawing of scroll bars when we detect that the
mode line changed its height.  In this place of redisplay_window:

      display_mode_lines (w);

      /* 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);
	}

Or maybe in redisplay_internal, when we see that f->fonts_changed is
set.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > But I thought we'd established that mode-line height calculation is
 > not the culprit, the culprit is the fact that a window's height is not
 > recomputed when the mode-line height changes.  Did I misunderstand?

I don't think we contradicted each other's sayings here.  To
recapitulate what we found out so far:

(1) To operate correctly in the user's sense, 'fit-window-to-buffer'
     has to know the height of the mode line of the window it operates
     on as it will be shown by the next redisplay.

(2) When users or applications (maybe implicitly) change the mode line
     appearance, they have no generally established way to tell Emacs
     that this happened.  The display engine has to find out by herself
     but then it's already too late for telling 'fit-window-to-buffer'.

The question we yet have to resolve is now "only" _whether_ we can
safely recompute a window's height after redisplay detects that the
mode line height has changed.  This may easily trigger window change
functions which might easily change the mode line height again ...

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 17 Nov 2019 09:55:47 +0100
> 
>  > But I thought we'd established that mode-line height calculation is
>  > not the culprit, the culprit is the fact that a window's height is not
>  > recomputed when the mode-line height changes.  Did I misunderstand?
> 
> I don't think we contradicted each other's sayings here.  To
> recapitulate what we found out so far:
> 
> (1) To operate correctly in the user's sense, 'fit-window-to-buffer'
>      has to know the height of the mode line of the window it operates
>      on as it will be shown by the next redisplay.

I didn't see where fit-window-to-buffer looks at the height of the
mode line.  What did I miss?

> (2) When users or applications (maybe implicitly) change the mode line
>      appearance, they have no generally established way to tell Emacs
>      that this happened.  The display engine has to find out by herself
>      but then it's already too late for telling 'fit-window-to-buffer'.

Suppose we had a Lisp-callable function which would return the height
of the mode line of a window as per the current mode-line-format for
that window -- would that make the solution possible/easier?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I didn't see where fit-window-to-buffer looks at the height of the
 > mode line.  What did I miss?

'window-text-pixel-size' does it (triggered by the sixth argument
MODE-AND-HEADER-LINE non-nil).

 > Suppose we had a Lisp-callable function which would return the height
 > of the mode line of a window as per the current mode-line-format for
 > that window -- would that make the solution possible/easier?

Possible, I think.  If we pass it on to 'window-text-pixel-size', it
wouldn't even have to be Lisp-callable.  Still, it will penalize every
‘fit-window-to-buffer’ call (without a redisplay, though).

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 17 Nov 2019 19:15:44 +0100
> 
>  > I didn't see where fit-window-to-buffer looks at the height of the
>  > mode line.  What did I miss?
> 
> 'window-text-pixel-size' does it (triggered by the sixth argument
> MODE-AND-HEADER-LINE non-nil).

Thanks.  So the only thing that's missing is that it should call
display_mode_lines, and then look at DESIRED_MODE_LINE_HEIGHT instead
of WINDOW_MODE_LINE_HEIGHT?

>  > Suppose we had a Lisp-callable function which would return the height
>  > of the mode line of a window as per the current mode-line-format for
>  > that window -- would that make the solution possible/easier?
> 
> Possible, I think.  If we pass it on to 'window-text-pixel-size', it
> wouldn't even have to be Lisp-callable.  Still, it will penalize every
> ‘fit-window-to-buffer’ call (without a redisplay, though).

Why "penalize"?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Thanks.  So the only thing that's missing is that it should call
 > display_mode_lines, and then look at DESIRED_MODE_LINE_HEIGHT instead
 > of WINDOW_MODE_LINE_HEIGHT?

I suppose so.  But please always keep in mind that the window code
does not handle problems caused by specifications in the mode and
header lines immediately.  For example, it will not auto-resize a
one-line window when its mode line height is increased to more than
its text height.  Try with a one-line window and Jonas'

(set-face-attribute 'mode-line nil :height 250)

 >> Still, it will penalize every
 >> ‘fit-window-to-buffer’ call (without a redisplay, though).
 >
 > Why "penalize"?

Because 'fit-window-to-buffer' now has to calculate the mode line
height which it didn't before and which for the majority of users
never changes.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 18 Nov 2019 10:44:56 +0100
> 
>  > Thanks.  So the only thing that's missing is that it should call
>  > display_mode_lines, and then look at DESIRED_MODE_LINE_HEIGHT instead
>  > of WINDOW_MODE_LINE_HEIGHT?
> 
> I suppose so.  But please always keep in mind that the window code
> does not handle problems caused by specifications in the mode and
> header lines immediately.

What kind of problems are we talking about?

> For example, it will not auto-resize a one-line window when its mode
> line height is increased to more than its text height.

That's a separate issue, isn't it?

>  >> Still, it will penalize every
>  >> ‘fit-window-to-buffer’ call (without a redisplay, though).
>  >
>  > Why "penalize"?
> 
> Because 'fit-window-to-buffer' now has to calculate the mode line
> height which it didn't before and which for the majority of users
> never changes.

Is that a significant problem?  It's not like fit-window-to-buffer is
expected to be called in a tight loop, right?  Displaying a mode line
is no more expensive (actually, even usually expensive) than calling
vertical-motion to move one line, and we would never think twice
before adding a call to the latter, right?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> I suppose so.  But please always keep in mind that the window code
 >> does not handle problems caused by specifications in the mode and
 >> header lines immediately.
 >
 > What kind of problems are we talking about?

Any problems with the minimum size of windows as the one described in
the next sentence.

 >> For example, it will not auto-resize a one-line window when its mode
 >> line height is increased to more than its text height.
 >
 > That's a separate issue, isn't it?

It's similar, at least.  The scroll bar issue is another similar one.

 >> Because 'fit-window-to-buffer' now has to calculate the mode line
 >> height which it didn't before and which for the majority of users
 >> never changes.
 >
 > Is that a significant problem?

I hope it isn't.

 > It's not like fit-window-to-buffer is
 > expected to be called in a tight loop, right?  Displaying a mode line
 > is no more expensive (actually, even usually expensive) than calling
 > vertical-motion to move one line, and we would never think twice
 > before adding a call to the latter, right?

Agreed (I suppose you meant to say "even usually less expensive"
above).

martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Suppose we had a Lisp-callable function which would return the height
 > of the mode line of a window as per the current mode-line-format for
 > that window -- would that make the solution possible/easier?

Suppose I wanted to write such a function.  Then the problem is with
scenarios like your earlier

   (defun test-popup ()
     (interactive)
     (set-face-attribute 'mode-line nil :height 350)
     (set-face-attribute 'mode-line-inactive nil :height 350)
     (with-current-buffer (generate-new-buffer "*test*")
       (save-excursion
	(insert "one\ntwo\nthree\nfour\nfive"))
       (let ((win (display-buffer (current-buffer)
				 '(display-buffer-in-side-window
				   (side . bottom)))))
	(fit-window-to-buffer win))))

If I wanted to take into account the changes of the face attributes in
'fit-window-to-buffer', I'd have to set 'inhibit-free-realized-faces'
there to nil in order to apply the necessary face changes.  Wouldn't
that possibly harm our window matrices?  Can you somehow summarize how
that variable is supposed to be treated in general?  I already gave up
fighting with it in Bug#40639.

Thanks, martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 2 May 2020 20:06:38 +0200
> 
>  > Suppose we had a Lisp-callable function which would return the height
>  > of the mode line of a window as per the current mode-line-format for
>  > that window -- would that make the solution possible/easier?
> 
> Suppose I wanted to write such a function.  Then the problem is with
> scenarios like your earlier

Sorry for the belated response, there's a tsunami out there...

>    (defun test-popup ()
>      (interactive)
>      (set-face-attribute 'mode-line nil :height 350)
>      (set-face-attribute 'mode-line-inactive nil :height 350)
>      (with-current-buffer (generate-new-buffer "*test*")
>        (save-excursion
> 	(insert "one\ntwo\nthree\nfour\nfive"))
>        (let ((win (display-buffer (current-buffer)
> 				 '(display-buffer-in-side-window
> 				   (side . bottom)))))
> 	(fit-window-to-buffer win))))
> 
> If I wanted to take into account the changes of the face attributes in
> 'fit-window-to-buffer', I'd have to set 'inhibit-free-realized-faces'
> there to nil in order to apply the necessary face changes.  Wouldn't
> that possibly harm our window matrices?  Can you somehow summarize how
> that variable is supposed to be treated in general?  I already gave up
> fighting with it in Bug#40639.

I don't think I follow.  Are you saying that
inhibit-free-realized-faces is non-nil when you run Lisp
interactively, or in general in a Lisp program that was not called,
directly or indirectly, from redisplay_internal?  That should never
happen, as the code arranges for inhibit-free-realized-faces to be
reset to its original value when redisplay_internal returns.  If this
doesn't work, then we have a serious bug on our hands, and should fix
it ASAP.

This variable is supposed to be non-nil only when we are done
preparing the desired matrices and are about to call update_frame,
because that function cannot cope with faces referenced in the desired
matrices that were meanwhile freed.  This could happen if some hook
called from redisplay_internal manages to run code that decides to
free the faces.

But once update_frame is done, we don't need this variable set
anymore, and it should revert to nil soon enough, because
redisplay_internal returns soon after that.

What am I missing?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I don't think I follow.  Are you saying that
 > inhibit-free-realized-faces is non-nil when you run Lisp
 > interactively,

Yes.

 > or in general in a Lisp program that was not called,
 > directly or indirectly, from redisplay_internal?  That should never
 > happen, as the code arranges for inhibit-free-realized-faces to be
 > reset to its original value when redisplay_internal returns.  If this
 > doesn't work, then we have a serious bug on our hands, and should fix
 > it ASAP.

The problem here seems that the original value is true.  So if I'm not
entirely misguided it must be somehow set by PRODUCE_GLYPHS outside of
redisplay_internal.

 > This variable is supposed to be non-nil only when we are done
 > preparing the desired matrices and are about to call update_frame,
 > because that function cannot cope with faces referenced in the desired
 > matrices that were meanwhile freed.  This could happen if some hook
 > called from redisplay_internal manages to run code that decides to
 > free the faces.
 >
 > But once update_frame is done, we don't need this variable set
 > anymore, and it should revert to nil soon enough, because
 > redisplay_internal returns soon after that.
 >
 > What am I missing?

Your explanation coincides with how I expected this variable to behave.
It does not coincide with the behavior I see.

Thanks, martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > So if I'm not
 > entirely misguided it must be somehow set by PRODUCE_GLYPHS outside of
 > redisplay_internal.

Confirmed meanwhile.  Something is utterly broken here.  Removing the
offending setting in PRODUCE_GLYPHS automagically fixes Bug#40639 and
Bug#41071, for example.

martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

Below find two backtraces with emacs -Q where PRODUCE_GLYPHS here sets
inhibit_free_realized_faces outside the scope of redisplay_internal
(that is, while redisplaying_p is nil) such that these will not get
caught by the unbind form in redisplay_internal.

do_inhibit_free_realized_faces is the function I used to trap these.
It's defined as

void
do_inhibit_free_realized_faces (void)
{
   if (redisplaying_p)
     inhibit_free_realized_faces = true;
   else if (!inhibit_free_realized_faces)
     inhibit_free_realized_faces = true;
}

and gets called by PRODUCE_GLYPHS instead of setting
inhibit_free_realized_faces directly.

The first one comes initially:

#0  0x000000000046f836 in do_inhibit_free_realized_faces () at ../../src/xdisp.c:2043
#1  0x000000000057098e in display_line (it=0x7fffffffab00, cursor_vpos=0) at ../../src/xdisp.c:23311
#2  0x00000000004c1a8e in try_window (window=XIL(0x12c2db5), pos=..., flags=0) at ../../src/xdisp.c:19180
#3  0x00000000004a1029 in display_echo_area_1 (a1=19672496, a2=XIL(0)) at ../../src/xdisp.c:11552
#4  0x00000000004a0270 in with_echo_area_buffer (w=0x12c2db0, which=0, fn=0x4a0f3d <display_echo_area_1>, a1=19672496, a2=XIL(0)) at ../../src/xdisp.c:11314
#5  0x00000000004a0ee8 in display_echo_area (w=0x12c2db0) at ../../src/xdisp.c:11510
#6  0x00000000004a4520 in echo_area_display (update_frame_p=true) at ../../src/xdisp.c:12023
#7  0x000000000049f59e in message3_nolog (m=XIL(0x12b6be4)) at ../../src/xdisp.c:11016
#8  0x000000000049f26b in message3 (m=XIL(0x12b6be4)) at ../../src/xdisp.c:10946
#9  0x0000000000834154 in Fmessage (nargs=2, args=0x7fffffffc210) at ../../src/editfns.c:2859
#10 0x00000000008456b1 in funcall_subr (subr=0xe0e680 <Smessage>, numargs=2, args=0x7fffffffc210) at ../../src/eval.c:2847
#11 0x0000000000845287 in Ffuncall (nargs=3, args=0x7fffffffc208) at ../../src/eval.c:2794
#12 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff4225264), vector=XIL(0x7ffff4225135), maxdepth=make_fixnum(7), args_template=make_fixnum(0), nargs=0, args=0x7fffffffc728) at ../../src/bytecode.c:633
#13 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff422510d), nargs=0, arg_vector=0x7fffffffc728) at ../../src/eval.c:2989
#14 0x00000000008452cb in Ffuncall (nargs=1, args=0x7fffffffc720) at ../../src/eval.c:2796
#15 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff42252a4), vector=XIL(0x7ffff4223e0d), maxdepth=make_fixnum(25), args_template=make_fixnum(257), nargs=1, args=0x7fffffffd0d8) at ../../src/bytecode.c:633
#16 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff4223ddd), nargs=1, arg_vector=0x7fffffffd0d0) at ../../src/eval.c:2989
#17 0x00000000008452cb in Ffuncall (nargs=2, args=0x7fffffffd0c8) at ../../src/eval.c:2796
#18 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff4229434), vector=XIL(0x7ffff42254f5), maxdepth=make_fixnum(14), args_template=make_fixnum(0), nargs=0, args=0x7fffffffdc18) at ../../src/bytecode.c:633
#19 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff42254c5), nargs=0, arg_vector=0x7fffffffdc18) at ../../src/eval.c:2989
#20 0x00000000008452cb in Ffuncall (nargs=1, args=0x7fffffffdc10) at ../../src/eval.c:2796
#21 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff422a0fc), vector=XIL(0x7ffff4229605), maxdepth=make_fixnum(12), args_template=make_fixnum(0), nargs=0, args=0x7fffffffe250) at ../../src/bytecode.c:633
#22 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff42295d5), nargs=0, arg_vector=0x7fffffffe250) at ../../src/eval.c:2989
#23 0x0000000000845b42 in apply_lambda (fun=XIL(0x7ffff42295d5), args=XIL(0), count=4) at ../../src/eval.c:2926
#24 0x0000000000843d3e in eval_sub (form=XIL(0x7ffff4385a83)) at ../../src/eval.c:2318
#25 0x00000000008430ce in Feval (form=XIL(0x7ffff4385a83), lexical=XIL(0)) at ../../src/eval.c:2102
#26 0x0000000000764b7f in top_level_2 () at ../../src/keyboard.c:1100
#27 0x000000000084126b in internal_condition_case (bfun=0x764b5c <top_level_2>, handlers=XIL(0x90), hfun=0x76455b <cmd_error>) at ../../src/eval.c:1355
#28 0x0000000000764bc7 in top_level_1 (ignore=XIL(0)) at ../../src/keyboard.c:1108
#29 0x000000000084071f in internal_catch (tag=XIL(0xd200), func=0x764b81 <top_level_1>, arg=XIL(0)) at ../../src/eval.c:1116
#30 0x0000000000764aa6 in command_loop () at ../../src/keyboard.c:1069
#31 0x0000000000764042 in recursive_edit_1 () at ../../src/keyboard.c:714
#32 0x000000000076423a in Frecursive_edit () at ../../src/keyboard.c:786
#33 0x00000000007603bd in main (argc=2, argv=0x7fffffffe7c8) at ../../src/emacs.c:2035

Lisp Backtrace:
"message" (0xffffc210)
"display-startup-echo-area-message" (0xffffc728)
"command-line-1" (0xffffd0d0)
"command-line" (0xffffdc18)
"normal-top-level" (0xffffe250)
(gdb)

After now evaluating

(setq inhibit-free-realized-faces nil)

the second one comes up as:

#0  0x000000000046f836 in do_inhibit_free_realized_faces () at ../../src/xdisp.c:2043
#1  0x000000000057098e in display_line (it=0x7fffffffa4c0, cursor_vpos=0) at ../../src/xdisp.c:23311
#2  0x00000000004c1a8e in try_window (window=XIL(0x12c2db5), pos=..., flags=0) at ../../src/xdisp.c:19180
#3  0x00000000004a1029 in display_echo_area_1 (a1=19672496, a2=XIL(0)) at ../../src/xdisp.c:11552
#4  0x00000000004a0270 in with_echo_area_buffer (w=0x12c2db0, which=0, fn=0x4a0f3d <display_echo_area_1>, a1=19672496, a2=XIL(0)) at ../../src/xdisp.c:11314
#5  0x00000000004a0ee8 in display_echo_area (w=0x12c2db0) at ../../src/xdisp.c:11510
#6  0x00000000004a4520 in echo_area_display (update_frame_p=true) at ../../src/xdisp.c:12023
#7  0x000000000049f59e in message3_nolog (m=XIL(0xf07bf4)) at ../../src/xdisp.c:11016
#8  0x000000000049f26b in message3 (m=XIL(0xf07bf4)) at ../../src/xdisp.c:10946
#9  0x0000000000834154 in Fmessage (nargs=2, args=0x7fffffffbd48) at ../../src/editfns.c:2859
#10 0x00000000008456b1 in funcall_subr (subr=0xe0e680 <Smessage>, numargs=2, args=0x7fffffffbd48) at ../../src/eval.c:2847
#11 0x0000000000845287 in Ffuncall (nargs=3, args=0x7fffffffbd40) at ../../src/eval.c:2794
#12 0x00000000008440f9 in Fapply (nargs=3, args=0x7fffffffbd40) at ../../src/eval.c:2381
#13 0x00000000008456b1 in funcall_subr (subr=0xe0f100 <Sapply>, numargs=3, args=0x7fffffffbd40) at ../../src/eval.c:2847
#14 0x0000000000845287 in Ffuncall (nargs=4, args=0x7fffffffbd38) at ../../src/eval.c:2794
#15 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff402e244), vector=XIL(0x7ffff402e075), maxdepth=make_fixnum(7), args_template=make_fixnum(385), nargs=2, args=0x7fffffffc238) at ../../src/bytecode.c:633
#16 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff402e045), nargs=2, arg_vector=0x7fffffffc230) at ../../src/eval.c:2989
#17 0x00000000008452cb in Ffuncall (nargs=3, args=0x7fffffffc228) at ../../src/eval.c:2796
#18 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff402e9cc), vector=XIL(0x7ffff402df45), maxdepth=make_fixnum(5), args_template=make_fixnum(256), nargs=1, args=0x7fffffffc6e8) at ../../src/bytecode.c:633
#19 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff402df15), nargs=1, arg_vector=0x7fffffffc6e0) at ../../src/eval.c:2989
#20 0x00000000008452cb in Ffuncall (nargs=2, args=0x7fffffffc6d8) at ../../src/eval.c:2796
#21 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff4031b2c), vector=XIL(0x7ffff40318ed), maxdepth=make_fixnum(4), args_template=make_fixnum(0), nargs=0, args=0x7fffffffcbb0) at ../../src/bytecode.c:633
#22 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff40318bd), nargs=0, arg_vector=0x7fffffffcbb0) at ../../src/eval.c:2989
#23 0x00000000008452cb in Ffuncall (nargs=1, args=0x7fffffffcba8) at ../../src/eval.c:2796
#24 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff4031b6c), vector=XIL(0x7ffff4031865), maxdepth=make_fixnum(1), args_template=make_fixnum(0), nargs=0, args=0x7fffffffd1e0) at ../../src/bytecode.c:633
#25 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff403183d), nargs=0, arg_vector=0x7fffffffd1e0) at ../../src/eval.c:2989
#26 0x00000000008452cb in Ffuncall (nargs=1, args=0x7fffffffd1d8) at ../../src/eval.c:2796
#27 0x0000000000844088 in Fapply (nargs=2, args=0x7fffffffd1d8) at ../../src/eval.c:2377
#28 0x00000000008456b1 in funcall_subr (subr=0xe0f100 <Sapply>, numargs=2, args=0x7fffffffd1d8) at ../../src/eval.c:2847
#29 0x0000000000845287 in Ffuncall (nargs=3, args=0x7fffffffd1d0) at ../../src/eval.c:2794
#30 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff438e57c), vector=XIL(0x7ffff438e445), maxdepth=make_fixnum(10), args_template=make_fixnum(257), nargs=1, args=0x7fffffffd730) at ../../src/bytecode.c:633
#31 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff438e415), nargs=1, arg_vector=0x7fffffffd728) at ../../src/eval.c:2989
#32 0x00000000008452cb in Ffuncall (nargs=2, args=0x7fffffffd720) at ../../src/eval.c:2796
#33 0x0000000000844bb7 in call1 (fn=XIL(0xcf30), arg1=XIL(0x172d2f5)) at ../../src/eval.c:2654
#34 0x000000000076e3ae in timer_check_2 (timers=XIL(0), idle_timers=XIL(0x170e5b3)) at ../../src/keyboard.c:4340
#35 0x000000000076e4e3 in timer_check () at ../../src/keyboard.c:4402
#36 0x000000000076c242 in readable_events (flags=1) at ../../src/keyboard.c:3401
#37 0x0000000000778a4e in get_input_pending (flags=1) at ../../src/keyboard.c:6800
#38 0x0000000000781f2b in detect_input_pending_run_timers (do_display=true) at ../../src/keyboard.c:10361
#39 0x00000000008aeb95 in wait_reading_process_output (time_limit=30, nsecs=0, read_kbd=-1, do_display=true, wait_for_cell=XIL(0), wait_proc=0x0, just_wait_proc=0) at ../../src/process.c:5701
#40 0x000000000042a466 in sit_for (timeout=make_fixnum(30), reading=true, display_option=1) at ../../src/dispnew.c:6077
#41 0x0000000000769957 in read_char (commandflag=1, map=XIL(0x1709e33), prev_event=XIL(0), used_mouse_menu=0x7fffffffe13f, end_time=0x0) at ../../src/keyboard.c:2738
#42 0x0000000000780011 in read_key_sequence (keybuf=0x7fffffffe2d0, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9547
#43 0x00000000007653c8 in command_loop_1 () at ../../src/keyboard.c:1350
#44 0x000000000084126b in internal_condition_case (bfun=0x764f4c <command_loop_1>, handlers=XIL(0x90), hfun=0x76455b <cmd_error>) at ../../src/eval.c:1355
#45 0x0000000000764b31 in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1091
#46 0x000000000084071f in internal_catch (tag=XIL(0xd200), func=0x764b04 <command_loop_2>, arg=XIL(0)) at ../../src/eval.c:1116
#47 0x0000000000764acf in command_loop () at ../../src/keyboard.c:1070
#48 0x0000000000764042 in recursive_edit_1 () at ../../src/keyboard.c:714
#49 0x000000000076423a in Frecursive_edit () at ../../src/keyboard.c:786
#50 0x00000000007603bd in main (argc=2, argv=0x7fffffffe7c8) at ../../src/emacs.c:2035

Lisp Backtrace:
"message" (0xffffbd48)
"apply" (0xffffbd40)
"eldoc-minibuffer-message" (0xffffc230)
"eldoc-message" (0xffffc6e0)
"eldoc-print-current-symbol-info" (0xffffcbb0)
0xf4031838 PVEC_COMPILED
"apply" (0xffffd1d8)
"timer-event-handler" (0xffffd728)
(gdb)

And below is one where I load my usual customizations

#0  0x000000000046f836 in do_inhibit_free_realized_faces () at ../../src/xdisp.c:2043
#1  0x000000000057098e in display_line (it=0x7fffffff8a30, cursor_vpos=0) at ../../src/xdisp.c:23311
#2  0x00000000004c1a8e in try_window (window=XIL(0x12c5db5), pos=..., flags=0) at ../../src/xdisp.c:19180
#3  0x00000000004a1029 in display_echo_area_1 (a1=19684784, a2=XIL(0)) at ../../src/xdisp.c:11552
#4  0x00000000004a0270 in with_echo_area_buffer (w=0x12c5db0, which=0, fn=0x4a0f3d <display_echo_area_1>, a1=19684784, a2=XIL(0)) at ../../src/xdisp.c:11314
#5  0x00000000004a0ee8 in display_echo_area (w=0x12c5db0) at ../../src/xdisp.c:11510
#6  0x00000000004a4520 in echo_area_display (update_frame_p=true) at ../../src/xdisp.c:12023
#7  0x000000000049f59e in message3_nolog (m=XIL(0xeef4b4)) at ../../src/xdisp.c:11016
#8  0x000000000049f26b in message3 (m=XIL(0xeef4b4)) at ../../src/xdisp.c:10946
#9  0x000000000049f891 in message_with_string (m=0x9a619d "Loading %s...", string=XIL(0x7ffff3d31104), log=true) at ../../src/xdisp.c:11089
#10 0x0000000000881110 in Fload (file=XIL(0x7ffff3d31104), noerror=XIL(0), nomessage=XIL(0), nosuffix=XIL(0), must_suffix=XIL(0)) at ../../src/lread.c:1440
#11 0x0000000000845883 in funcall_subr (subr=0xe11c40 <Sload>, numargs=1, args=0x7fffffffa3c8) at ../../src/eval.c:2879
#12 0x0000000000845287 in Ffuncall (nargs=2, args=0x7fffffffa3c0) at ../../src/eval.c:2794
#13 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff3d4ef0c), vector=XIL(0x7ffff3d4ec45), maxdepth=make_fixnum(14), args_template=make_fixnum(257), nargs=1, args=0x7fffffffa960) at ../../src/bytecode.c:633
#14 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff3d4ec15), nargs=1, arg_vector=0x7fffffffa958) at ../../src/eval.c:2989
#15 0x00000000008452cb in Ffuncall (nargs=2, args=0x7fffffffa950) at ../../src/eval.c:2796
#16 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff3d4f8f4), vector=XIL(0x7ffff3d4e9ad), maxdepth=make_fixnum(15), args_template=make_fixnum(385), nargs=142, args=0x7fffffffaf00) at ../../src/bytecode.c:633
#17 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff3d4e97d), nargs=142, arg_vector=0x7fffffffaef8) at ../../src/eval.c:2989
#18 0x00000000008452cb in Ffuncall (nargs=143, args=0x7fffffffaef0) at ../../src/eval.c:2796
#19 0x00000000008444c3 in Fapply (nargs=3, args=0x7fffffffb518) at ../../src/eval.c:2424
#20 0x00000000008456b1 in funcall_subr (subr=0xe0f100 <Sapply>, numargs=3, args=0x7fffffffb518) at ../../src/eval.c:2847
#21 0x0000000000845287 in Ffuncall (nargs=4, args=0x7fffffffb510) at ../../src/eval.c:2794
#22 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff3d213dc), vector=XIL(0x7ffff41be795), maxdepth=make_fixnum(5), args_template=make_fixnum(128), nargs=141, args=0x7fffffffb930) at ../../src/bytecode.c:633
#23 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff41be765), nargs=141, arg_vector=0x7fffffffb930) at ../../src/eval.c:2989
#24 0x0000000000845b42 in apply_lambda (fun=XIL(0x7ffff41be765), args=XIL(0x1631ab3), count=36) at ../../src/eval.c:2926
#25 0x0000000000843d3e in eval_sub (form=XIL(0x1631b23)) at ../../src/eval.c:2318
#26 0x0000000000882b74 in readevalloop_eager_expand_eval (val=XIL(0x1631b23), macroexpand=XIL(0x7ffff31f4a98)) at ../../src/lread.c:1912
#27 0x00000000008833a7 in readevalloop (readcharfun=XIL(0x168c415), infile0=0x0, sourcename=XIL(0x13b67b4), printflag=false, unibyte=XIL(0), readfun=XIL(0), start=XIL(0), end=XIL(0)) at ../../src/lread.c:2094
#28 0x00000000008836d2 in Feval_buffer (buffer=XIL(0x168c415), printflag=XIL(0), filename=XIL(0x13b67b4), unibyte=XIL(0), do_allow_print=XIL(0x30)) at ../../src/lread.c:2167
#29 0x0000000000845883 in funcall_subr (subr=0xe11cc0 <Seval_buffer>, numargs=5, args=0x7fffffffc1d0) at ../../src/eval.c:2879
#30 0x0000000000845287 in Ffuncall (nargs=6, args=0x7fffffffc1c8) at ../../src/eval.c:2794
#31 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff3d5d664), vector=XIL(0x7ffff3d5cfe5), maxdepth=make_fixnum(6), args_template=XIL(0), nargs=0, args=0x0) at ../../src/bytecode.c:633
#32 0x00000000008464a0 in funcall_lambda (fun=XIL(0x7ffff3d5cfb5), nargs=4, arg_vector=0x7ffff3d5cfe5) at ../../src/eval.c:3067
#33 0x00000000008452cb in Ffuncall (nargs=5, args=0x7fffffffc740) at ../../src/eval.c:2796
#34 0x0000000000844c98 in call4 (fn=XIL(0x7ffff2eda6a0), arg1=XIL(0x13b67b4), arg2=XIL(0x13b67b4), arg3=XIL(0x30), arg4=XIL(0x30)) at ../../src/eval.c:2676
#35 0x0000000000880f40 in Fload (file=XIL(0x13354d4), noerror=XIL(0x7ffff2edac10), nomessage=XIL(0x7ffff2edaa68), nosuffix=XIL(0), must_suffix=XIL(0)) at ../../src/lread.c:1376
#36 0x0000000000845883 in funcall_subr (subr=0xe11c40 <Sload>, numargs=3, args=0x7fffffffcaf8) at ../../src/eval.c:2879
#37 0x0000000000845287 in Ffuncall (nargs=4, args=0x7fffffffcaf0) at ../../src/eval.c:2794
#38 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff422855c), vector=XIL(0x7ffff42282c5), maxdepth=make_fixnum(18), args_template=make_fixnum(769), nargs=3, args=0x7fffffffd0d8) at ../../src/bytecode.c:633
#39 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff4228295), nargs=3, arg_vector=0x7fffffffd0c0) at ../../src/eval.c:2989
#40 0x00000000008452cb in Ffuncall (nargs=4, args=0x7fffffffd0b8) at ../../src/eval.c:2796
#41 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff4229434), vector=XIL(0x7ffff42254f5), maxdepth=make_fixnum(14), args_template=make_fixnum(0), nargs=0, args=0x7fffffffdbf8) at ../../src/bytecode.c:633
#42 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff42254c5), nargs=0, arg_vector=0x7fffffffdbf8) at ../../src/eval.c:2989
#43 0x00000000008452cb in Ffuncall (nargs=1, args=0x7fffffffdbf0) at ../../src/eval.c:2796
#44 0x000000000089cd03 in exec_byte_code (bytestr=XIL(0x7ffff422a0fc), vector=XIL(0x7ffff4229605), maxdepth=make_fixnum(12), args_template=make_fixnum(0), nargs=0, args=0x7fffffffe230) at ../../src/bytecode.c:633
#45 0x0000000000845f0b in funcall_lambda (fun=XIL(0x7ffff42295d5), nargs=0, arg_vector=0x7fffffffe230) at ../../src/eval.c:2989
#46 0x0000000000845b42 in apply_lambda (fun=XIL(0x7ffff42295d5), args=XIL(0), count=4) at ../../src/eval.c:2926
#47 0x0000000000843d3e in eval_sub (form=XIL(0x7ffff4385a83)) at ../../src/eval.c:2318
#48 0x00000000008430ce in Feval (form=XIL(0x7ffff4385a83), lexical=XIL(0)) at ../../src/eval.c:2102
#49 0x0000000000764b7f in top_level_2 () at ../../src/keyboard.c:1100
#50 0x000000000084126b in internal_condition_case (bfun=0x764b5c <top_level_2>, handlers=XIL(0x90), hfun=0x76455b <cmd_error>) at ../../src/eval.c:1355
#51 0x0000000000764bc7 in top_level_1 (ignore=XIL(0)) at ../../src/keyboard.c:1108
#52 0x000000000084071f in internal_catch (tag=XIL(0xd200), func=0x764b81 <top_level_1>, arg=XIL(0)) at ../../src/eval.c:1116
#53 0x0000000000764aa6 in command_loop () at ../../src/keyboard.c:1069
#54 0x0000000000764042 in recursive_edit_1 () at ../../src/keyboard.c:714
#55 0x000000000076423a in Frecursive_edit () at ../../src/keyboard.c:786
#56 0x00000000007603bd in main (argc=2, argv=0x7fffffffe7a8) at ../../src/emacs.c:2035

Lisp Backtrace:
"load" (0xffffa3c8)
"custom-load-symbol" (0xffffa958)
"custom-theme-set-variables" (0xffffaef8)
"apply" (0xffffb518)
"custom-set-variables" (0xffffb930)
"eval-buffer" (0xffffc1d0)
"load-with-code-conversion" (0xffffc748)
"load" (0xffffcaf8)
"startup--load-user-init-file" (0xffffd0c0)
"command-line" (0xffffdbf8)
"normal-top-level" (0xffffe230)
(gdb)

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> Date: Tue, 5 May 2020 10:32:31 +0200
> 
> Below find two backtraces with emacs -Q where PRODUCE_GLYPHS here sets
> inhibit_free_realized_faces outside the scope of redisplay_internal
> (that is, while redisplaying_p is nil) such that these will not get
> caught by the unbind form in redisplay_internal.

Thanks.  All of these enter redisplay via echo_area_display.  That
calls redisplay_internal, but only after it displays the mini-window.

However, I had my doubts regarding the accuracy of my mental model.
Namely, the part where I said that inhibit_free_realized_faces should
never be non-zero outside of redisplay.  So I looked at the code and
its history, and it turns out I was wrong: the line that sets the flag
in PRODUCE_GLYPHS was there since Emacs 21, and I see the flag set to
non-zero all the way back to Emacs 22.1 (and probably earlier).

So it sounds like we always were running like that.  Therefore, I must
turn the table and ask you to please describe in more detail,
preferably with Lisp snippets to try, how the fact that this flag is
non-zero gets in the way of something we need to do with faces, both
in this bug and the other one you mentioned.  I'd like to understand
better how this flag interferes in these use cases.

Thanks (and sorry for spreading misinformation: I was somehow
confident that it was myself who added the setting of
inhibit_free_realized_faces in PRODUCE_GLYPHS, but the truth is that
it was Gerd, long ago).

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> Below find two backtraces with emacs -Q where PRODUCE_GLYPHS here sets
 >> inhibit_free_realized_faces outside the scope of redisplay_internal
 >> (that is, while redisplaying_p is nil) such that these will not get
 >> caught by the unbind form in redisplay_internal.
 >
 > Thanks.  All of these enter redisplay via echo_area_display.  That
 > calls redisplay_internal, but only after it displays the mini-window.
 >
 > However, I had my doubts regarding the accuracy of my mental model.
 > Namely, the part where I said that inhibit_free_realized_faces should
 > never be non-zero outside of redisplay.  So I looked at the code and
 > its history, and it turns out I was wrong: the line that sets the flag
 > in PRODUCE_GLYPHS was there since Emacs 21, and I see the flag set to
 > non-zero all the way back to Emacs 22.1 (and probably earlier).

C-x v l told me it was

commit 18155a1d5a93cec23255becab15f26e77cace9e7
Author: Richard M. Stallman <rms@gnu.org>
Date:   Thu Aug 29 14:41:28 2002 +0000

     (PRODUCE_GLYPHS): Set inhibit_free_realized_faces
     when iterator is adding glyphs to a glyph matrix.

but I didn't dig any further.

 > So it sounds like we always were running like that.  Therefore, I must
 > turn the table and ask you to please describe in more detail,
 > preferably with Lisp snippets to try, how the fact that this flag is
 > non-zero gets in the way of something we need to do with faces, both
 > in this bug and the other one you mentioned.  I'd like to understand
 > better how this flag interferes in these use cases.

With Bug#40639 and Bug#41071 the situation is simple: The internal
border changed face but when we enter init_iterator
inhibit_free_realized_faces is true and we don't handle it.  Redisplay
takes care of the face change for normal frames only when it evaluates
their titles and that strange incident is what I used to fix these bugs
on master.

The situation with the present bug is that I rewrote
'window-text-pixel-size' as

DEFUN ("window-text-pixel-size", Fwindow_text_pixel_size, Swindow_text_pixel_size, 0, 6, 0,
        doc: /* Return the size of the text of WINDOW's buffer in pixels.
WINDOW must be a live window and defaults to the selected one.  The
return value is a cons of the maximum pixel-width of any text line and
the maximum pixel-height of all text lines.

The optional argument FROM, if non-nil, specifies the first text
position and defaults to the minimum accessible position of the buffer.
If FROM is t, use the minimum accessible position that starts a
non-empty line.  TO, if non-nil, specifies the last text position and
defaults to the maximum accessible position of the buffer.  If TO is t,
use the maximum accessible position that ends a non-empty line.

The optional argument X-LIMIT, if non-nil, specifies the maximum text
width that can be returned.  X-LIMIT nil or omitted, means to use the
pixel-width of WINDOW's body; use this if you want to know how high
WINDOW should be become in order to fit all of its buffer's text with
the width of WINDOW unaltered.  Use the maximum width WINDOW may assume
if you intend to change WINDOW's width.  In any case, text whose
x-coordinate is beyond X-LIMIT is ignored.  Since calculating the width
of long lines can take some time, it's always a good idea to make this
argument as small as possible; in particular, if the buffer contains
long lines that shall be truncated anyway.

The optional argument Y-LIMIT, if non-nil, specifies the maximum text
height (excluding the height of the mode- or header-line, if any) that
can be returned.  Text lines whose y-coordinate is beyond Y-LIMIT are
ignored.  Since calculating the text height of a large buffer can take
some time, it makes sense to specify this argument if the size of the
buffer is large or unknown.

Optional argument MODE-AND-HEADER-LINE nil or omitted means do not
include the height of the mode- or header-line of WINDOW in the return
value.  If it is either the symbol `mode-line' or `header-line', include
only the height of that line, if present, in the return value.  If t,
include the height of both, if present, in the return value.  */)
   (Lisp_Object window, Lisp_Object from, Lisp_Object to, Lisp_Object x_limit,
    Lisp_Object y_limit, Lisp_Object mode_and_header_line)
{
   struct window *w = decode_live_window (window);
   Lisp_Object buffer = w->contents;
   struct buffer *b;
   struct it it;
   struct buffer *old_b = NULL;
   ptrdiff_t start, end, pos;
   struct text_pos startp;
   void *itdata = NULL;
   int c, max_x = 0, max_y = 0, x = 0, y = 0;

   CHECK_BUFFER (buffer);
   b = XBUFFER (buffer);

   if (b != current_buffer)
     {
       old_b = current_buffer;
       set_buffer_internal (b);
     }

   if (NILP (from))
     start = BEGV;
   else if (EQ (from, Qt))
     {
       start = pos = BEGV;
       while ((pos++ < ZV) && (c = FETCH_CHAR (pos))
	     && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
	start = pos;
       while ((pos-- > BEGV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == '\t'))
	start = pos;
     }
   else
     start = clip_to_bounds (BEGV, fix_position (from), ZV);

   if (NILP (to))
     end = ZV;
   else if (EQ (to, Qt))
     {
       end = pos = ZV;
       while ((pos-- > BEGV) && (c = FETCH_CHAR (pos))
	     && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
	end = pos;
       while ((pos++ < ZV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == '\t'))
	end = pos;
     }
   else
     end = clip_to_bounds (start, fix_position (to), ZV);

   if (!NILP (x_limit) && RANGED_FIXNUMP (0, x_limit, INT_MAX))
     max_x = XFIXNUM (x_limit);

   if (NILP (y_limit))
     max_y = INT_MAX;
   else if (RANGED_FIXNUMP (0, y_limit, INT_MAX))
     max_y = XFIXNUM (y_limit);

   /* Recompute faces if needed.  */
   if (face_change || XFRAME (w->frame)->face_change)
     {
       Lisp_Object window_header_line_format
	= window_parameter (w, Qheader_line_format);
       Lisp_Object window_tab_line_format
	= window_parameter (w, Qtab_line_format);
       Lisp_Object window_mode_line_format
	= window_parameter (w, Qmode_line_format);

       if (((EQ (mode_and_header_line, Qheader_line)
	    || EQ (mode_and_header_line, Qt))
	   && !EQ (window_header_line_format, Qnone)
	   && (!NILP (window_header_line_format)
	       || !NILP (BVAR (current_buffer, header_line_format))))
	  || ((EQ (mode_and_header_line, Qtab_line)
	       || EQ (mode_and_header_line, Qt))
	      && !EQ (window_tab_line_format, Qnone)
	      && (!NILP (window_tab_line_format)
		  || !NILP (BVAR (current_buffer, tab_line_format))))
	  || ((EQ (mode_and_header_line, Qmode_line)
	       || EQ (mode_and_header_line, Qt))
	      && !EQ (window_mode_line_format, Qnone)
	      && (!NILP (window_mode_line_format)
		  || !NILP (BVAR (current_buffer, mode_line_format)))))
	{
	  if (face_change)
	    {
	      face_change = false;
	      XFRAME (w->frame)->face_change = 0;
	      free_all_realized_faces (Qnil);
	    }
	  else
	    {
	      XFRAME (w->frame)->face_change = 0;
	      free_all_realized_faces (w->frame);
	    }
	}
     }

   itdata = bidi_shelve_cache ();
   SET_TEXT_POS (startp, start, CHAR_TO_BYTE (start));
   start_display (&it, w, startp);
   /* It makes no sense to measure dimensions of region of text that
      crosses the point where bidi reordering changes scan direction.
      By using unidirectional movement here we at least support the use
      case of measuring regions of text that have a uniformly R2L
      directionality, and regions that begin and end in text of the
      same directionality.  */
   it.bidi_p = false;

   int move_op = MOVE_TO_POS | MOVE_TO_Y;
   int to_x = -1;
   if (!NILP (x_limit))
     {
       it.last_visible_x = max_x;
       /* Actually, we never want move_it_to stop at to_x.  But to make
	 sure that move_it_in_display_line_to always moves far enough,
	 we set to_x to INT_MAX and specify MOVE_TO_X.  */
       move_op |= MOVE_TO_X;
       to_x = INT_MAX;
     }

   void *it2data = NULL;
   struct it it2;
   SAVE_IT (it2, it, it2data);

   x = move_it_to (&it, end, to_x, max_y, -1, move_op);

   /* We could have a display property at END, in which case asking
      move_it_to to stop at END will overshoot and stop at position
      after END.  So we try again, stopping before END, and account for
      the width of the last buffer position manually.  */
   if (IT_CHARPOS (it) > end)
     {
       end--;
       RESTORE_IT (&it, &it2, it2data);
       x = move_it_to (&it, end, to_x, max_y, -1, move_op);
       /* Add the width of the thing at TO, but only if we didn't
	 overshoot it; if we did, it is already accounted for.  Also,
	 account for the height of the thing at TO.  */
       if (IT_CHARPOS (it) == end)
	{
	  x += it.pixel_width;
	  it.max_ascent = max (it.max_ascent, it.ascent);
	  it.max_descent = max (it.max_descent, it.descent);
	}
     }
   if (!NILP (x_limit))
     {
       /* Don't return more than X-LIMIT.  */
       if (x > max_x)
         x = max_x;
     }

   /* Subtract height of header-line which was counted automatically by
      start_display.  */
   y = it.current_y + it.max_ascent + it.max_descent
     - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w);
   /* Don't return more than Y-LIMIT.  */
   if (y > max_y)
     y = max_y;

   if (EQ (mode_and_header_line, Qtab_line)
       || EQ (mode_and_header_line, Qt))
     {
       Lisp_Object window_tab_line_format
	= window_parameter (w, Qtab_line_format);

       if (!EQ (window_tab_line_format, Qnone)
	  && (!NILP (window_tab_line_format)
	      || !NILP (BVAR (current_buffer, tab_line_format))))
	/* Re-add height of tab-line as requested.  */
	y = y + display_mode_line (w, TAB_LINE_FACE_ID,
				   NILP (window_tab_line_format)
				   ? BVAR (current_buffer, tab_line_format)
				   : window_tab_line_format);
     }

   if (EQ (mode_and_header_line, Qheader_line)
       || EQ (mode_and_header_line, Qt))
     {
       Lisp_Object window_header_line_format
	= window_parameter (w, Qheader_line_format);

       if (!EQ (window_header_line_format, Qnone)
	  && (!NILP (window_header_line_format)
	      || !NILP (BVAR (current_buffer, header_line_format))))
	/* Re-add height of header-line as requested.  */
	y = y + display_mode_line (w, HEADER_LINE_FACE_ID,
				   NILP (window_header_line_format)
				   ? BVAR (current_buffer, header_line_format)
				   : window_header_line_format);
     }

   if (EQ (mode_and_header_line, Qmode_line)
       || EQ (mode_and_header_line, Qt))
     {
       Lisp_Object window_mode_line_format
	= window_parameter (w, Qmode_line_format);

       if (!EQ (window_mode_line_format, Qnone)
	  && (!NILP (window_mode_line_format)
	      || !NILP (BVAR (current_buffer, mode_line_format))))
	/* Add height of mode-line as requested.  */
	y = y + display_mode_line (w, CURRENT_MODE_LINE_FACE_ID (w),
				   NILP (window_mode_line_format)
				   ? BVAR (current_buffer, mode_line_format)
				   : window_mode_line_format);
     }

   bidi_unshelve_cache (itdata, false);

   if (old_b)
     set_buffer_internal (old_b);

   return Fcons (make_fixnum (x), make_fixnum (y));
}

This takes any face change into consideration and works.  But it
obviously disregards inhibit_free_realized_faces and so I'm not sure
whether it's TRT (rather, I'm quite confident that it is not TRT).  At
the very least I probably should set windows_or_buffers_changed.  There
is this comment in redisplay_internal

   /* If face_change, init_iterator will free all realized faces, which
      includes the faces referenced from current matrices.  So, we
      can't reuse current matrices in this case.  */
   if (face_change)
     windows_or_buffers_changed = 47;

which hints at that (without considering that f->face_change is not
handled there).

 > Thanks (and sorry for spreading misinformation: I was somehow
 > confident that it was myself who added the setting of
 > inhibit_free_realized_faces in PRODUCE_GLYPHS, but the truth is that
 > it was Gerd, long ago).

Nevertheless, the fact that inhibit_free_realized_faces is true when
entering the iterator after a face change is IMO a bug.  We apparently
always run the iterator with the old faces first.  Only after we have
(incidentally?) detected some change that forces us to "retry", we have
redisplay set inhibit_free_realized_faces to false itself and the face
change will get applied in the next iteration.  If so, the outcome of
the previous iterations gets lost if a face changed there.  Does such a
strategy give us any gains?

Again, the question I tried to ask earlier was: Does a current matrix in
between two redisplays rely on the old realized faces?  If so, the
answer is that inhibit_free_realized_faces should be always true but for
the small window within retrying in redisplay_internal.  And maybe
somewhere at the beginning of redisplay when a face change occurred for
the window's frame and we clear all matrices therefore and avoid any
redisplay optimizations.

In either case, it strikes me as a strange idea that redisplay_internal
saves and restores the value of a variable which is apparently always
true when it is entered (I suppose redisplay_internal is not re-entrant
given

   /* I don't think this happens but let's be paranoid.  */
   if (redisplaying_p)
     return;

) but maybe there are exceptions I don't know.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 5 May 2020 18:57:06 +0200
> 
> With Bug#40639 and Bug#41071 the situation is simple: The internal
> border changed face but when we enter init_iterator
> inhibit_free_realized_faces is true and we don't handle it.

Why do we need to enter init_iterator when the border face changes?
And why do we need init_iterator to handle the face change?

Thanks for the rest, I will look at that soon.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Why do we need to enter init_iterator when the border face changes?
 > And why do we need init_iterator to handle the face change?

If our only tool is a hammer ... Or should we call 'clear-face-cache' or
'redraw-display' whenever changing the border face?

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

On May 6, 2020 9:50:06 AM GMT+03:00, martin rudalics <rudalics@gmx.at> wrote:
>  > Why do we need to enter init_iterator when the border face changes?
>  > And why do we need init_iterator to handle the face change?
> 
> If our only tool is a hammer ... Or should we call 'clear-face-cache'
> or
> 'redraw-display' whenever changing the border face?
> 
> martin

I simply don't yet understand why updating the frame decorations that have nothing to do with text layout needs init_iterator to run.  What am I missing?  Redisplay doesn't in general redraw the whole frame, it only redraws windows.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I simply don't yet understand why updating the frame decorations that
 > have nothing to do with text layout needs init_iterator to run.  What
 > am I missing?  Redisplay doesn't in general redraw the whole frame, it
 > only redraws windows.

I don't understand either.  Fact is, that init_iterator is the only
agent within redisplay that frees realized faces and causes the new
border face to be realized.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Date: Wed, 06 May 2020 12:27:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> 
> I simply don't yet understand why updating the frame decorations that have nothing to do with text layout needs init_iterator to run.  What am I missing?  Redisplay doesn't in general redraw the whole frame, it only redraws windows.

Answering my own question: because the clear_under_internal_border
method, which redraws the internal border, is called from
redisplay_internal.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> I simply don't yet understand why updating the frame decorations that have nothing to do with text layout needs init_iterator to run.  What am I missing?  Redisplay doesn't in general redraw the whole frame, it only redraws windows.
 >
 > Answering my own question: because the clear_under_internal_border
 > method, which redraws the internal border, is called from
 > redisplay_internal.

That's only a secondary issue, I think.  clear_under_internal_border is
called from so many places.  The real reason is, as I tried to explain
before, to update the basic faces, however that is accomplished.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 7 May 2020 10:34:40 +0200
> 
>  >> I simply don't yet understand why updating the frame decorations that have nothing to do with text layout needs init_iterator to run.  What am I missing?  Redisplay doesn't in general redraw the whole frame, it only redraws windows.
>  >
>  > Answering my own question: because the clear_under_internal_border
>  > method, which redraws the internal border, is called from
>  > redisplay_internal.
> 
> That's only a secondary issue, I think.  clear_under_internal_border is
> called from so many places.  The real reason is, as I tried to explain
> before, to update the basic faces, however that is accomplished.

What puzzled me was how init_iterator was involved: without its being
called at least once, the call to clear_under_internal_border will not
use the updated faces.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 5 May 2020 18:57:06 +0200
> 
>  > So it sounds like we always were running like that.  Therefore, I must
>  > turn the table and ask you to please describe in more detail,
>  > preferably with Lisp snippets to try, how the fact that this flag is
>  > non-zero gets in the way of something we need to do with faces, both
>  > in this bug and the other one you mentioned.  I'd like to understand
>  > better how this flag interferes in these use cases.
> 
> With Bug#40639 and Bug#41071 the situation is simple: The internal
> border changed face but when we enter init_iterator
> inhibit_free_realized_faces is true and we don't handle it.

Can you show the backtrace from this call to init_iterator?  If it is
called directly or indirectly from redisplay_internal, the
inhibit_free_realized_faces flag should be reset to zero.  So I'm
guessing it is called in some other way.  That other way is not
necessarily the situation which should solve the delayed face
refreshing, since the corresponding parts of the frame will not be
redrawn until we enter redisplay_internal anyway, right?  Or am I
missing something.

> 	  if (face_change)
> 	    {
> 	      face_change = false;
> 	      XFRAME (w->frame)->face_change = 0;
> 	      free_all_realized_faces (Qnil);
> 	    }
> 	  else
> 	    {
> 	      XFRAME (w->frame)->face_change = 0;
> 	      free_all_realized_faces (w->frame);
> 	    }

You don't really need to free the faces everywhere here, only for the
window W's frame, right?  I think we shouldn't throw away face caches
of other frames in this situation, it's too radical.

> This takes any face change into consideration and works.  But it
> obviously disregards inhibit_free_realized_faces and so I'm not sure
> whether it's TRT (rather, I'm quite confident that it is not TRT).

I think the only situation where it can hurt is when this code is
called while redisplay_internal runs.  IOW, we need to test the value
of redisplaying_p.

> At the very least I probably should set windows_or_buffers_changed.
> There is this comment in redisplay_internal
> 
>    /* If face_change, init_iterator will free all realized faces, which
>       includes the faces referenced from current matrices.  So, we
>       can't reuse current matrices in this case.  */
>    if (face_change)
>      windows_or_buffers_changed = 47;
> 
> which hints at that (without considering that f->face_change is not
> handled there).

The call to free_all_realized_faces invalidates the current matrices
of all the windows on the frame, so the stale face IDs will not be
used after that on that frame, and therefore nothing else needs to be
done in that case.  If _all_ the face caches are discarded on all
frames, we need windows_or_buffers_changed to force redisplay_internal
consider more than just the selected frame.  So if you modify your
code to free faces only on the frame of the window for which you
compute the mode-line height, the problem with
windows_or_buffers_changed will disappear.

> Nevertheless, the fact that inhibit_free_realized_faces is true when
> entering the iterator after a face change is IMO a bug.  We apparently
> always run the iterator with the old faces first.  Only after we have
> (incidentally?) detected some change that forces us to "retry", we have
> redisplay set inhibit_free_realized_faces to false itself and the face
> change will get applied in the next iteration.  If so, the outcome of
> the previous iterations gets lost if a face changed there.  Does such a
> strategy give us any gains?

I don't think I follow.  redisplay_internal resets the
inhibit_free_realized_faces flag to zero near its beginning.  It is
true that we also reset it when we retry, but the first try doesn't
bypass this resetting.  Am I missing something?

> Again, the question I tried to ask earlier was: Does a current matrix in
> between two redisplays rely on the old realized faces?

Of course it does.  The glyph matrices don't hold any face
information, they only hold the ID of each face, and the ID is just
the index of the face's cache slot.  If the face cache is thrown away,
we will not be able to use the current matrix.

The current matrix is used for the following purposes:

  . outside of the redisplay cycle, for redisplaying portions of
    windows when we get expose events -- in this case we simply write
    to the glass the relevant portions of the matrix, without
    reproducing it

  . during a redisplay cycle, for decisions about redisplay
    optimizations

  . at the end of a redisplay cycle, for comparison with the desired
    matrix, which is needed for figuring out what to write to the
    glass

For the first purpose, we have the following defenses:

 . expose_frame:

  if (FRAME_FACE_CACHE (f) == NULL
      || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL)
    {
      redisplay_trace ("expose_frame no faces\n");
      return;
    }

 . expose_window (called by expose_frame):

  if (w->current_matrix == NULL)
    return false;

So this case is covered, i.e. you can call free_all_realized_faces,
and the next expose event will be handled correctly.

The other two are the reason why we set the
inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
during redisplay, until update_frame finished its job.  Otherwise we
will sooner or later crash.

So I think each function that might need to notice face changes as
soon as they happened, should be able to reset
inhibit_free_realized_faces, provided that (a) it makes sure
redisplaying_p is zero, and (b) it only does that if necessary and for
a single frame.  The latter part is because clearing the face cache
will cause all the move_it_* functions to work harder, because they
will have to recompute all the faces.

> If so, the answer is that inhibit_free_realized_faces should be
> always true but for the small window within retrying in
> redisplay_internal.

I don't think I agree, but maybe I'm missing something.

> In either case, it strikes me as a strange idea that redisplay_internal
> saves and restores the value of a variable which is apparently always
> true when it is entered (I suppose redisplay_internal is not re-entrant
> given
> 
>    /* I don't think this happens but let's be paranoid.  */
>    if (redisplaying_p)
>      return;

redisplay_internal is non-reentrant, but I see no harm in restoring
the value on exit.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> With Bug#40639 and Bug#41071 the situation is simple: The internal
 >> border changed face but when we enter init_iterator
 >> inhibit_free_realized_faces is true and we don't handle it.
 >
 > Can you show the backtrace from this call to init_iterator?

I run emacs -Q and evaluate there the following form

(progn
   (set-face-background 'internal-border "red")
   (select-window
    (display-buffer-in-child-frame
     (get-buffer-create "*scratch*")
     '((child-frame-parameters
        .
        ((left . 100)
	(top  . 100)
	(height . 10)
	(width . 20)
	(minibuffer . nil)
	(internal-border-width . 50)))))))

Now _if I remove the following two lines_

     if ((IT)->glyph_row != NULL)                        \
       inhibit_free_realized_faces = true;		\

from PRODUCE_GLYPHS, evaluating the form above does pick up the face
triggered by

XFRAME (w->frame)->face_change

true as

#0  0x0000000000463cbf in init_iterator (it=0x7fffffff8c60, w=0x193e060, charpos=1, bytepos=1, row=0x1947f90, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2975
#1  0x00000000004652e8 in start_display (it=0x7fffffff8c60, w=0x193e060, pos=...) at ../../src/xdisp.c:3298
#2  0x0000000000497468 in try_window (window=XIL(0x193e065), pos=..., flags=1) at ../../src/xdisp.c:19120
#3  0x00000000004941e9 in redisplay_window (window=XIL(0x193e065), just_this_one_p=false) at ../../src/xdisp.c:18544
#4  0x000000000048be75 in redisplay_window_0 (window=XIL(0x193e065)) at ../../src/xdisp.c:16258
#5  0x00000000007af751 in internal_condition_case_1 (bfun=0x48be33 <redisplay_window_0>, arg=XIL(0x193e065), handlers=XIL(0x7ffff3e98ee3), hfun=0x48bdfb <redisplay_window_error>) at ../../src/eval.c:1379
#6  0x000000000048bdcd in redisplay_windows (window=XIL(0x193e065)) at ../../src/xdisp.c:16238
#7  0x000000000048a7fb in redisplay_internal () at ../../src/xdisp.c:15706
#8  0x00000000004883f9 in redisplay () at ../../src/xdisp.c:14933
#9  0x000000000064edbd in read_char (commandflag=1, map=XIL(0x17e4f93), prev_event=XIL(0), used_mouse_menu=0x7fffffffe0ff, end_time=0x0) at ../../src/keyboard.c:2493
#10 0x0000000000661ce5 in read_key_sequence (keybuf=0x7fffffffe290, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9553
#11 0x000000000064b283 in command_loop_1 () at ../../src/keyboard.c:1350
#12 0x00000000007af676 in internal_condition_case (bfun=0x64ae07 <command_loop_1>, handlers=XIL(0x90), hfun=0x64a416 <cmd_error>) at ../../src/eval.c:1355
#13 0x000000000064a9ec in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1091
#14 0x00000000007aeb2a in internal_catch (tag=XIL(0xd110), func=0x64a9bf <command_loop_2>, arg=XIL(0)) at ../../src/eval.c:1116
#15 0x000000000064a98a in command_loop () at ../../src/keyboard.c:1070
#16 0x0000000000649efd in recursive_edit_1 () at ../../src/keyboard.c:714
#17 0x000000000064a0f5 in Frecursive_edit () at ../../src/keyboard.c:786
#18 0x00000000006404fe in main (argc=4, argv=0x7fffffffe788) at ../../src/emacs.c:2054

Lisp Backtrace:
"redisplay_internal (C function)" (0x0)
(gdb) c

With PRODUCE_GLYPHS unaltered, that breakpoint does _not_ trigger.  Only
if I set frame titles for child frames (as I do on master now) I can get
a backtrace like the below

#0  0x00000000004821dc in init_iterator (it=0x7fffffffb4a0, w=0x1a4b800, charpos=-1, bytepos=-1, row=0x0, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2988
#1  0x00000000004a575b in gui_consider_frame_title (frame=XIL(0x1a5e8f5)) at ../../src/xdisp.c:12447
#2  0x00000000004a5c07 in prepare_menu_bars () at ../../src/xdisp.c:12544
#3  0x00000000004aaeaf in redisplay_internal () at ../../src/xdisp.c:15392
#4  0x00000000004a9aeb in redisplay () at ../../src/xdisp.c:14977
#5  0x0000000000768eb6 in read_char (commandflag=1, map=XIL(0x1c5c3d3), prev_event=XIL(0), used_mouse_menu=0x7fffffffe10f, end_time=0x0) at ../../src/keyboard.c:2493
#6  0x00000000007800e3 in read_key_sequence (keybuf=0x7fffffffe2a0, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9547
#7  0x000000000076549a in command_loop_1 () at ../../src/keyboard.c:1350
#8  0x000000000084133d in internal_condition_case (bfun=0x76501e <command_loop_1>, handlers=XIL(0x90), hfun=0x76462d <cmd_error>) at ../../src/eval.c:1355
#9  0x0000000000764c03 in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1091
#10 0x00000000008407f1 in internal_catch (tag=XIL(0xd200), func=0x764bd6 <command_loop_2>, arg=XIL(0)) at ../../src/eval.c:1116
#11 0x0000000000764ba1 in command_loop () at ../../src/keyboard.c:1070
#12 0x0000000000764114 in recursive_edit_1 () at ../../src/keyboard.c:714
#13 0x000000000076430c in Frecursive_edit () at ../../src/keyboard.c:786
#14 0x000000000076048f in main (argc=3, argv=0x7fffffffe798) at ../../src/emacs.c:2035

which also picks up the change of the internal border's background.

With an unaltered release I can't get init_iterator to notice the face
change without, in some clumsy way, switching to that child frame with
the mouse or something the like.

Disclaimer: In all those runs I do not know whether the

   (set-face-background 'internal-border "red")

set the face_change flag or something else did.

 >> 	  if (face_change)
 >> 	    {
 >> 	      face_change = false;
 >> 	      XFRAME (w->frame)->face_change = 0;
 >> 	      free_all_realized_faces (Qnil);
 >> 	    }
 >> 	  else
 >> 	    {
 >> 	      XFRAME (w->frame)->face_change = 0;
 >> 	      free_all_realized_faces (w->frame);
 >> 	    }
 >
 > You don't really need to free the faces everywhere here, only for the
 > window W's frame, right?

Right.

 > I think we shouldn't throw away face caches
 > of other frames in this situation, it's too radical.

Agreed.

 > I think the only situation where it can hurt is when this code is
 > called while redisplay_internal runs.  IOW, we need to test the value
 > of redisplaying_p.

OK.

 >> At the very least I probably should set windows_or_buffers_changed.
 >> There is this comment in redisplay_internal
 >>
 >>     /* If face_change, init_iterator will free all realized faces, which
 >>        includes the faces referenced from current matrices.  So, we
 >>        can't reuse current matrices in this case.  */
 >>     if (face_change)
 >>       windows_or_buffers_changed = 47;
 >>
 >> which hints at that (without considering that f->face_change is not
 >> handled there).
 >
 > The call to free_all_realized_faces invalidates the current matrices
 > of all the windows on the frame, so the stale face IDs will not be
 > used after that on that frame, and therefore nothing else needs to be
 > done in that case.  If _all_ the face caches are discarded on all
 > frames, we need windows_or_buffers_changed to force redisplay_internal
 > consider more than just the selected frame.  So if you modify your
 > code to free faces only on the frame of the window for which you
 > compute the mode-line height, the problem with
 > windows_or_buffers_changed will disappear.

OK.

 >> Nevertheless, the fact that inhibit_free_realized_faces is true when
 >> entering the iterator after a face change is IMO a bug.  We apparently
 >> always run the iterator with the old faces first.  Only after we have
 >> (incidentally?) detected some change that forces us to "retry", we have
 >> redisplay set inhibit_free_realized_faces to false itself and the face
 >> change will get applied in the next iteration.  If so, the outcome of
 >> the previous iterations gets lost if a face changed there.  Does such a
 >> strategy give us any gains?
 >
 > I don't think I follow.  redisplay_internal resets the
 > inhibit_free_realized_faces flag to zero near its beginning.  It is
 > true that we also reset it when we retry, but the first try doesn't
 > bypass this resetting.  Am I missing something?

It might have been a wrong conclusion on my side.  Maybe the problem is
caused by the fact that face_change is not set by 'set-face-background'
and the face change that triggered the backtraces above came from
somewhere else.

 >> Again, the question I tried to ask earlier was: Does a current matrix in
 >> between two redisplays rely on the old realized faces?
 >
 > Of course it does.  The glyph matrices don't hold any face
 > information, they only hold the ID of each face, and the ID is just
 > the index of the face's cache slot.  If the face cache is thrown away,
 > we will not be able to use the current matrix.

So if I set `inhibit-free-realized-faces' to nil at some arbitrary time
things may go wrong.

 > The current matrix is used for the following purposes:
 >
 >    . outside of the redisplay cycle, for redisplaying portions of
 >      windows when we get expose events -- in this case we simply write
 >      to the glass the relevant portions of the matrix, without
 >      reproducing it
 >
 >    . during a redisplay cycle, for decisions about redisplay
 >      optimizations
 >
 >    . at the end of a redisplay cycle, for comparison with the desired
 >      matrix, which is needed for figuring out what to write to the
 >      glass
 >
 > For the first purpose, we have the following defenses:
 >
 >   . expose_frame:
 >
 >    if (FRAME_FACE_CACHE (f) == NULL
 >        || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL)
 >      {
 >        redisplay_trace ("expose_frame no faces\n");
 >        return;
 >      }
 >
 >   . expose_window (called by expose_frame):
 >
 >    if (w->current_matrix == NULL)
 >      return false;
 >
 > So this case is covered, i.e. you can call

... from Elisp, at any time, I presume ...

 > free_all_realized_faces,
 > and the next expose event will be handled correctly.

 > The other two are the reason why we set the
 > inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
 > during redisplay, until update_frame finished its job.  Otherwise we
 > will sooner or later crash.

I'm running without this for a couple of days now and it did not crash
so far.  Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
inhibit_free_realized_faces iff redisplaying_p is true?

 > So I think each function that might need to notice face changes as
 > soon as they happened, should be able to reset
 > inhibit_free_realized_faces, provided that (a) it makes sure
 > redisplaying_p is zero, and (b) it only does that if necessary and for
 > a single frame.  The latter part is because clearing the face cache
 > will cause all the move_it_* functions to work harder, because they
 > will have to recompute all the faces.

So it's not so entirely trivial to do that in Elisp.

 >> If so, the answer is that inhibit_free_realized_faces should be
 >> always true but for the small window within retrying in
 >> redisplay_internal.
 >
 > I don't think I agree, but maybe I'm missing something.

OK.  But IIUC so far we do not allow inhibit_free_realized_faces nil
outside of redisplay_internal.  Unless someone sets
'inhibit-free-realized-faces' which is, in general, to recommended.
Right?

 >> In either case, it strikes me as a strange idea that redisplay_internal
 >> saves and restores the value of a variable which is apparently always
 >> true when it is entered (I suppose redisplay_internal is not re-entrant
 >> given
 >>
 >>     /* I don't think this happens but let's be paranoid.  */
 >>     if (redisplaying_p)
 >>       return;
 >
 > redisplay_internal is non-reentrant, but I see no harm in restoring
 > the value on exit.

If someone sets it to nil now, observing the precautions you gave above,
things change radically.  We may restore the value to nil, which was
hardly the case before.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 7 May 2020 10:34:53 +0200
> 
>  >> With Bug#40639 and Bug#41071 the situation is simple: The internal
>  >> border changed face but when we enter init_iterator
>  >> inhibit_free_realized_faces is true and we don't handle it.
>  >
>  > Can you show the backtrace from this call to init_iterator?
> 
> I run emacs -Q and evaluate there the following form
> 
> (progn
>    (set-face-background 'internal-border "red")
>    (select-window
>     (display-buffer-in-child-frame
>      (get-buffer-create "*scratch*")
>      '((child-frame-parameters
>         .
>         ((left . 100)
> 	(top  . 100)
> 	(height . 10)
> 	(width . 20)
> 	(minibuffer . nil)
> 	(internal-border-width . 50)))))))
> 
> Now _if I remove the following two lines_
> 
>      if ((IT)->glyph_row != NULL)                        \
>        inhibit_free_realized_faces = true;		\
> 
> from PRODUCE_GLYPHS, evaluating the form above does pick up the face
> triggered by
> 
> XFRAME (w->frame)->face_change
> 
> true as
> 
> #0  0x0000000000463cbf in init_iterator (it=0x7fffffff8c60, w=0x193e060, charpos=1, bytepos=1, row=0x1947f90, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2975
> #1  0x00000000004652e8 in start_display (it=0x7fffffff8c60, w=0x193e060, pos=...) at ../../src/xdisp.c:3298
> #2  0x0000000000497468 in try_window (window=XIL(0x193e065), pos=..., flags=1) at ../../src/xdisp.c:19120
> #3  0x00000000004941e9 in redisplay_window (window=XIL(0x193e065), just_this_one_p=false) at ../../src/xdisp.c:18544
> #4  0x000000000048be75 in redisplay_window_0 (window=XIL(0x193e065)) at ../../src/xdisp.c:16258
> #5  0x00000000007af751 in internal_condition_case_1 (bfun=0x48be33 <redisplay_window_0>, arg=XIL(0x193e065), handlers=XIL(0x7ffff3e98ee3), hfun=0x48bdfb <redisplay_window_error>) at ../../src/eval.c:1379
> #6  0x000000000048bdcd in redisplay_windows (window=XIL(0x193e065)) at ../../src/xdisp.c:16238
> #7  0x000000000048a7fb in redisplay_internal () at ../../src/xdisp.c:15706
> [...]
> With PRODUCE_GLYPHS unaltered, that breakpoint does _not_ trigger.  Only
> if I set frame titles for child frames (as I do on master now) I can get
> a backtrace like the below
> 
> #0  0x00000000004821dc in init_iterator (it=0x7fffffffb4a0, w=0x1a4b800, charpos=-1, bytepos=-1, row=0x0, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2988
> #1  0x00000000004a575b in gui_consider_frame_title (frame=XIL(0x1a5e8f5)) at ../../src/xdisp.c:12447
> #2  0x00000000004a5c07 in prepare_menu_bars () at ../../src/xdisp.c:12544
> #3  0x00000000004aaeaf in redisplay_internal () at ../../src/xdisp.c:15392

The second call happens before the first one, so it looks more correct
to me.  Why did you think you didn't need to set the frame title for
child frames?

> Disclaimer: In all those runs I do not know whether the
> 
>    (set-face-background 'internal-border "red")
> 
> set the face_change flag or something else did.

It doesn't.  It sets the face_change flag of each frame instead.  See
internal-set-lisp-face-attribute.

>  >> Nevertheless, the fact that inhibit_free_realized_faces is true when
>  >> entering the iterator after a face change is IMO a bug.  We apparently
>  >> always run the iterator with the old faces first.  Only after we have
>  >> (incidentally?) detected some change that forces us to "retry", we have
>  >> redisplay set inhibit_free_realized_faces to false itself and the face
>  >> change will get applied in the next iteration.  If so, the outcome of
>  >> the previous iterations gets lost if a face changed there.  Does such a
>  >> strategy give us any gains?
>  >
>  > I don't think I follow.  redisplay_internal resets the
>  > inhibit_free_realized_faces flag to zero near its beginning.  It is
>  > true that we also reset it when we retry, but the first try doesn't
>  > bypass this resetting.  Am I missing something?
> 
> It might have been a wrong conclusion on my side.  Maybe the problem is
> caused by the fact that face_change is not set by 'set-face-background'
> and the face change that triggered the backtraces above came from
> somewhere else.

See above.  It could be that we somehow fail to redisplay the child
frame thoroughly enough, though.

>  >> Again, the question I tried to ask earlier was: Does a current matrix in
>  >> between two redisplays rely on the old realized faces?
>  >
>  > Of course it does.  The glyph matrices don't hold any face
>  > information, they only hold the ID of each face, and the ID is just
>  > the index of the face's cache slot.  If the face cache is thrown away,
>  > we will not be able to use the current matrix.
> 
> So if I set `inhibit-free-realized-faces' to nil at some arbitrary time
> things may go wrong.

Only if you do that in code that can run while redisplay_internal is
doing its job.

>  >   . expose_frame:
>  >
>  >    if (FRAME_FACE_CACHE (f) == NULL
>  >        || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL)
>  >      {
>  >        redisplay_trace ("expose_frame no faces\n");
>  >        return;
>  >      }
>  >
>  >   . expose_window (called by expose_frame):
>  >
>  >    if (w->current_matrix == NULL)
>  >      return false;
>  >
>  > So this case is covered, i.e. you can call
> 
> ... from Elisp, at any time, I presume ...

Yes, but not limited to that.

>  > The other two are the reason why we set the
>  > inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
>  > during redisplay, until update_frame finished its job.  Otherwise we
>  > will sooner or later crash.
> 
> I'm running without this for a couple of days now and it did not crash
> so far.

Try setting things up such that faces are modified by some Lisp that
runs during redisplay, like some :eval form of the mode line or one of
the hooks called by the display code.  That's how crashes with face =
NULL invariably start.

> Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
> inhibit_free_realized_faces iff redisplaying_p is true?

No, because some functions we call from Lisp actually write into the
current matrix.  For example, tool-bar-height and tab-bar-height.

>  > So I think each function that might need to notice face changes as
>  > soon as they happened, should be able to reset
>  > inhibit_free_realized_faces, provided that (a) it makes sure
>  > redisplaying_p is zero, and (b) it only does that if necessary and for
>  > a single frame.  The latter part is because clearing the face cache
>  > will cause all the move_it_* functions to work harder, because they
>  > will have to recompute all the faces.
> 
> So it's not so entirely trivial to do that in Elisp.

Why do we need to do this in Lisp?

>  >> If so, the answer is that inhibit_free_realized_faces should be
>  >> always true but for the small window within retrying in
>  >> redisplay_internal.
>  >
>  > I don't think I agree, but maybe I'm missing something.
> 
> OK.  But IIUC so far we do not allow inhibit_free_realized_faces nil
> outside of redisplay_internal.  Unless someone sets
> 'inhibit-free-realized-faces' which is, in general, to recommended.
> Right?

Yes, but I see no reason not to reset it in other places, provided
that we do it carefully and only when absolutely needed.

>  >> In either case, it strikes me as a strange idea that redisplay_internal
>  >> saves and restores the value of a variable which is apparently always
>  >> true when it is entered (I suppose redisplay_internal is not re-entrant
>  >> given
>  >>
>  >>     /* I don't think this happens but let's be paranoid.  */
>  >>     if (redisplaying_p)
>  >>       return;
>  >
>  > redisplay_internal is non-reentrant, but I see no harm in restoring
>  > the value on exit.
> 
> If someone sets it to nil now, observing the precautions you gave above,
> things change radically.  We may restore the value to nil, which was
> hardly the case before.

That'd be shooting ourselves in the foot during the next redisplay,
but it isn't a catastrophe, AFAIU.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > The second call happens before the first one, so it looks more correct
 > to me.  Why did you think you didn't need to set the frame title for
 > child frames?

Because on all systems excluding Windows child frames do not show their
titles.  Calculating something that cannot be displayed doesn't strike
me as a good idea.  And obviously native tooltip frames which suffer the
same problem using

(progn
   (setq x-gtk-use-system-tooltips nil)
   (set-face-background 'internal-border "red"))

would still not display borders correctly.  Or should we set their
titles too?

 >> Disclaimer: In all those runs I do not know whether the
 >>
 >>     (set-face-background 'internal-border "red")
 >>
 >> set the face_change flag or something else did.
 >
 > It doesn't.  It sets the face_change flag of each frame instead.  See
 > internal-set-lisp-face-attribute.

This way of setting things confuses me.  What is then that global
face_change bool needed for?

 >> It might have been a wrong conclusion on my side.  Maybe the problem is
 >> caused by the fact that face_change is not set by 'set-face-background'
 >> and the face change that triggered the backtraces above came from
 >> somewhere else.
 >
 > See above.  It could be that we somehow fail to redisplay the child
 > frame thoroughly enough, though.

I have not tried yet but I'm convinced that we would fail for a normal
frame as well if we didn't set its frame title.  This reliance of one
(internal border color) upon the conceptually unrelated other (setting
the frame title) looks fishy to me and IIRC causes redisplay_internal to
initially run with old character sizes until the actual ones get
realized when setting the frame title.

 >> So if I set `inhibit-free-realized-faces' to nil at some arbitrary time
 >> things may go wrong.
 >
 > Only if you do that in code that can run while redisplay_internal is
 > doing its job.

That's what I meant, yes.

 >>   > The other two are the reason why we set the
 >>   > inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
 >>   > during redisplay, until update_frame finished its job.  Otherwise we
 >>   > will sooner or later crash.
 >>
 >> I'm running without this for a couple of days now and it did not crash
 >> so far.
 >
 > Try setting things up such that faces are modified by some Lisp that
 > runs during redisplay, like some :eval form of the mode line or one of
 > the hooks called by the display code.  That's how crashes with face =
 > NULL invariably start.

Not really needed.  You convinced me here.

 >> Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
 >> inhibit_free_realized_faces iff redisplaying_p is true?
 >
 > No, because some functions we call from Lisp actually write into the
 > current matrix.  For example, tool-bar-height and tab-bar-height.

Then there's more to it than the above "the flag must be set during
redisplay, until update_frame finished its job"?  I'm still confused but
that's probably my poor mind.  Maybe re-reading your text after a day or
so will help.

 >> So it's not so entirely trivial to do that in Elisp.
 >
 > Why do we need to do this in Lisp?

Didn't you propose to calculate the mode-line height from
'fit-window-to-buffer'?

 >> OK.  But IIUC so far we do not allow inhibit_free_realized_faces nil
 >> outside of redisplay_internal.  Unless someone sets
 >> 'inhibit-free-realized-faces' which is, in general, to recommended.
 >> Right?
 >
 > Yes, but I see no reason not to reset it in other places, provided
 > that we do it carefully and only when absolutely needed.

We should say that in the doc-string of 'inhibit-free-realized-faces',
maybe.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 11 May 2020 10:30:45 +0200
> 
>  > The second call happens before the first one, so it looks more correct
>  > to me.  Why did you think you didn't need to set the frame title for
>  > child frames?
> 
> Because on all systems excluding Windows child frames do not show their
> titles.  Calculating something that cannot be displayed doesn't strike
> me as a good idea.  And obviously native tooltip frames which suffer the
> same problem using
> 
> (progn
>    (setq x-gtk-use-system-tooltips nil)
>    (set-face-background 'internal-border "red"))
> 
> would still not display borders correctly.  Or should we set their
> titles too?

Is that a serious question?

Anyway, if we want to be able create frames without titles, I guess we
should simply arrange for the face cache to be cleared and the basic
faces recomputed somewhere near the beginning of redisplay_internal.

>  >> Disclaimer: In all those runs I do not know whether the
>  >>
>  >>     (set-face-background 'internal-border "red")
>  >>
>  >> set the face_change flag or something else did.
>  >
>  > It doesn't.  It sets the face_change flag of each frame instead.  See
>  > internal-set-lisp-face-attribute.
> 
> This way of setting things confuses me.  What is then that global
> face_change bool needed for?

For when we don't want to loop over all the frames setting their
individual flags, I guess.

> I have not tried yet but I'm convinced that we would fail for a normal
> frame as well if we didn't set its frame title.  This reliance of one
> (internal border color) upon the conceptually unrelated other (setting
> the frame title) looks fishy to me and IIRC causes redisplay_internal to
> initially run with old character sizes until the actual ones get
> realized when setting the frame title.

"Initially run with old character sizes" doing what?  AFAIR, we do the
frame title thingy quite early during the redisplay cycle.  What is
done before that that needs to know about the faces change?

>  >> Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
>  >> inhibit_free_realized_faces iff redisplaying_p is true?
>  >
>  > No, because some functions we call from Lisp actually write into the
>  > current matrix.  For example, tool-bar-height and tab-bar-height.
> 
> Then there's more to it than the above "the flag must be set during
> redisplay, until update_frame finished its job"?

More in what way?  This just means we sometimes set it elsewhere as
well.

>  >> So it's not so entirely trivial to do that in Elisp.
>  >
>  > Why do we need to do this in Lisp?
> 
> Didn't you propose to calculate the mode-line height from
> 'fit-window-to-buffer'?

I don't remember, but if so, it doesn't yet mean everything must be
done in Lisp.

>  >> OK.  But IIUC so far we do not allow inhibit_free_realized_faces nil
>  >> outside of redisplay_internal.  Unless someone sets
>  >> 'inhibit-free-realized-faces' which is, in general, to recommended.
>  >> Right?
>  >
>  > Yes, but I see no reason not to reset it in other places, provided
>  > that we do it carefully and only when absolutely needed.
> 
> We should say that in the doc-string of 'inhibit-free-realized-faces',
> maybe.

We should? why?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> And obviously native tooltip frames which suffer the
 >> same problem using
 >>
 >> (progn
 >>     (setq x-gtk-use-system-tooltips nil)
 >>     (set-face-background 'internal-border "red"))
 >>
 >> would still not display borders correctly.  Or should we set their
 >> titles too?
 >
 > Is that a serious question?

Given my current knowledge of the matter, yes.  Note that ATK apparently
even mandates that tooltip windows have some sort of title set.

 > Anyway, if we want to be able create frames without titles,

Our tooltips are frames and don't have titles so we do that already.

 >> This way of setting things confuses me.  What is then that global
 >> face_change bool needed for?
 >
 > For when we don't want to loop over all the frames setting their
 > individual flags, I guess.

Agreed.  But does our code adhere to that idea?  gui_set_font_backend
sets face_change to true and so does IT_set_frame_parameters (not that
it matters here - I'm just talking about the idea) while working on a
specific frame.  And in x_create_tip_frame and w32_create_tip_frame we
even have to save the value of it via face_change_before even though
these two functions really should only affect the tip frame about to be
created.  All other settings of face_change happen in xfaces.c and that
one really should know better.

 > "Initially run with old character sizes" doing what?  AFAIR, we do the
 > frame title thingy quite early during the redisplay cycle.  What is
 > done before that that needs to know about the faces change?

Earlier, when debugging this issue, I spent some time stepping through
the iterator with the old faces in place and just wondered why the new
faces would not get applied.  Whenever I'm back there I might be able to
tell more.  But before that I'd rather wait whether your

 > I guess we
 > should simply arrange for the face cache to be cleared and the basic
 > faces recomputed somewhere near the beginning of redisplay_internal.

could be implemented and solve all these problems in one rush.

 >>   >> Sheer luck, I presume.  Couldn't PRODUCE_GLYPHS set
 >>   >> inhibit_free_realized_faces iff redisplaying_p is true?
 >>   >
 >>   > No, because some functions we call from Lisp actually write into the
 >>   > current matrix.  For example, tool-bar-height and tab-bar-height.
 >>
 >> Then there's more to it than the above "the flag must be set during
 >> redisplay, until update_frame finished its job"?
 >
 > More in what way?  This just means we sometimes set it elsewhere as
 > well.

With the inevitable consequence that redisplay_internal restores it to
true when it exits.  Is that the idea behind "we sometimes set it
elsewhere as well" or just some sort of collateral damage?

 >> We should say that in the doc-string of 'inhibit-free-realized-faces',
 >> maybe.
 >
 > We should? why?

Because it might help people like me understand this issue.  Do you
think that my questions are provocative?  Maybe they are, but then
that's a consequence of the fact that I don't get the picture of our
implementation of faces.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 16 May 2020 10:44:38 +0200
> 
>  > Anyway, if we want to be able create frames without titles,
> 
> Our tooltips are frames and don't have titles so we do that already.

Yes, of course.  I hope you don't assume I didn't know that.

>  >> This way of setting things confuses me.  What is then that global
>  >> face_change bool needed for?
>  >
>  > For when we don't want to loop over all the frames setting their
>  > individual flags, I guess.
> 
> Agreed.  But does our code adhere to that idea?  gui_set_font_backend
> sets face_change to true and so does IT_set_frame_parameters (not that
> it matters here - I'm just talking about the idea) while working on a
> specific frame.  And in x_create_tip_frame and w32_create_tip_frame we
> even have to save the value of it via face_change_before even though
> these two functions really should only affect the tip frame about to be
> created.  All other settings of face_change happen in xfaces.c and that
> one really should know better.

Could you please tell where is this discussion going?  Because I no
longer understand that.  We seem to be talking around the issue, but
to what end?  Why are these details important for whatever job you
have in mind?  I'd like to help you do that job, but I'm lost in
"twisty little passages, all alike".

>  > I guess we
>  > should simply arrange for the face cache to be cleared and the basic
>  > faces recomputed somewhere near the beginning of redisplay_internal.
> 
> could be implemented and solve all these problems in one rush.

All it takes is to call free_all_realized_faces for the frame in
question, and then do what init_iterator does:

  if (FRAME_FACE_CACHE (it->f) == NULL)
    init_frame_faces (it->f);
  if (FRAME_FACE_CACHE (it->f)->used == 0)
    recompute_basic_faces (it->f);

> With the inevitable consequence that redisplay_internal restores it to
> true when it exits.  Is that the idea behind "we sometimes set it
> elsewhere as well" or just some sort of collateral damage?

I don't think it matters that this flag is set when we are outside
redisplay.

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

>> Could fit-window-to-buffer invoke
>> 'redisplay' internally, perhaps?
>
> It could but it's called way too often to warrant such behavior by
> default.  But we could give it a separate optional argument so users
> can avoid the advice.  I think Jonas could easily write and a test a
> patch along this idea.

Again, the mode-line-prettifiers are not the ones who create new buffers
and then call fit-buffer-to-window.  It's arbitrary other packages that
do that.  An optional argument therefore would not help because when one
of the prettifier modes is active, then each and every third-party
caller of fit-buffer-to-window would have to pass that optional
argument.

This is the advice I currently use:

(defvar-local moody--size-hacked-p nil)

(defun moody-redisplay (&optional _force &rest _ignored)
  (unless moody--size-hacked-p
    (setq moody--size-hacked-p t)
    (redisplay t)))

(advice-add 'fit-window-to-buffer :before #'moody-redisplay)

Of course fit-buffer-to-window itself could be changed to do that and it
could also be taught to only do so iff the user opted in to doing it.

----

Creating and displaying a new buffer and creating and resizing a new
window surely *already* causes a "redisplay" without the programmer
having to explicitly call `redisplay'.  So if we explicitly tell
fit-window-to-buffer to redisplay, then that means that we are
redisplaying twice, right?

I am under the impression (but this is just wild speculation) that
redisplay only performs some of the necessary size calculations before
doing the actual redisplaying.  But some other calculations (including
those concerning the mode-lien) are done only after the actual
redisplaying has already happened.  That is too late for this redisplay
round but causes the values to be in place for all subsequent
redisplays.  So the fix could be to do the mode-line based calculations
earlier?

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: Eli Zaretskii <eliz@gnu.org>, 38181@debbugs.gnu.org
> Date: Sat, 16 Nov 2019 16:27:12 +0100
> 
> >> Could fit-window-to-buffer invoke
> >> 'redisplay' internally, perhaps?
> >
> > It could but it's called way too often to warrant such behavior by
> > default.  But we could give it a separate optional argument so users
> > can avoid the advice.  I think Jonas could easily write and a test a
> > patch along this idea.
> 
> Again, the mode-line-prettifiers are not the ones who create new buffers
> and then call fit-buffer-to-window.  It's arbitrary other packages that
> do that.

So how are mode-line-prettifiers triggered by those packages creating
and showing new buffers?

> Of course fit-buffer-to-window itself could be changed to do that and it
> could also be taught to only do so iff the user opted in to doing it.

Can you suggest a way of knowing that this situation happened?

> Creating and displaying a new buffer and creating and resizing a new
> window surely *already* causes a "redisplay" without the programmer
> having to explicitly call `redisplay'.  So if we explicitly tell
> fit-window-to-buffer to redisplay, then that means that we are
> redisplaying twice, right?

Yes.  But if you don't call 'redisplay' _before_ fit-window-to-buffer,
that function will use stale data about the window's text area height,
computed before the mode line was updated.

You are right saying that displaying a window causes a redisplay, but
keep in mind that redisplay triggered by that happens _after_ the
command which enlarged the mode-line height finishes, and by that time
fit-window-to-buffer will have already run (using stale window
dimensions).

> I am under the impression (but this is just wild speculation) that
> redisplay only performs some of the necessary size calculations before
> doing the actual redisplaying.  But some other calculations (including
> those concerning the mode-lien) are done only after the actual
> redisplaying has already happened.  That is too late for this redisplay
> round but causes the values to be in place for all subsequent
> redisplays.  So the fix could be to do the mode-line based calculations
> earlier?

If you look at the code of redisplay_window, which is the function
that handles redisplay of each window, you will see that it indeed
calls display_mode_lines near its end (because the mode line includes
constructs that depend on position of point and other state, and that
could change as result of redisplaying a window).  However, if after
calling display_mode_lines we detect that the height of the mode line
changed, we schedule an immediate thorough redisplay.  So your theory
doesn't sound correct, at least not when taken at face value.

And once again, please keep in mind that by the time redisplay runs
(without an explicit call to 'redisplay' inside the recipe you
posted), 'fit-window-to-buffer' was already called, and it already
used stale value of height stored in the window object.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > However, if after
 > calling display_mode_lines we detect that the height of the mode line
 > changed, we schedule an immediate thorough redisplay.

... which, unfortunately, will not re-run ‘fit-window-to-buffer’ but
just use its earlier computed values.  Theoretically, 'redisplay'
could check the 'preserve-size' parameter of each window and try to
preserve the text height when resizing the mode line.  But I wouldn't
want to think of all the tricky details of such an approach.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 16 Nov 2019 20:30:55 +0100
> 
>  > However, if after
>  > calling display_mode_lines we detect that the height of the mode line
>  > changed, we schedule an immediate thorough redisplay.
> 
> ... which, unfortunately, will not re-run ‘fit-window-to-buffer’ but
> just use its earlier computed values.

Maybe fit-window-to-buffer should set some hook, which redisplay
should call if it finds out that the mode-line height changed?

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Maybe fit-window-to-buffer should set some hook, which redisplay
 > should call if it finds out that the mode-line height changed?

Maybe a two-bits sticky bit-field in the window structure called
refit_window_to_buffer.  The Lisp interface to it is a function called
'refit-window-to-buffer' setting refit_window_to_buffer to 1 if it is
zero.  'fit-window-to-buffer' always calls 'refit-window-to-buffer'.

display_mode_lines sets refit_window_to_buffer to 2 provided it is 1
and the mode line height of this window has changed.  The display
engine eventually calls 'fit-window-to-buffer' for all windows that
have refit_window_to_buffer equal 2 and sets it to 3.  When the
display engine is done with redisplay it resets refit_window_to_buffer
for all windows to 0.

This might be tricky for windows stored in configurations and states.
And always remember the fact that in one and the same display cycle
'fit-window-to-buffer' may have been called for two windows above each
other.  So this is definitely not for Emacs 27.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 17 Nov 2019 10:01:44 +0100
> 
>  > Maybe fit-window-to-buffer should set some hook, which redisplay
>  > should call if it finds out that the mode-line height changed?
> 
> Maybe a two-bits sticky bit-field in the window structure called
> refit_window_to_buffer.  The Lisp interface to it is a function called
> 'refit-window-to-buffer' setting refit_window_to_buffer to 1 if it is
> zero.  'fit-window-to-buffer' always calls 'refit-window-to-buffer'.

I'd prefer a hook, because the problem probably isn't limited to
fit-window-to-buffer.

Wait, don't we already call window-size-change-functions in this case?
If not, would it help if we did?

> display_mode_lines sets refit_window_to_buffer to 2 provided it is 1
> and the mode line height of this window has changed.

The detection of the change in mode-line height is outside
display_mode_lines, see the snippet I posted up-thread.

> The display engine eventually calls 'fit-window-to-buffer' for all
> windows that have refit_window_to_buffer equal 2 and sets it to 3.

We need to figure out the "eventually" part.  It should happen after
the windows have their dimensions updated due to the mode-line
changes.  Do you know where this happens?

> This might be tricky for windows stored in configurations and states.

Why tricky?  A stored configuration shouldn't be affected by any
changes after it's tored, no?

> And always remember the fact that in one and the same display cycle
> 'fit-window-to-buffer' may have been called for two windows above each
> other.

Mmm... actually, it could be that we cannot resize windows during
redisplay at all.  See, for example, how resize_mini_window does its
tricky job.  We may need to call this outside of redisplay.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I'd prefer a hook, because the problem probably isn't limited to
 > fit-window-to-buffer.

The display engine has to be able to ultimately control the behavior
to avoid ending up in an infinite loop.  Hooks are dangerous (though
evaluating the mode line can be dangerous too IIRC).

 > Wait, don't we already call window-size-change-functions in this case?
 > If not, would it help if we did?

That's precisely what I would like to avoid.  Note that we do not
re-run window change functions when they, for example, change the size
of a window.  Such changes are, in a sense, lost to the next redisplay
cycle.

 >> display_mode_lines sets refit_window_to_buffer to 2 provided it is 1
 >> and the mode line height of this window has changed.
 >
 > The detection of the change in mode-line height is outside
 > display_mode_lines, see the snippet I posted up-thread.

Right.  It should be done there.

 >> The display engine eventually calls 'fit-window-to-buffer' for all
 >> windows that have refit_window_to_buffer equal 2 and sets it to 3.
 >
 > We need to figure out the "eventually" part.  It should happen after
 > the windows have their dimensions updated due to the mode-line
 > changes.  Do you know where this happens?

Nowhere, hopefully.  Managing the mode-line happens in the display
engine only.  Note that a function like 'window-text-height', for
example, detracts the mode line, header line and now even the tab line
heights on-the-fly from the stored pixel_height and might even use
estimate_mode_line_height.  The stored text_height _does_ include mode
and header line.

 >> This might be tricky for windows stored in configurations and states.
 >
 > Why tricky?  A stored configuration shouldn't be affected by any
 > changes after it's tored, no?

When we restore a configuration we probably should nullify the
bit-field to avoid unwanted re-runs.  Or not nullify to ensure
re-runs?

 > Mmm... actually, it could be that we cannot resize windows during
 > redisplay at all.  See, for example, how resize_mini_window does its
 > tricky job.  We may need to call this outside of redisplay.

Inherently, resize_mini_window does what 'fit-window-to-buffer' does
and it gets called from redisplay_internal.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 17 Nov 2019 19:16:32 +0100
> 
>  > Mmm... actually, it could be that we cannot resize windows during
>  > redisplay at all.  See, for example, how resize_mini_window does its
>  > tricky job.  We may need to call this outside of redisplay.
> 
> Inherently, resize_mini_window does what 'fit-window-to-buffer' does
> and it gets called from redisplay_internal.

And it immediately returns if that happens while redisplaying a
window.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >>   > Mmm... actually, it could be that we cannot resize windows during
 >>   > redisplay at all.  See, for example, how resize_mini_window does its
 >>   > tricky job.  We may need to call this outside of redisplay.
 >>
 >> Inherently, resize_mini_window does what 'fit-window-to-buffer' does
 >> and it gets called from redisplay_internal.
 >
 > And it immediately returns if that happens while redisplaying a
 > window.

But IIUC in neither of our proposals we would have done that.  I'd
even say that we should resize windows when redisplay finds out that
the mode line height has been increased and would obscure the window
above, not leave at least one line of its window's text visible (see
the example in my other mail), ...

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 18 Nov 2019 10:45:13 +0100
> 
>  >>   > Mmm... actually, it could be that we cannot resize windows during
>  >>   > redisplay at all.  See, for example, how resize_mini_window does its
>  >>   > tricky job.  We may need to call this outside of redisplay.
>  >>
>  >> Inherently, resize_mini_window does what 'fit-window-to-buffer' does
>  >> and it gets called from redisplay_internal.
>  >
>  > And it immediately returns if that happens while redisplaying a
>  > window.
> 
> But IIUC in neither of our proposals we would have done that.

I wouldn't be so sure: the code fragment I've shown that detects
mode-line height changes is inside redisplay_window.

> I'd even say that we should resize windows when redisplay finds out
> that the mode line height has been increased and would obscure the
> window above, not leave at least one line of its window's text
> visible

Some might dislike such side effects, I think, for the same reason
some dislike the resizing which happens due to mini-window growth.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> But IIUC in neither of our proposals we would have done that.
 >
 > I wouldn't be so sure: the code fragment I've shown that detects
 > mode-line height changes is inside redisplay_window.

But we should collect evidence from all windows first before
redisplaying again.

 >> I'd even say that we should resize windows when redisplay finds out
 >> that the mode line height has been increased and would obscure the
 >> window above, not leave at least one line of its window's text
 >> visible
 >
 > Some might dislike such side effects, I think, for the same reason
 > some dislike the resizing which happens due to mini-window growth.

You probably misunderstood.  I meant the example from my other mail
where enlarging the mode line covers the entire text area of its
window.  In that case we sooner or later will resize anyway, that is
when the window resizing code becomes aware of the fact.

martin

bug#38181: Actual height of mode-line not taken into account

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> You are right saying that displaying a window causes a redisplay, but
> keep in mind that redisplay triggered by that happens _after_ the
> command which enlarged the mode-line height finishes, and by that time
> fit-window-to-buffer will have already run (using stale window
> dimensions).

Thanks for that reminder.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Again, the mode-line-prettifiers are not the ones who create new buffers
 > and then call fit-buffer-to-window.  It's arbitrary other packages that
 > do that.  An optional argument therefore would not help because when one
 > of the prettifier modes is active, then each and every third-party
 > caller of fit-buffer-to-window would have to pass that optional
 > argument.

I see.  Maybe a function 'set-default-mode-line-format' would be
useful here.  Anyway: At the time you prettify a mode line or show a
new buffer in a window with a to be prettified mode line, would doing
an immediate redisplay work around future problems?

 > This is the advice I currently use:
 >
 > (defvar-local moody--size-hacked-p nil)
 >
 > (defun moody-redisplay (&optional _force &rest _ignored)
 >    (unless moody--size-hacked-p
 >      (setq moody--size-hacked-p t)
 >      (redisplay t)))
 >
 > (advice-add 'fit-window-to-buffer :before #'moody-redisplay)
 >
 > Of course fit-buffer-to-window itself could be changed to do that and it
 > could also be taught to only do so iff the user opted in to doing it.

If we don't want 'fit-window-to-buffer' to do that always we'd need
some variable, either buffer local or even a window parameter, that
'fit-window-to-buffer' would inspect once and reset immediately in
order to perform only the redisplay call that's really needed.

 > Creating and displaying a new buffer and creating and resizing a new
 > window surely *already* causes a "redisplay" without the programmer
 > having to explicitly call `redisplay'.  So if we explicitly tell
 > fit-window-to-buffer to redisplay, then that means that we are
 > redisplaying twice, right?

I think so.  Maybe 'fit-window-to-buffer' could use the string
returned by 'format-mode-line' instead and calculate its height
without redisplaying anything.

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi,

I'm seeing what I believe is an instance of this issue in a vanilla
build from emacs-28 branch, ns/cocoa backed.

Here are some instructions for reproduction:
https://github.com/seagle0128/doom-modeline/issues/480#issue-1027051204

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I'm seeing what I believe is an instance of this issue in a vanilla
 > build from emacs-28 branch, ns/cocoa backed.
 >
 > Here are some instructions for reproduction:
 > https://github.com/seagle0128/doom-modeline/issues/480#issue-1027051204

If the underlying scenario is that you first fit a window to its buffer
and then enlarge the height of that window's mode-line, there's not much
we can do here currently.  It's like fitting a window to its buffer and
then adding a header line or a horizontal scroll bar to that window.

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Martin,

> If the underlying scenario is that you first fit a window to its buffer
> and then enlarge the height of that window's mode-line, there's not much
> we can do here currently.  It's like fitting a window to its buffer and
> then adding a header line or a horizontal scroll bar to that window.

I can't say I understand the details discussed here but I'm not quite
sure your description fits the situation.

The workaround since this bug was first reported has been: immediately
before fit-window-to-buffer execute an advice that forces a redisplay,
then deactivate the advice.

But the problem I'm seeing now is that the first time
org-set-tags-command is executed its popup is correctly sized (with
said workaround in place, of course), but further executions show a
popup with the wrong geometry that trims part of the text off. If I
remove the "just once" clause in the workaround, so that the redisplay
is forced after each fit-window-to-buffer execution, then the layout
is correct every time. It goes without saying that
org-set-tags-command is ultimately triggering the execution of
fit-window-to-buffer, but that stuff you probably know much better
than me. Thing is, there was no enlargement of the modeline
in-between, as far as I can understand and see nothing changed in this
regard between the first and the second execution of
org-set-tags-command.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita2@gmail.com>
> Date: Fri, 15 Oct 2021 04:26:51 -0300
> Cc: 38181@debbugs.gnu.org
> 
> > If the underlying scenario is that you first fit a window to its buffer
> > and then enlarge the height of that window's mode-line, there's not much
> > we can do here currently.  It's like fitting a window to its buffer and
> > then adding a header line or a horizontal scroll bar to that window.
> 
> I can't say I understand the details discussed here but I'm not quite
> sure your description fits the situation.
> 
> The workaround since this bug was first reported has been: immediately
> before fit-window-to-buffer execute an advice that forces a redisplay,
> then deactivate the advice.
> 
> But the problem I'm seeing now is that the first time
> org-set-tags-command is executed its popup is correctly sized (with
> said workaround in place, of course), but further executions show a
> popup with the wrong geometry that trims part of the text off. If I
> remove the "just once" clause in the workaround, so that the redisplay
> is forced after each fit-window-to-buffer execution, then the layout
> is correct every time. It goes without saying that
> org-set-tags-command is ultimately triggering the execution of
> fit-window-to-buffer, but that stuff you probably know much better
> than me. Thing is, there was no enlargement of the modeline
> in-between, as far as I can understand and see nothing changed in this
> regard between the first and the second execution of
> org-set-tags-command.

Can you show the recipe for the problem, starting from "emacs -Q"?

It sounds from what you say that the problem is not with the mode
line, but with something else.  If that is true, please submit a new
bug report.  This bug is about redisplaying the mode line when
something changes in mode-line-format that affects the height of the
mode line.  If there are other problems with fit-window-to-buffer,
they should be discussed separately.

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

> Can you show the recipe for the problem, starting from "emacs -Q"?

Yes, of course, the example Jonas provided in the opening post will do
it with a small modification.

1. Execute the code.
2. Call test-popup and check that the layout is wrong.
3. Redefine test-popup with (redisplay) uncommented.
4. Call it again and check that the layout is right.
5. Redefine test-popup with (redisplay) commented again.
6. Call it again and check that the layout is wrong again.

Now, consider the workaround [1] that seemingly has been doing the job
for almost two years in at least moody and doom modelines. AFAICS if
this workaround was working then, (6) above is showing us something
new. Or perhaps it's just my setup and the workaround still works for
everybody else.

> It sounds from what you say that the problem is not with the mode line, but with something else.  If that is true, please submit a new bug report.

Please, you tell me what to do in this regard because I'm unable to
tell if the problem is in the modeline or elsewhere. To my untrained
eye it looks quite similar to what was reported here, although with
the aforementioned difference.

Best regards,
Carlos

---

[1] https://github.com/tarsius/moody/blob/9b679400ca885b8ff51bcfd75b87f79d66c0ee26/moody.el#L303

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > But the problem I'm seeing now is that the first time
 > org-set-tags-command is executed its popup is correctly sized (with
 > said workaround in place, of course), but further executions show a
 > popup with the wrong geometry that trims part of the text off. If I
 > remove the "just once" clause in the workaround, so that the redisplay
 > is forced after each fit-window-to-buffer execution, then the layout
 > is correct every time. It goes without saying that
 > org-set-tags-command is ultimately triggering the execution of
 > fit-window-to-buffer, but that stuff you probably know much better
 > than me. Thing is, there was no enlargement of the modeline
 > in-between, as far as I can understand and see nothing changed in this
 > regard between the first and the second execution of
 > org-set-tags-command.

I'm not sure I follow.  What is the purpose of "the redisplay is forced
after each fit-window-to-buffer execution"?  If you want a new mode line
height to be taken into account by 'fit-window-to-buffer', you probably
should do any such redisplay _before_ executing 'fit-window-to-buffer'.
Or what am I missing?

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

On Fri, Oct 15, 2021 at 5:35 AM martin rudalics <rudalics@gmx.at> wrote:

> I'm not sure I follow.  What is the purpose of "the redisplay is forced
> after each fit-window-to-buffer execution"?

My bad, it wasn't the best way to express it. It's executed before, of
course, it's a before advice. What I meant is that previously we had
the sequence:

1. redisplay fit fit ...

and now:

2. redisplay fit redisplay fit redisplay ...

Supposedly 1 was a fix for:

0. fit fit fit ...

but now I need 2.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

>  > 2. redisplay fit redisplay fit redisplay ...

> Sorry.  I still don't understand the purpose of the last redisplay here.

It's not the last, it's a repeating sequence, I left the dangling
redisplay instead of ending it in a fit because I was explaining my
unfortunate choice of words when I said "after", but it's formally the
same, the point being that the first redisplay is not enough.

> Does
>
> 2. redisplay fit redisplay fit ...
>
> fail?  What are the values returned by 'window-mode-line-height' after
> each redisplay?

As far as I can see this works, as I said before. In the example
provided by Jonas window-mode-line-height is 29 immediately after
calling set-face-attribute, but after setting mode-line-format it's
always 60, no matter if I call redisplay or fit or none of them or in
what order. But even if the reported height is the same, the layout is
not, omitting the (previous) redisplay always crops the text.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >>   > 2. redisplay fit redisplay fit redisplay ...
 >
 >> Sorry.  I still don't understand the purpose of the last redisplay here.
 >
 > It's not the last, it's a repeating sequence, I left the dangling
 > redisplay instead of ending it in a fit because I was explaining my
 > unfortunate choice of words when I said "after", but it's formally the
 > same, the point being that the first redisplay is not enough.

OK.  Can we settle on something like

2. redisplay fit redisplay fit redisplay ... fit

Bear with me but I'm trying to simultaneously understand what you are
doing and what Emacs is doing.  This is multitasking and I'm bad at
that.

 >> Does
 >>
 >> 2. redisplay fit redisplay fit ...
 >>
 >> fail?  What are the values returned by 'window-mode-line-height' after
 >> each redisplay?
 >
 > As far as I can see this works,

What is "this" here?

 > as I said before. In the example
 > provided by Jonas window-mode-line-height is 29 immediately after
 > calling set-face-attribute, but after setting mode-line-format it's
 > always 60, no matter if I call redisplay or fit or none of them or in
 > what order. But even if the reported height is the same, the layout is
 > not, omitting the (previous) redisplay always crops the text.

I'm currently not even sure what we are comparing here.  Let's settle on
the following idioms:

(defun selected-window-mode-line-height ()
   (pos-visible-in-window-p t nil t)
   (window-mode-line-height))

(let ((val (selected-window-mode-line-height)))
   (fit-window-to-buffer)
   (cons (selected-window-mode-line-height) val))

Do you see any change in the return value here?  And when you run this
repeatedly as in the

redisplay fit redisplay fit redisplay ... fit

scenario?  And with 'set-face-attribute' and setting 'mode-line-format'
intermingled?  Maybe Emacs sometimes doesn't cache the result of an
earlier call of display_mode_line although it could (or should).

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Martin,

>>> Does  2. redisplay fit redisplay fit ... fail?

>> As far as I can see this works,

> What is "this" here?

Well, it is "that" there :). "redisplay fit redisplay fit ..." works just fine.
Details below.

> I'm currently not even sure what we are comparing here.

Let's see. I run emacs -q. Then:

(window-mode-line-height) -> 14
(fit-window-to-buffer)
(window-mode-line-height) -> 14

(set-face-attribute 'mode-line nil :height 250)
(set-face-attribute 'mode-line-inactive nil :height 250)
(window-mode-line-height) -> 29
(fit-window-to-buffer)
(window-mode-line-height) -> 29

(setq-default mode-line-format ...) ; full version in Jonas' post
(window-mode-line-height) -> 60
(fit-window-to-buffer)
(window-mode-line-height) -> 60
(redisplay t)
(window-mode-line-height) -> 60
(fit-window-to-buffer)
(window-mode-line-height) -> 60
(redisplay t)
(window-mode-line-height) -> 60
etc

This is not actually very interesting nor informative. The numbers are
fine, but it's always the same window and I guess I'm never in that
instant when fit-window-to-buffer runs before some calculations it
requires have taken place.

Now, let's define test-popup-no-redisplay as the version of test-popup
with the call to redisplay removed, and test-popup-redisplay as the
version that calls redisplay before fitting the window to its buffer:

(test-popup-no-redisplay) -> cropped content
close popup
(test-popup-no-redisplay) -> cropped content
close popup
(test-popup-redisplay) -> ok
close popup
(test-popup-redisplay) -> ok
close popup
(test-popup-no-redisplay) -> cropped content
close popup

As you can see, the redisplay only "fixes" the next fit, after that the
problem reappears.

This may be of interest:

(test-popup-no-redisplay) -> cropped content
(test-popup-no-redisplay) -> ok
(test-popup-no-redisplay) -> ok
(test-popup-no-redisplay) -> ok

(N.B. I'm not closing the popup) It seems like the window was reused
and the second time it got things right.

My interpretation is that the early redisplay "prepares" the current
window for the fit. Now, the workaround that I linked above does a
redisplay once per buffer, not once per window. The issue I observe,
which I believe is the same one that motivated this report in the
first place, is an org-set-tags-command menu clipped at the bottom
(it's not the only case, though). Since the popup windows that
org-mode opens for this and other menus are transient but their
buffers likely remain hidden, that may be the reason why the
workaround is not, well, working around. What I'm failing to grasp is
how could it ever work... maybe there was some change in the
implementation of org-mode popups.

I would be happy with a sound statement like "if you change the
modeline height so it is different to the default char size, you need
to call redisplay for each window that sports the modified modeline
before executing any operation that requires knowledge of the geometry
of that modeline, including fit-window-to-buffer". If that's true then the
current trick could be reasonably modified to cope with every possible
case instead of hooking to particular functions (fit-window-to-buffer) in
maybe the wrong scope (buffer), by just triggering an early redisplay
each time a new window is created.

Rereading (some of) the above thread, I noticed you said:

> If we don't want 'fit-window-to-buffer' to do that always we'd need
> some variable, either buffer local or even a window parameter, that
> 'fit-window-to-buffer' would inspect once and reset immediately in
> order to perform only the redisplay call that's really needed.

I believe this is similar to what I've just suggested. Then a long
discussion full of technicalities that I won't be able to follow in
depth anytime soon took place. I understand that maybe forcing an
early display is not ideal because, in principle, we only want to
update the size of the modeline, not prematurely redisplay the window.
Moreover, modelines could change after creating a window,
but this is not an interesting use case in real life.
But since creating a window is not a frequent operation, and
changing a modeline on the fly is not a likely event, would it be
so bad to trigger that early redisplay on window creation?
I'm not saying that emacs should do it, I'm confident that your
decision in this regard will be far better than anything I could
come up with, I'm just looking for a workaround that gets the job
done or, alternatively, the certainty that it's a bad idea and that we
should refrain from pretiffing modelines.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Now, let's define test-popup-no-redisplay as the version of test-popup
 > with the call to redisplay removed, and test-popup-redisplay as the
 > version that calls redisplay before fitting the window to its buffer:
 >
 > (test-popup-no-redisplay) -> cropped content
 > close popup
 > (test-popup-no-redisplay) -> cropped content
 > close popup
 > (test-popup-redisplay) -> ok
 > close popup
 > (test-popup-redisplay) -> ok
 > close popup
 > (test-popup-no-redisplay) -> cropped content
 > close popup
 >
 > As you can see, the redisplay only "fixes" the next fit, after that the
 > problem reappears.

Right.  The "close popup" removes anything redisplay cached for the
window's mode line in the last call of 'test-popup-redisplay'.

 > This may be of interest:
 >
 > (test-popup-no-redisplay) -> cropped content
 > (test-popup-no-redisplay) -> ok
 > (test-popup-no-redisplay) -> ok
 > (test-popup-no-redisplay) -> ok
 >
 > (N.B. I'm not closing the popup) It seems like the window was reused
 > and the second time it got things right.

Right.  It always uses the cached mode line height in this case.

 > My interpretation is that the early redisplay "prepares" the current
 > window for the fit. Now, the workaround that I linked above does a
 > redisplay once per buffer, not once per window.

What is the "workaround"?  IIUC there is no "redisplay" one can call on
a "per buffer" base.

 > The issue I observe,
 > which I believe is the same one that motivated this report in the
 > first place, is an org-set-tags-command menu clipped at the bottom
 > (it's not the only case, though). Since the popup windows that
 > org-mode opens for this and other menus are transient but their
 > buffers likely remain hidden, that may be the reason why the
 > workaround is not, well, working around. What I'm failing to grasp is
 > how could it ever work... maybe there was some change in the
 > implementation of org-mode popups.
 >
 > I would be happy with a sound statement like "if you change the
 > modeline height so it is different to the default char size, you need
 > to call redisplay for each window that sports the modified modeline
 > before executing any operation that requires knowledge of the geometry
 > of that modeline, including fit-window-to-buffer". If that's true then the
 > current trick could be reasonably modified to cope with every possible
 > case instead of hooking to particular functions (fit-window-to-buffer) in
 > maybe the wrong scope (buffer), by just triggering an early redisplay
 > each time a new window is created.

Even the 'redisplay' trick will not be sufficient: Consider Eli's
scenario in this thread but with _different_ heights for active and
inactive mode lines.  It will break things when after doing the
'fit-window-to-buffer' call you (de-)select the window you've just fit.

 > Rereading (some of) the above thread, I noticed you said:
 >
 >> If we don't want 'fit-window-to-buffer' to do that always we'd need
 >> some variable, either buffer local or even a window parameter, that
 >> 'fit-window-to-buffer' would inspect once and reset immediately in
 >> order to perform only the redisplay call that's really needed.
 >
 > I believe this is similar to what I've just suggested. Then a long
 > discussion full of technicalities that I won't be able to follow in
 > depth anytime soon took place. I understand that maybe forcing an
 > early display is not ideal because, in principle, we only want to
 > update the size of the modeline, not prematurely redisplay the window.
 > Moreover, modelines could change after creating a window,
 > but this is not an interesting use case in real life.
 > But since creating a window is not a frequent operation, and
 > changing a modeline on the fly is not a likely event, would it be
 > so bad to trigger that early redisplay on window creation?

I think you mean with every set_window_buffer?  And probably with every
set_window_configuration too.  Did you try it?

 > I'm not saying that emacs should do it, I'm confident that your
 > decision in this regard will be far better than anything I could
 > come up with, I'm just looking for a workaround that gets the job
 > done or, alternatively, the certainty that it's a bad idea and that we
 > should refrain from pretiffing modelines.

Here I have no problems with the scenarios I've seen in this thread
because I update the mode line caches with every change of a window
height (which includes the case where a window is created) and with any
change of a buffer local variable like 'mode-line-format'.  So basically
no redisplay is ever needed in the first place, just a recalculation of
the mode line heights of all windows whose heights have changed.

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Martin, thank you for the commentary.

> What is the "workaround"?  IIUC there is no "redisplay" one can call on
> a "per buffer" base.

The one I linked above which some packages have been using for almost
a couple of years now and, indeed, was mentioned early in this thread
but never questioned AFAICS, namely:

(defvar-local moody--size-hacked-p nil)

(defun moody-redisplay (&optional _force &rest _ignored)
  (when (and mode-line-format (not moody--size-hacked-p))
    (setq moody--size-hacked-p t)
    (redisplay t)))

(advice-add 'fit-window-to-buffer :before #'moody-redisplay)

The problem I see with it is that buffers often survive their windows
and are later reused in other windows. This happens with some of the
popups that fit to their buffers we are talking about. Moreover, using
a window parameter instead of the buffer local won't be enough,
because sometimes fit-window-to-buffer is called from another window
(I mean, not the popup window) after the excursion that prepares the
popup, so the redisplay ends up running in the wrong window.

> Even the 'redisplay' trick will not be sufficient: Consider Eli's
> scenario in this thread but with _different_ heights for active and
> inactive mode lines.  It will break things when after doing the
> 'fit-window-to-buffer' call you (de-)select the window you've just fit.

I think I follow the argument but I don't see it as having practical
interest for the use cases that motivated this report. All modeline
customizations I'm aware of use a single modeline height, although the
modeline contents and faces may of course change with major and minor
modes and activity status.

>  > changing a modeline on the fly is not a likely event, would it be
>  > so bad to trigger that early redisplay on window creation?
>
> I think you mean with every set_window_buffer?  And probably with every
> set_window_configuration too.  Did you try it?

I still need to refine my concept of "on window creation". When you say
"with evert set_window_buffer" I'm not sure whether you mean something
like:

1. There is an event (a) of window creation and later and event (b)
when the buffer is set for the new window. So if I call redisplay
immediately after (a) it would be too early because the modeline won't
be properly set up until (b).

Or, instead, something like:

2. Windows often change buffers and buffers may have modeline formats
that imply different modeline heights.

If 1 is the case then sure, maybe it's a different event that I need
to listen to, but it's still one forced early redisplay per window.
But if you mean 2, again I don't see the relevance in practice.

With regard to set_window_configuration similar observations can be done.

> change of a buffer local variable like 'mode-line-format'.  So basically
> no redisplay is ever needed in the first place, just a recalculation of
> the mode line heights of all windows whose heights have changed.

This is good to know.

Thank you again.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I still need to refine my concept of "on window creation". When you say
 > "with evert set_window_buffer" I'm not sure whether you mean something
 > like:
 >
 > 1. There is an event (a) of window creation and later and event (b)
 > when the buffer is set for the new window. So if I call redisplay
 > immediately after (a) it would be too early because the modeline won't
 > be properly set up until (b).

Window creation means 'split-window' which assigns the new window a
buffer that appears in the window that was split.

 > Or, instead, something like:
 >
 > 2. Windows often change buffers and buffers may have modeline formats
 > that imply different modeline heights.
 >
 > If 1 is the case then sure, maybe it's a different event that I need
 > to listen to, but it's still one forced early redisplay per window.
 > But if you mean 2, again I don't see the relevance in practice.

How comes?  'display-buffer-in-side-window' may reuse an existing window
or pop up a new one.  Or do the mode lines of _all_ your windows have
the same height?  That indeed would simplify things a lot.

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

I was just about to email you with some questions that you now have
answered to some extent. Anyway, I'm copying my original questions
with some annotations.

> > I think you mean with every set_window_buffer?  And probably with every
> > set_window_configuration too.  Did you try it?
>
> I still need to refine my concept of "on window creation".

Well, any relevant window hook in [1] is advertised as "called during
redisplay", so that surely isn't the place to trigger that same
redisplay to begin with...

Let's go back to advising then. Here are some questions I have:

1. Is there a primitive function that is like the mother of all
windows? There is no window-create nor create-window alikes, so maybe
split-window-internal?

    > Window creation means 'split-window' which assigns the new window a
    > buffer that appears in the window that was split.

    So this more or less settles the question, but to be more precise:
    do you see any difference of relevance between advicing split-window
    and split-window-internal?

2. Or is it better to advise set-window-buffer/configuration for
whatever reasons? Any or both of them?

3. If advising set-window-buffer/configuration is the preferred
method, I'm still assuming that we will keep a window parameter in
order to disable the forced redisplay after its first execution for the
window. Is there any reason not do so (taking into account what
I said before of the target use case: a single modeline height for
every modeline during the entire session).

     > Or do the mode lines of _all_ your windows have
     > the same height?  That indeed would simplify things a lot.

     Yes, this is by far the usual case and I believe it's a fact that
     favours the "advise window creation" strategy over the
     "advise window buffer/config change events" (with or without
     the "just once per window" clause) strategy.

Thanks again,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

In practice this seems to nicely cover the relevant cases:

(defun my-redisplay-hack (&rest _)
  (when mode-line-format
    (redisplay t)))
(advice-add #'split-window :after #'my-redisplay-hack)

but there is some flickering because the window is displayed before
the fit and then immediately replaced by the fitted version. Although
I see no way around that glitch, so it might be the price to pay.

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita2@gmail.com>
> Date: Sat, 16 Oct 2021 18:27:10 -0300
> Cc: 38181@debbugs.gnu.org
> 
> In practice this seems to nicely cover the relevant cases:
> 
> (defun my-redisplay-hack (&rest _)
>   (when mode-line-format
>     (redisplay t)))
> (advice-add #'split-window :after #'my-redisplay-hack)

I wonder how this fixes anything, since split-window already triggers
redisplay.  Martin, do you understand the difference?

> but there is some flickering because the window is displayed before
> the fit and then immediately replaced by the fitted version.

Which means there are two redisplay cycles, and the first one (which
produces incorrect display) is the one from your hack.  So once again,
I wonder why that call to 'redisplay' is needed.

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Eli,

> > In practice this seems to nicely cover the relevant cases:
> >
> > (defun my-redisplay-hack (&rest _)
> >   (when mode-line-format
> >     (redisplay t)))
> > (advice-add #'split-window :after #'my-redisplay-hack)
>
> I wonder how this fixes anything, since split-window already triggers
> redisplay.

In the use cases discussed above, IIUC the problem is that the
redisplay is triggered at some point, but only after
fit-window-to-buffer has already taken place.

Consider for example what org-attach does:

1. Create a new window
2. Enter a window excursion and insert stuff into the new window
3. After the excursion fit the new window to the buffer (this is done
from another window)
4. Wait for user input

Seemingly, at point 3 the redisplay has not happened yet. The
redisplay-before-fit advice won't work in this case because the
current window is not the popup anymore when the fit (and
hence the forced redisplay) happens.

Another different example is org-set-tags-command, this is what it does:

1. Create a new window
2. Enter a window excursion and insert stuff into the new window
3. Inside the excursion fit the new window to the buffer (now this is
done while the popup is the current window)
4. After the excursion, wait for user input

So in this case the redisplay-before-fit advice works fine since it's
triggered inside the popup. But then the popup is killed while its
buffer remains hidden, outliving the popup. Remember that the advice
is really a redisplay-before-fit-once-per-buffer advice, so the next
time you call org-set-tags-command it will fail to redisplay.

This is why I believe these cases weren't properly addressed by the
previous workaround but are now.

> Which means there are two redisplay cycles, and the first one (which
> produces incorrect display) is the one from your hack.  So once again,
> I wonder why that call to 'redisplay' is needed.

Yes, I know that is the cause. The redisplay is not needed per se and,
worse, produces undesirable visual artifacts, but apparently it's the
only way we have to update the information about the mode-line height
before fit-window-to-buffer runs.

Now, if you are saying that step 1 in the examples above already
triggered a redisplay, and if a redisplay always updates whatever
information about the modeline geometry that needs to be updated, then
something is not adding up.

Best regards
--
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 >> In practice this seems to nicely cover the relevant cases:
 >>
 >> (defun my-redisplay-hack (&rest _)
 >>    (when mode-line-format
 >>      (redisplay t)))
 >> (advice-add #'split-window :after #'my-redisplay-hack)
 >
 > I wonder how this fixes anything, since split-window already triggers
 > redisplay.  Martin, do you understand the difference?

There is no difference in the use cases posted here.  The results will
change only when a window is reused and then fit to its buffer.

 >> but there is some flickering because the window is displayed before
 >> the fit and then immediately replaced by the fitted version.
 >
 > Which means there are two redisplay cycles, and the first one (which
 > produces incorrect display) is the one from your hack.  So once again,
 > I wonder why that call to 'redisplay' is needed.

Where "incorrect redisplay" stands for "window that has not been yet fit
to its buffer".

martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > In practice this seems to nicely cover the relevant cases:
 >
 > (defun my-redisplay-hack (&rest _)
 >    (when mode-line-format
 >      (redisplay t)))
 > (advice-add #'split-window :after #'my-redisplay-hack)
 >
 > but there is some flickering because the window is displayed before
 > the fit and then immediately replaced by the fitted version. Although
 > I see no way around that glitch, so it might be the price to pay.

Nothing new here.  The sequence remains the same as before:
'split-window', 'redisplay', 'fit-window-to-buffer'.

And remember: People usually tolerate when Emacs occasionally crashes.
They never tolerate flickering.

martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > Well, any relevant window hook in [1] is advertised as "called during
 > redisplay", so that surely isn't the place to trigger that same
 > redisplay to begin with...

Roughly spoken, redisplay calculates the height of the minibuffer window
and those of the mode lines and, if any of these changed since last
redisplay, does another redisplay in the hope that they stick to their
values now.  After that, it runs the hooks you mention above so these
can take the now final (wrt redisplay) window and mode line sizes into
account.

A function on any of these hooks that changes a mode line height or a
window size does that on its own risk.  It's _not_ recommended.

 > Let's go back to advising then. Here are some questions I have:
 >
 > 1. Is there a primitive function that is like the mother of all
 > windows?

I always wanted to write one to make 'display-buffer' simpler but nobody
was enthusiastic about it.

 > There is no window-create nor create-window alikes, so maybe
 > split-window-internal?
 >
 >      > Window creation means 'split-window' which assigns the new window a
 >      > buffer that appears in the window that was split.
 >
 >      So this more or less settles the question, but to be more precise:
 >      do you see any difference of relevance between advicing split-window
 >      and split-window-internal?

Never advise an internal function or one with an '-internal' postfix.
Such function may change or be removed at any time.

 > 2. Or is it better to advise set-window-buffer/configuration for
 > whatever reasons? Any or both of them?

Both.  Especially because you may want to reuse a window for showing
another buffer in it.

 > 3. If advising set-window-buffer/configuration is the preferred
 > method, I'm still assuming that we will keep a window parameter in
 > order to disable the forced redisplay after its first execution for the
 > window. Is there any reason not do so (taking into account what
 > I said before of the target use case: a single modeline height for
 > every modeline during the entire session).

The mode line height is window specific, taking into account what the
buffer, the window and the window's frame have set up for it.

 >       > Or do the mode lines of _all_ your windows have
 >       > the same height?  That indeed would simplify things a lot.
 >
 >       Yes, this is by far the usual case and I believe it's a fact that
 >       favours the "advise window creation" strategy over the
 >       "advise window buffer/config change events" (with or without
 >       the "just once per window" clause) strategy.

In one of my earlier postings I falsely assumed that we _could_ retrieve
the prospective heights of mode lines by calling
'pos-visible-in-window-p' followed by 'window-mode-line-height',
forgetting that the former restores the previous values after doing its
calculations.  By looking at these functions you could derive a new one
that calculates mode line heights in a similar way and peruse the return
values of that function in 'window-text-pixel-size' in the hope that
they won't change with the next redisplay.  The 'mode-lines' argument of
'window-text-pixel-size' should then accept a value like 'update' to
reflect this special use case.  It might be worth the experience.

martin

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

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

 > By looking at these functions you could derive a new one
 > that calculates mode line heights in a similar way and peruse the return
 > values of that function in 'window-text-pixel-size' in the hope that
 > they won't change with the next redisplay.  The 'mode-lines' argument of
 > 'window-text-pixel-size' should then accept a value like 'update' to
 > reflect this special use case.  It might be worth the experience.

Or try the attached patch on master.  It seems to fix the issue here.

martin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xdisp.c.diff --]
[-- Type: text/x-patch; name="xdisp.c.diff", Size: 2042 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 783ef396a3..ad833d99e3 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10847,17 +10847,45 @@ DEFUN ("window-text-pixel-size", Fwindow_text_pixel_size, Swindow_text_pixel_siz
   if (y > max_y)
     y = max_y;

-  if (EQ (mode_lines, Qtab_line) || EQ (mode_lines, Qt))
-    /* Re-add height of tab-line as requested.  */
-    y = y + WINDOW_TAB_LINE_HEIGHT (w);
+  if ((EQ (mode_lines, Qtab_line)
+       || EQ (mode_lines, Qt))
+      && window_wants_tab_line (w))
+    /* Add height of tab-line as requested.  */
+    {
+      Lisp_Object window_tab_line_format
+	= window_parameter (w, Qtab_line_format);
+
+      y = y + display_mode_line (w, TAB_LINE_FACE_ID,
+				 NILP (window_tab_line_format)
+				 ? BVAR (current_buffer, tab_line_format)
+				 : window_tab_line_format);
+    }

-  if (EQ (mode_lines, Qheader_line) || EQ (mode_lines, Qt))
-    /* Re-add height of header-line as requested.  */
-    y = y + WINDOW_HEADER_LINE_HEIGHT (w);
+  if ((EQ (mode_lines, Qheader_line)
+       || EQ (mode_lines, Qt))
+      && window_wants_header_line (w))
+    {
+      Lisp_Object window_header_line_format
+	= window_parameter (w, Qheader_line_format);

-  if (EQ (mode_lines, Qmode_line) || EQ (mode_lines, Qt))
-    /* Add height of mode-line as requested.  */
-    y = y + WINDOW_MODE_LINE_HEIGHT (w);
+      y = y + display_mode_line (w, HEADER_LINE_FACE_ID,
+				 NILP (window_header_line_format)
+				 ? BVAR (current_buffer, header_line_format)
+				 : window_header_line_format);
+    }
+
+  if ((EQ (mode_lines, Qmode_line)
+       || EQ (mode_lines, Qt))
+      && window_wants_mode_line (w))
+    {
+      Lisp_Object window_mode_line_format
+	= window_parameter (w, Qmode_line_format);
+
+      y = y + display_mode_line (w, CURRENT_MODE_LINE_FACE_ID (w),
+				 NILP (window_mode_line_format)
+				 ? BVAR (current_buffer, mode_line_format)
+				 : window_mode_line_format);
+    }

   bidi_unshelve_cache (itdata, false);

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Martin,

Thank you very much.

I've tested it in both master and emacs-28 branches and I've seen none
of the aforementioned issues nor, at least in a cursory examination,
any regression.

It seems like you're displaying the modeline instead of merely using
the current height? There is no perceptible flickering in any case.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > I've tested it in both master and emacs-28 branches and I've seen none
 > of the aforementioned issues nor, at least in a cursory examination,
 > any regression.
 >
 > It seems like you're displaying the modeline instead of merely using
 > the current height? There is no perceptible flickering in any case.

There's no "displaying" involved.  IIUC display_mode_line has become a
misnomer ever since it's used in pos_visible_p.  Basically, I do two
things: Run 'format-mode-line' and compute_line_metrics on the current
mode line strings.

Eli, I think this could go into Emacs 28.  WDYT?

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 18 Oct 2021 19:44:08 +0200
> Cc: 38181@debbugs.gnu.org
> 
>  > I've tested it in both master and emacs-28 branches and I've seen none
>  > of the aforementioned issues nor, at least in a cursory examination,
>  > any regression.
>  >
>  > It seems like you're displaying the modeline instead of merely using
>  > the current height? There is no perceptible flickering in any case.
> 
> There's no "displaying" involved.

There is, for some value of "display": this function actually
generates a glyph row in the glyph matrix.

> Eli, I think this could go into Emacs 28.  WDYT?

No, it's too radical, and the problem it fixes is too obscure.  Let's
install it on master, please.

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

(Sorry if it ends up being a dup but I'm resending this because I'd
previously sent it with an attached screencast and it seems that it's
not being accepted by the bug tracker. In any case, it was the wrong
cast :P. Please let me know if there's a policy wrt screencasts,
even the right ones. I think it could be useful in this case but it's
not that important.)

Hi,

Here is one more example that tries to capture the essence of another
popup dialog in org that is giving me headaches. It's fixed by
applying Martin's patch, it's not "fixed" with the
redisplay-before-fit workaround (although the redisplay does make a
difference as shown below) and it's "fixed" with the
redisplay-after-creation workaround.
1) I run emacs -q and execute this simple code that opens a window
with 30 numbers, each in its own line:

(progn
  (switch-to-buffer-other-window "popup")
  (erase-buffer)
  (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
  (fit-window-to-buffer))

The window properly fits the numbers, which are all visible  (please
ensure your screen and frame is large enough). Now I close the window
and make the modeline bigger as in Jonas' example:

(setq-default
 mode-line-format
 (cons (propertize " " 'display
                   (create-image "/* XPM */ static char * image[] = {
\"3 60 1 1\",
\"0 c #00aaff\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\"
};" 'xpm t :ascent 'center))
       mode-line-format))

2) If I run the code above one more time, that is:

(progn
  (switch-to-buffer-other-window "popup")
  (erase-buffer)
  (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
  (fit-window-to-buffer))

The numbers are only partially visible, the window looks as if I had
scrolled the buffer down to the bottom. Scrolling up it's easy to see
that not all of the numbers fit the window. I guess this is expected
because of the miscalculations regarding the modeline height.

3) Now I add a redisplay before the fit:

(progn
  (switch-to-buffer-other-window "popup")
  (erase-buffer)
  (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
  (redisplay t)
  (fit-window-to-buffer))

Again, only about half of the numbers are visible and the window looks
as if its buffer has been scrolled down. But now by scrolling up I can
check that all the numbers fit the window. So the height of the window
is right but there is that unnecessary scrolling down of the contents.

4) Now I repeat with the forced redisplay even earlier:

(progn
  (switch-to-buffer-other-window "popup")
  (redisplay t)
  (erase-buffer)
  (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
  (fit-window-to-buffer))

This seems to work around the issue.

I believe that in 3 there is some miscalculation during the insertion
of text into the buffer that forces a scroll, but I'm having a hard
time trying to explain that. Anyway, it's another case the
redisplay-before-fit advice can't cope with.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

> Please let me know if there's a policy wrt screencasts,
> even the right ones. I think it could be useful in this case but it's
> not that important.

FWIW here is a permalink (I hope) to the cast:
https://user-images.githubusercontent.com/2845433/137821853-c887649d-848d-4d55-8a1f-299da8020ff4.mov

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > There is, for some value of "display": this function actually
 > generates a glyph row in the glyph matrix.

The same thing happens with 'pos-visible-in-window-p', I suppose.

 >> Eli, I think this could go into Emacs 28.  WDYT?
 >
 > No, it's too radical, and the problem it fixes is too obscure.  Let's
 > install it on master, please.

I'll wait a couple of days and do that if Carlos doesn't see any
problems.

Thanks, martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: carlosjosepita2@gmail.com, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 19 Oct 2021 11:25:09 +0200
> 
>  > There is, for some value of "display": this function actually
>  > generates a glyph row in the glyph matrix.
> 
> The same thing happens with 'pos-visible-in-window-p', I suppose.

If it calls display_mode_line, yes.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

close 38181 29.1
quit

 >  > No, it's too radical, and the problem it fixes is too obscure.  Let's
 >  > install it on master, please.
 >
 > I'll wait a couple of days and do that if Carlos doesn't see any
 > problems.

Done.  Bug closed.

martin

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Martin,

>  > I'll wait a couple of days and do that if Carlos doesn't see any
>  > problems.

sorry I didn't take that couple of days in a literal sense, but was
waiting for the weekend to give it a more thorough test. In any case,
I'll keep you informed of the results.

Thank you very much to both of you for your patience in dealing with
me and for your effort in finding a solution to this problem!

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Just for the record: I've been running emacs-28 with Martin's patch
for a couple of weeks now and I've not seen any issue that may be
related to it. I've not done any tests on master yet.

bug#38181: Actual height of mode-line not taken into account

[martin rudalics]

 > 3) Now I add a redisplay before the fit:
 >
 > (progn
 >    (switch-to-buffer-other-window "popup")
 >    (erase-buffer)
 >    (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
 >    (redisplay t)
 >    (fit-window-to-buffer))
 >
 > Again, only about half of the numbers are visible and the window looks
 > as if its buffer has been scrolled down. But now by scrolling up I can
 > check that all the numbers fit the window. So the height of the window
 > is right but there is that unnecessary scrolling down of the contents.
 >
 > 4) Now I repeat with the forced redisplay even earlier:
 >
 > (progn
 >    (switch-to-buffer-other-window "popup")
 >    (redisplay t)
 >    (erase-buffer)
 >    (insert (mapconcat #'number-to-string (number-sequence 1 30) "\n"))
 >    (fit-window-to-buffer))
 >
 > This seems to work around the issue.
 >
 > I believe that in 3 there is some miscalculation during the insertion
 > of text into the buffer that forces a scroll, but I'm having a hard
 > time trying to explain that. Anyway, it's another case the
 > redisplay-before-fit advice can't cope with.

In (3) the redisplay happens _after_ the insertion so redisplay may have
to scroll the buffer to make point, which is at its maximum, visible.
The subsequent 'fit-window-to-buffer' and the final implicit redisplay
won't change the window's start position after that.  If the window
after the split were large enough to encompass 30 lines, this won't
happen.

In (4) the redisplay happens _before_ the insertion so erasing the
buffer will set 'window-start' to 'point-min' and the implicit redisplay
at the end will see point within the visible portion and not change
'window-start'.

martin

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita2@gmail.com>
> Date: Fri, 15 Oct 2021 02:13:14 -0300
> 
> I'm seeing what I believe is an instance of this issue in a vanilla
> build from emacs-28 branch, ns/cocoa backed.
> 
> Here are some instructions for reproduction:
> https://github.com/seagle0128/doom-modeline/issues/480#issue-1027051204

That bug was about doom-modeline and similar extensions, and it was
never fixed (because we found the solution to be elusive and nowhere
near simple), so it's small wonder the issue still exists.

Is there something new you see that you think will help us in dealing
with this problem?  If so, could you please describe the new
information?

Thanks.

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Eli,

> Is there something new you see that you think will help us in dealing
> with this problem?  If so, could you please describe the new
> information?

I don't know if it will help you dealing with the problem, but I
believe that what I've just answered to Martin is new, indeed the
workaround that was working before carefully deactivated itself after
the first forced redisplay. What I can't see is why after the first
redisplay, if the modeline hasn't changed, the popup will be chopped
like that (a fortiori considering that previously it was correctly
laid out).

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita2@gmail.com>
> Date: Fri, 15 Oct 2021 05:00:19 -0300
> Cc: 38181@debbugs.gnu.org
> 
> I don't know if it will help you dealing with the problem, but I
> believe that what I've just answered to Martin is new, indeed the
> workaround that was working before carefully deactivated itself after
> the first forced redisplay. What I can't see is why after the first
> redisplay, if the modeline hasn't changed, the popup will be chopped
> like that (a fortiori considering that previously it was correctly
> laid out).

Does the recipe involve any change in the appearance of the mode line?
If not, then it's a separate issue.

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Eli,

> Does the recipe involve any change in the appearance of the mode line?
> If not, then it's a separate issue.

Yes, it does, it's step 1 of the recipe.

Best regards,
Carlos

bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita2@gmail.com>
> Date: Fri, 15 Oct 2021 15:33:00 -0300
> Cc: 38181@debbugs.gnu.org
> 
> > Does the recipe involve any change in the appearance of the mode line?
> > If not, then it's a separate issue.
> 
> Yes, it does, it's step 1 of the recipe.

"Step 1" being the original recipe of Jonas?  Then what is new about
this report that Jonas didn't already report and we didn't already
discuss and analyze?

bug#38181: Actual height of mode-line not taken into account

[Carlos Pita]

Hi Eli,

> "Step 1" being the original recipe of Jonas?  Then what is new about
> this report that Jonas didn't already report and we didn't already
> discuss and analyze?

Perhaps the new steps? I will provide my perspective and the formal
procedure to reproduce the issue one last time. Then I let it up to
you to decide if you have already discussed it because the exact
relationship between what I see and what happens under the hood is way
beyond my knowledge. If after that you still feel that I should file a
new ticket, just let me know and I will gladly do it as a mere automaton.

In message #20 you said:

> Why the need to use advising? Your recipe shows that calling
> redisplay before fit-window-to-buffer also solves the problem.  Can't
> you do something like that only when you add such tall images to the
> mode line?

In message #29:

> Code like 'fit-window-to-buffer' works on the text area
> only, it doesn't care whether a scroll bar or mode line changed size
> since last redisplay.

In message #33:

> If this is to make the mode line prettier, then it should be done
> [calling redisplay] once, at the beginning of a session, right?

In message #39 Martin said:

> If we don't want 'fit-window-to-buffer' to do that always we'd need
> some variable, either buffer local or even a window parameter, that
> 'fit-window-to-buffer' would inspect once and reset immediately in
> order to perform only the redisplay call that's really needed.

There are more examples but I hope you get the gist of it. Moreover,
there is the "conventional" workaround [1] which likely was a
consequence of the discussion above. The advice deliberately
deactivates itself after the first redisplay.

So from what I can see there was some consensus regarding that a
single call to redisplay after changing the modeline geometry would
suffice. Now that's exactly what I am bringing into question here. I
reiterate the recipe:

1. Execute the code.
2. Call test-popup and check that the layout is wrong.
3. Redefine test-popup with (redisplay) uncommented.
4. Call it again and check that the layout is right.
5. Redefine test-popup with (redisplay) commented again.
6. Call it again and check that the layout is wrong again.

Specifically, step 6 contradicts the statement that doing a redisplay
after modifying the modeline geometry is enough.

Now you might say: but then why are you reporting this here, it is
not a modeline issue. To what I would answer: you still need step 1
that involves modifying the modeline. And if you then asked: but
what's new wrt what was already discussed? I would answer exactly like
I'm answering now and we would be going around in circles.
I understand the issue is confusing but sadly I don't know enough about
the subject to make it clearer to you. Maybe I'm not saying anything
new about the internals you already know, but from outside it looks
different.


Best regards,
Carlos

---

[1] https://github.com/tarsius/moody/blob/9b679400ca885b8ff51bcfd75b87f79d66c0ee26/moody.el#L303