bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Gabriel]

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

Description:

The global-mode-string is incorrectly right-aligned in the tab-bar when
global-display-line-numbers-mode is enabled.  The issue actually depends
on the order of execution of global-display-line-numbers-mode.  I could
not find an offending commit, so I am not sure for how long this issue
exists.  I am currently debugging the Emacs internals to find the root
cause.

Steps:

1) emacs -Q (master 690f7ac86ad9a9d714b1107d05c5e856a43bb18d)

2) Eval the following to enable global-display-line-numbers-mode:

(progn
  (global-display-line-numbers-mode 1))

3) Eval the following to enable display-time-mode, which at this moment
will be displayed in the mode-line:

(progn
  (setopt display-time-interval 1
          display-time-string-forms '((format-time-string "%d/%m/%Y %H:%M:%S" now)))
  (display-time-mode 1))

4) Eval the following to add global-mode-line to tab-bar, right-aligned:

(progn
  (setopt tab-bar-format '(tab-bar-format-tabs-groups
                           tab-bar-separator
                           tab-bar-format-align-right
                           tab-bar-format-global))
  (tab-bar-mode 1))

Result: the global-mode-line is displayed in the tab-bar, right-aligned,
as expected.

5) Run step 2) again

Result: the global-mode-line is displayed in the tab-bar, right-aligned,
with an incorrect padding on the right. See attached video.


[-- Attachment #2: bug.mp4 --]
[-- Type: video/mp4, Size: 533604 bytes --]

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


---
Gabriel

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Juri Linkov]

> 2) Eval the following to enable global-display-line-numbers-mode:
>
> (progn
>   (global-display-line-numbers-mode 1))
> ...
> Result: the global-mode-line is displayed in the tab-bar, right-aligned,
> with an incorrect padding on the right. See attached video.

Thanks for the bug report.  Here is a possible fix
to get the correct pixel width without taking into account
line numbers inserted to the text area of the same buffer:

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 6e4d88b4df..92bbdfaa9c 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -322,6 +322,8 @@ string-pixel-width
     ;; Keeping a work buffer around is more efficient than creating a
     ;; new temporary buffer.
     (with-current-buffer (get-buffer-create " *string-pixel-width*")
+      (when display-line-numbers-mode
+        (display-line-numbers-mode -1))
       (delete-region (point-min) (point-max))
       (insert string)
       (car (buffer-text-pixel-size nil nil t)))))

But really I think the bug is in global-display-line-numbers-mode.
Maybe it should not enable display-line-numbers-mode
in internal buffers whose names start with a space?

For example, here is what tab-line-mode--turn-on does:

  (defun tab-line-mode--turn-on ()
    (unless (or (minibufferp)
                (string-match-p "\\` " (buffer-name))
                (memq major-mode tab-line-exclude-modes)
                (get major-mode 'tab-line-exclude)
                (buffer-local-value 'tab-line-exclude (current-buffer)))
      (tab-line-mode 1)))

But display-line-numbers--turn-on is only:

  (defun display-line-numbers--turn-on ()
    (unless (minibufferp)
      (display-line-numbers-mode)))

I think we should add at least (string-match-p "\\` " (buffer-name))
to display-line-numbers--turn-on.

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Eli Zaretskii]

> Cc: 59311@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Fri, 18 Nov 2022 09:15:02 +0200
> 
> > 2) Eval the following to enable global-display-line-numbers-mode:
> >
> > (progn
> >   (global-display-line-numbers-mode 1))
> > ...
> > Result: the global-mode-line is displayed in the tab-bar, right-aligned,
> > with an incorrect padding on the right. See attached video.
> 
> Thanks for the bug report.  Here is a possible fix
> to get the correct pixel width without taking into account
> line numbers inserted to the text area of the same buffer:
> 
> diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
> index 6e4d88b4df..92bbdfaa9c 100644
> --- a/lisp/emacs-lisp/subr-x.el
> +++ b/lisp/emacs-lisp/subr-x.el
> @@ -322,6 +322,8 @@ string-pixel-width
>      ;; Keeping a work buffer around is more efficient than creating a
>      ;; new temporary buffer.
>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
> +      (when display-line-numbers-mode
> +        (display-line-numbers-mode -1))
>        (delete-region (point-min) (point-max))
>        (insert string)
>        (car (buffer-text-pixel-size nil nil t)))))

Yes.  Or subtract what (line-number-display-width t) returns (it will
return zero when line-numbers are turned OFF).

> But really I think the bug is in global-display-line-numbers-mode.
> Maybe it should not enable display-line-numbers-mode
> in internal buffers whose names start with a space?

Regardless, functions that must make sure line-numbers are off should
do that explicitly, or account for the line-number width in their
geometry calculations.  That's because whatever
global-display-line-numbers-mode does, it is always possible that a
particular buffer will have line-numbers enabled for some reason.

> For example, here is what tab-line-mode--turn-on does:
> 
>   (defun tab-line-mode--turn-on ()
>     (unless (or (minibufferp)
>                 (string-match-p "\\` " (buffer-name))
>                 (memq major-mode tab-line-exclude-modes)
>                 (get major-mode 'tab-line-exclude)
>                 (buffer-local-value 'tab-line-exclude (current-buffer)))
>       (tab-line-mode 1)))
> 
> But display-line-numbers--turn-on is only:
> 
>   (defun display-line-numbers--turn-on ()
>     (unless (minibufferp)
>       (display-line-numbers-mode)))
> 
> I think we should add at least (string-match-p "\\` " (buffer-name))
> to display-line-numbers--turn-on.

That's a possibility, but you may find that some Lisp code somewhere
assumes line-numbers are ON in a temporary buffer, e.g. because it
wants to measure something that is directly related to line-numbers.

So this kind of reasoning is a slippery slope, IME, and tends to
introduce subtle bugs elsewhere it takes us years to find.

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Juanma Barranquero]

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

On Fri, Nov 18, 2022 at 9:43 AM Eli Zaretskii <eliz@gnu.org> wrote:

> That's a possibility, but you may find that some Lisp code somewhere
> assumes line-numbers are ON in a temporary buffer, e.g. because it
> wants to measure something that is directly related to line-numbers.

That's kind of unlikely, and if it happens, using your own reasoning that
Lisp code should set line-numbers explicitly.

[-- Attachment #2: Type: text/html, Size: 778 bytes --]

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 18 Nov 2022 10:46:33 +0100
> Cc: Juri Linkov <juri@linkov.net>, 59311@debbugs.gnu.org, gabriel376@hotmail.com
> 
> On Fri, Nov 18, 2022 at 9:43 AM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > That's a possibility, but you may find that some Lisp code somewhere
> > assumes line-numbers are ON in a temporary buffer, e.g. because it
> > wants to measure something that is directly related to line-numbers.
> 
> That's kind of unlikely, and if it happens, using your own reasoning that Lisp code should set line-numbers
> explicitly.

Unlikely or not, string-pixel-with is a function that must not fail
this way.  It must be rock-solid, even if the unthinkable happens.

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Juri Linkov]

>> @@ -322,6 +322,8 @@ string-pixel-width
>>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
>> +      (when display-line-numbers-mode
>> +        (display-line-numbers-mode -1))
>>        (delete-region (point-min) (point-max))
>>        (insert string)
>>        (car (buffer-text-pixel-size nil nil t)))))
>
> Yes.  Or subtract what (line-number-display-width t) returns (it will
> return zero when line-numbers are turned OFF).

Shouldn't then buffer-text-pixel-size subtract line-number-display-width?
Isn't this the responsibility of buffer-text-pixel-size,
not responsibility of a caller like string-pixel-width?

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: gabriel376@hotmail.com,  59311@debbugs.gnu.org
> Date: Sat, 19 Nov 2022 21:03:08 +0200
> 
> >> @@ -322,6 +322,8 @@ string-pixel-width
> >>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
> >> +      (when display-line-numbers-mode
> >> +        (display-line-numbers-mode -1))
> >>        (delete-region (point-min) (point-max))
> >>        (insert string)
> >>        (car (buffer-text-pixel-size nil nil t)))))
> >
> > Yes.  Or subtract what (line-number-display-width t) returns (it will
> > return zero when line-numbers are turned OFF).
> 
> Shouldn't then buffer-text-pixel-size subtract line-number-display-width?
> Isn't this the responsibility of buffer-text-pixel-size,
> not responsibility of a caller like string-pixel-width?

No, because line-numbers take space, and some use cases of
buffer-text-pixel-size want to know that.  Only the caller knows whether the
line-numbers should or shouldn't be included.  The principle is that we
measure the space taken in the text-area, no matter how it is used.  (There
are other display features that affect the result, for example,
line-prefix.)

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Juri Linkov]

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

>> >> @@ -322,6 +322,8 @@ string-pixel-width
>> >>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
>> >> +      (when display-line-numbers-mode
>> >> +        (display-line-numbers-mode -1))
>> >>        (delete-region (point-min) (point-max))
>> >>        (insert string)
>> >>        (car (buffer-text-pixel-size nil nil t)))))
>> >
>> > Yes.  Or subtract what (line-number-display-width t) returns (it will
>> > return zero when line-numbers are turned OFF).
>> 
>> Shouldn't then buffer-text-pixel-size subtract line-number-display-width?
>> Isn't this the responsibility of buffer-text-pixel-size,
>> not responsibility of a caller like string-pixel-width?
>
> No, because line-numbers take space, and some use cases of
> buffer-text-pixel-size want to know that.  Only the caller knows whether the
> line-numbers should or shouldn't be included.  The principle is that we
> measure the space taken in the text-area, no matter how it is used.  (There
> are other display features that affect the result, for example,
> line-prefix.)

I still don't understand why string-pixel-width should handle line-numbers
that also degrades its performance.  I think it should be sufficient only
to disable line-numbers in internal buffers.  But ok, here are both:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: string-pixel-width-line-number.patch --]
[-- Type: text/x-diff, Size: 1017 bytes --]

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 6e4d88b4df..fba817b010 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -324,7 +324,8 @@ string-pixel-width
     (with-current-buffer (get-buffer-create " *string-pixel-width*")
       (delete-region (point-min) (point-max))
       (insert string)
-      (car (buffer-text-pixel-size nil nil t)))))
+      (- (car (buffer-text-pixel-size nil nil t))
+         (line-number-display-width t)))))
 
 ;;;###autoload
 (defun string-glyph-split (string)
diff --git a/lisp/display-line-numbers.el b/lisp/display-line-numbers.el
index 897a88398f..cf5d353fba 100644
--- a/lisp/display-line-numbers.el
+++ b/lisp/display-line-numbers.el
@@ -101,7 +101,8 @@ display-line-numbers-mode
 
 (defun display-line-numbers--turn-on ()
   "Turn on `display-line-numbers-mode'."
-  (unless (minibufferp)
+  (unless (or (minibufferp)
+              (string-match-p "\\` " (buffer-name)))
     (display-line-numbers-mode)))
 
 ;;;###autoload

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: gabriel376@hotmail.com,  59311@debbugs.gnu.org
> Date: Sun, 20 Nov 2022 10:20:40 +0200
> 
> > No, because line-numbers take space, and some use cases of
> > buffer-text-pixel-size want to know that.  Only the caller knows whether the
> > line-numbers should or shouldn't be included.  The principle is that we
> > measure the space taken in the text-area, no matter how it is used.  (There
> > are other display features that affect the result, for example,
> > line-prefix.)
> 
> I still don't understand why string-pixel-width should handle line-numbers
> that also degrades its performance.

Because string-pixel-width uses a temporary buffer as its internal
implementation detail.  (And why do you think this degrades performance? any
measurements?)

> I think it should be sufficient only
> to disable line-numbers in internal buffers.

I think it's too late for that, since display-line-numbers-mode is with us
for the last 2 major releases.

> But ok, here are both:
> 
> diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
> index 6e4d88b4df..fba817b010 100644
> --- a/lisp/emacs-lisp/subr-x.el
> +++ b/lisp/emacs-lisp/subr-x.el
> @@ -324,7 +324,8 @@ string-pixel-width
>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
>        (delete-region (point-min) (point-max))
>        (insert string)
> -      (car (buffer-text-pixel-size nil nil t)))))
> +      (- (car (buffer-text-pixel-size nil nil t))
> +         (line-number-display-width t)))))

This is fine by me.  (Or you could turn off display-line-numbers-mode
instead, right after with-current-buffer.)

>  ;;;###autoload
>  (defun string-glyph-split (string)
> diff --git a/lisp/display-line-numbers.el b/lisp/display-line-numbers.el
> index 897a88398f..cf5d353fba 100644
> --- a/lisp/display-line-numbers.el
> +++ b/lisp/display-line-numbers.el
> @@ -101,7 +101,8 @@ display-line-numbers-mode
>  
>  (defun display-line-numbers--turn-on ()
>    "Turn on `display-line-numbers-mode'."
> -  (unless (minibufferp)
> +  (unless (or (minibufferp)
> +              (string-match-p "\\` " (buffer-name)))
>      (display-line-numbers-mode)))

I don't agree with this part.

Thanks.

bug#59311: 29.0.50; tab-bar global-mode-string affected by global-display-line-numbers

[Juri Linkov]

>> I still don't understand why string-pixel-width should handle line-numbers
>> that also degrades its performance.
>
> Because string-pixel-width uses a temporary buffer as its internal
> implementation detail.  (And why do you think this degrades performance? any
> measurements?)

Any additional code makes it slower.  How much slower is a separate question.

>> I think it should be sufficient only
>> to disable line-numbers in internal buffers.
>
> I think it's too late for that, since display-line-numbers-mode is with us
> for the last 2 major releases.

Leaving it as is means waiting for more trouble in other places.

>> @@ -324,7 +324,8 @@ string-pixel-width
>>      (with-current-buffer (get-buffer-create " *string-pixel-width*")
>>        (delete-region (point-min) (point-max))
>>        (insert string)
>> -      (car (buffer-text-pixel-size nil nil t)))))
>> +      (- (car (buffer-text-pixel-size nil nil t))
>> +         (line-number-display-width t)))))
>
> This is fine by me.  (Or you could turn off display-line-numbers-mode
> instead, right after with-current-buffer.)

Ok, fixed with the latter, and closed.