bug#60210: 30.0.50; tab-bar height not recalculated when face changes

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gabriel do Nascimento Ribeiro]

Steps:

1) emacs -Q (master "ae91da52335aafaff5405a49c23460082dfb460d")

2) Enable tab-bar:

   (tab-bar-mode +1)

3) Change some face attribute of tab-bar to force increase the tab-bar
height, e.g:

   (set-face-attribute 'tab-bar nil :box 4)
   (set-face-attribute 'tab-bar nil :height 1.2)

Result: the new face is displayed and the tab-bar height is properly
adjusted to reflect the new height.

4) Reset face attributes of tab-bar to force original height, e.g.:

   (set-face-attribute 'tab-bar nil :box nil)
   (set-face-attribute 'tab-bar nil :height 1.0)

Result: the new face is displayed, but the tab-bar height is not
properly adjusted to reflect the new height.

---
Gabriel

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Juri Linkov]

> 2) Enable tab-bar:
>
>    (tab-bar-mode +1)
>
> 3) Change some face attribute of tab-bar to force increase the tab-bar
> height, e.g:
>
>    (set-face-attribute 'tab-bar nil :box 4)
>    (set-face-attribute 'tab-bar nil :height 1.2)
>
> Result: the new face is displayed and the tab-bar height is properly
> adjusted to reflect the new height.
>
> 4) Reset face attributes of tab-bar to force original height, e.g.:
>
>    (set-face-attribute 'tab-bar nil :box nil)
>    (set-face-attribute 'tab-bar nil :height 1.0)
>
> Result: the new face is displayed, but the tab-bar height is not
> properly adjusted to reflect the new height.

It seems this is related to recent bug reports about fonts
such as bug#52493, bug#58912, bug#59271, bug#59285, etc.
since there is no such problem in Emacs 28.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Cc: 60210@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 25 Dec 2022 10:35:10 +0200
> 
> > 2) Enable tab-bar:
> >
> >    (tab-bar-mode +1)
> >
> > 3) Change some face attribute of tab-bar to force increase the tab-bar
> > height, e.g:
> >
> >    (set-face-attribute 'tab-bar nil :box 4)
> >    (set-face-attribute 'tab-bar nil :height 1.2)
> >
> > Result: the new face is displayed and the tab-bar height is properly
> > adjusted to reflect the new height.
> >
> > 4) Reset face attributes of tab-bar to force original height, e.g.:
> >
> >    (set-face-attribute 'tab-bar nil :box nil)
> >    (set-face-attribute 'tab-bar nil :height 1.0)
> >
> > Result: the new face is displayed, but the tab-bar height is not
> > properly adjusted to reflect the new height.
> 
> It seems this is related to recent bug reports about fonts
> such as bug#52493, bug#58912, bug#59271, bug#59285, etc.
> since there is no such problem in Emacs 28.

I doubt the font-related changes are relevant, but bisecting will be
welcome.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

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


>
> I doubt the font-related changes are relevant, but bisecting will be 
> welcome.
>

Indeed, the culprit is 5db4ec20fe.

Patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve-handling-of-tab-bar-height.patch --]
[-- Type: text/x-diff; name=Improve-handling-of-tab-bar-height.patch, Size: 1430 bytes --]

From f2081576513e8b6fba71e67f36622cebe915c4c4 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 26 Dec 2022 00:20:59 +0000
Subject: [PATCH] Improve handling of tab-bar height.

* src/xdisp.c (redisplay_tab_bar): When 'auto-resize-tab-bar' is
not 'grow-only', also consider the case when the tab-bar height
needs to shrink.  Fixes bug#60210.
---
 src/xdisp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index ea2d11e8b4e..c9b3b187fe2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -14271,12 +14271,14 @@ redisplay_tab_bar (struct frame *f)
 	frame_default_tab_bar_height = new_height;
     }
 
-  /* If new_height or new_nrows indicate that we need to enlarge the
-     tab-bar window, we can return right away.  */
+  /* If new_height or new_nrows indicate that we need to enlarge or
+     shrink the tab-bar window, we can return right away.  */
   if (new_nrows > f->n_tab_bar_rows
       || (EQ (Vauto_resize_tab_bars, Qgrow_only)
 	  && !f->minimize_tab_bar_window_p
-	  && new_height > WINDOW_PIXEL_HEIGHT (w)))
+	  && new_height > WINDOW_PIXEL_HEIGHT (w))
+      || (! EQ (Vauto_resize_tab_bars, Qgrow_only)
+	  && new_height < WINDOW_PIXEL_HEIGHT (w)))
     {
       if (FRAME_TERMINAL (f)->change_tab_bar_height_hook)
 	FRAME_TERMINAL (f)->change_tab_bar_height_hook (f, new_height);
-- 
2.35.1

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Date: Mon, 26 Dec 2022 00:24:11 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Juri Linkov <juri@linkov.net>, 60210@debbugs.gnu.org, 
>     gabriel376@hotmail.com
> 
> > I doubt the font-related changes are relevant, but bisecting will be 
> > welcome.
> >
> 
> Indeed, the culprit is 5db4ec20fe.
> 
> Patch attached.

Thanks, this LGTM.  If you tested this with tab bars of more than a
single line, including when tabs are removed so a single line is once
again long enough, please install on the release branch.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>>> I doubt the font-related changes are relevant, but bisecting will be 
>>> welcome.
>>
>> Indeed, the culprit is 5db4ec20fe.
>>
>> Patch attached.
>
> Thanks, this LGTM.  If you tested this with tab bars of more than a 
> single line, including when tabs are removed so a single line is once 
> again long enough, please install on the release branch.
>

I did test it, but I don't use tab bars, so I'm not 110% sure.  Juri, 
could you please test the patch?

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Juri Linkov]

>>>> I doubt the font-related changes are relevant, but bisecting will be
>>>> welcome.
>>>
>>> Indeed, the culprit is 5db4ec20fe.
>>>
>>> Patch attached.
>>
>> Thanks, this LGTM.  If you tested this with tab bars of more than
>> a single line, including when tabs are removed so a single line is once
>> again long enough, please install on the release branch.
>
> I did test it, but I don't use tab bars, so I'm not 110% sure.  Juri, could
> you please test the patch?

Thanks for the patch.  I tested it in emacs-29 with tab-bar-lines > 1
and see no problems.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>>> Thanks, this LGTM.  If you tested this with tab bars of more than a 
>>> single line, including when tabs are removed so a single line is once 
>>> again long enough, please install on the release branch.
>>
>> I did test it, but I don't use tab bars, so I'm not 110% sure.  Juri, 
>> could you please test the patch?
>
> Thanks for the patch.  I tested it in emacs-29 with tab-bar-lines > 1 
> and see no problems.
>

Thanks!  Pushed (b14bbd108e), and closing.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>
> Thanks!  Pushed (b14bbd108e), and closing.
>

I got (privately) an interesting bug report about this commit.  The bug is 
that, because of this change, the tab-bar is not displayed anymore (or is 
displayed but stays empty) in certain circumstances.  It's a font-related 
bug again, so it might be difficult to reproduce.  On my system,

(set-face-attribute 'default nil :font "JetBrains Mono")
(tab-bar-mode 1)

triggers the bug.

The bug is that, in redisplay_tab_bar, WINDOW_PIXEL_HEIGHT (w) uses the 
height of the default face, which is 39 pixels, whereas new_height, which 
is computed with tab_bar_height, uses the font of the tab-bar face 
(variable-pitch in emacs -Q).  On my system, new_height is (with a single 
*scratch* tab) 36 pixels.  Therefore new_height < WINDOW_PIXEL_HEIGHT (w), 
when in fact according to the logic of the code we should have new_height 
== WINDOW_PIXEL_HEIGHT (w).

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Date: Sun, 01 Jan 2023 17:56:36 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60210@debbugs.gnu.org
> 
> The bug is that, in redisplay_tab_bar, WINDOW_PIXEL_HEIGHT (w) uses the 
> height of the default face, which is 39 pixels, whereas new_height, which 
> is computed with tab_bar_height, uses the font of the tab-bar face 
> (variable-pitch in emacs -Q).  On my system, new_height is (with a single 
> *scratch* tab) 36 pixels.  Therefore new_height < WINDOW_PIXEL_HEIGHT (w), 
> when in fact according to the logic of the code we should have new_height 
> == WINDOW_PIXEL_HEIGHT (w).

I'm not sure I understand how the above causes the tab bar not to be
displayed, or become empty.  AFAIU, it just means the frame's
change_tab_bar_height_hook will be called.  What did I miss?

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>> The bug is that, in redisplay_tab_bar, WINDOW_PIXEL_HEIGHT (w) uses the 
>> height of the default face, which is 39 pixels, whereas new_height, 
>> which is computed with tab_bar_height, uses the font of the tab-bar 
>> face (variable-pitch in emacs -Q).  On my system, new_height is (with a 
>> single *scratch* tab) 36 pixels.  Therefore new_height < 
>> WINDOW_PIXEL_HEIGHT (w), when in fact according to the logic of the 
>> code we should have new_height == WINDOW_PIXEL_HEIGHT (w).
>
> I'm not sure I understand how the above causes the tab bar not to be 
> displayed, or become empty.  AFAIU, it just means the frame's 
> change_tab_bar_height_hook will be called.  What did I miss?
>

I do not fully understand it either yet.  A simpler recipe, which does not 
involve changing fonts:

(set-face-attribute 'tab-bar nil :height 0.5)
(tab-bar-mode 1)

This should display a tiny tab-bar, it displays a white bar instead.

In redisplay_tab_bar, new_height is set to half the height of a canonical 
line (17 and 34 on my system).  Therefore redisplay_tab_bar returns true. 
The next redisplay cycle finds that WINDOW_TOTAL_LINES (w) == 0, and does 
nothing.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Date: Sun, 01 Jan 2023 21:50:56 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60210@debbugs.gnu.org, juri@linkov.net
> 
> 
> >> The bug is that, in redisplay_tab_bar, WINDOW_PIXEL_HEIGHT (w) uses the 
> >> height of the default face, which is 39 pixels, whereas new_height, 
> >> which is computed with tab_bar_height, uses the font of the tab-bar 
> >> face (variable-pitch in emacs -Q).  On my system, new_height is (with a 
> >> single *scratch* tab) 36 pixels.  Therefore new_height < 
> >> WINDOW_PIXEL_HEIGHT (w), when in fact according to the logic of the 
> >> code we should have new_height == WINDOW_PIXEL_HEIGHT (w).
> >
> > I'm not sure I understand how the above causes the tab bar not to be 
> > displayed, or become empty.  AFAIU, it just means the frame's 
> > change_tab_bar_height_hook will be called.  What did I miss?
> >
> 
> I do not fully understand it either yet.  A simpler recipe, which does not 
> involve changing fonts:
> 
> (set-face-attribute 'tab-bar nil :height 0.5)
> (tab-bar-mode 1)
> 
> This should display a tiny tab-bar, it displays a white bar instead.
> 
> In redisplay_tab_bar, new_height is set to half the height of a canonical 
> line (17 and 34 on my system).  Therefore redisplay_tab_bar returns true. 
> The next redisplay cycle finds that WINDOW_TOTAL_LINES (w) == 0, and does 
> nothing.

Ah, if WINDOW_TOTAL_LINES becomes zero, that will indeed explain the
problem.  We should prevent that from happening as long as the tab bar
is turned on.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Date: Sun, 01 Jan 2023 21:50:56 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60210@debbugs.gnu.org, juri@linkov.net
> 
> > I'm not sure I understand how the above causes the tab bar not to be 
> > displayed, or become empty.  AFAIU, it just means the frame's 
> > change_tab_bar_height_hook will be called.  What did I miss?
> >
> 
> I do not fully understand it either yet.  A simpler recipe, which does not 
> involve changing fonts:
> 
> (set-face-attribute 'tab-bar nil :height 0.5)
> (tab-bar-mode 1)
> 
> This should display a tiny tab-bar, it displays a white bar instead.
> 
> In redisplay_tab_bar, new_height is set to half the height of a canonical 
> line (17 and 34 on my system).  Therefore redisplay_tab_bar returns true. 
> The next redisplay cycle finds that WINDOW_TOTAL_LINES (w) == 0, and does 
> nothing.

Thanks, I hope I fixed this now on emacs-29, please re-test.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>
> Thanks, I hope I fixed this now on emacs-29, please re-test.
>

Thanks, it works now!  My guess that is was a rounding problem was right. 
But I'm curious: in *_change_tab_bar_height we set f->tab_bar_lines, and 
in redisplay_tab_bar we check w->total_lines.  Where is the former used to 
set the latter?

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Eli Zaretskii]

> Date: Mon, 02 Jan 2023 15:28:33 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60210@debbugs.gnu.org, juri@linkov.net
> 
> > Thanks, I hope I fixed this now on emacs-29, please re-test.
> 
> Thanks, it works now!  My guess that is was a rounding problem was right. 
> But I'm curious: in *_change_tab_bar_height we set f->tab_bar_lines, and 
> in redisplay_tab_bar we check w->total_lines.  Where is the former used to 
> set the latter?

In adjust_frame_size and its subroutines, I guess.

bug#60210: 30.0.50; tab-bar height not recalculated when face changes

[Gregory Heytings]

>>> Thanks, I hope I fixed this now on emacs-29, please re-test.
>>
>> Thanks, it works now!  My guess that is was a rounding problem was 
>> right. But I'm curious: in *_change_tab_bar_height we set 
>> f->tab_bar_lines, and in redisplay_tab_bar we check w->total_lines. 
>> Where is the former used to set the latter?
>
> In adjust_frame_size and its subroutines, I guess.
>

Thanks, I'll have a look.  The person who sent me the bug report confirmed 
that the bug is also fixed with his configuration, so all is well.