bug#55696: 28.1; eshell fails to respect text-scale-increase

[Jim Porter <jporterbugs@gmail.com>]

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

On 6/3/2022 11:20 PM, Eli Zaretskii wrote:
> I suggest that you try, it shouldn't be too hard.  Look at what
> window-font-height does, its main job is done in C as well, so you can
> call those functions directly from C, or do the same "by hand".  Feel
> free to ask questions if something is not clear.

Ok, here's an updated patch. Hopefully I got everything reasonably close 
to correct. I chose to add a new meaning for the PIXELWISE argument to 
`window-body-(width|height)' (and rename it to UNIT), since each of the 
three cases (canonical character size, remapped character size, and 
pixels) are all mutually-exclusive.

>>> Is integer division here really TRT?  Shouldn't we round to the
>>> nearest integer instead of truncating?
[snip]
> 
> My point is that "partly off-screen" might mean 1 or 2 pixels
> off-screen, something that users will usually not even see, because
> those pixels are unused by most glyphs.  So maybe some heuristics is
> more pertinent, like rounding up only when the result is "almost" one
> more line.
> 
> But if you are okay with "losing" a line in these cases, it's fine by
> me.

Since `window-body-(width|height)' round down by default, I think it 
makes sense to do that when those functions account for face remapping 
as well. Either rounding behavior would probably be ok for Eshell, but 
this way seems like the least surprising to users of these functions in 
general.

[-- Attachment #2: 0001-Account-for-remapped-faces-in-COLUMNS-and-LINES-in-E.patch --]
[-- Type: text/plain, Size: 20189 bytes --]

From 5f8a961791a0870e30b503684ca956cc230b492a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 2 Jun 2022 21:12:04 -0700
Subject: [PATCH] Account for remapped faces in $COLUMNS and $LINES in Eshell

* src/window.h (body_unit): New enum...
(window_body_width): ... use it.

* src/window.c (window_body_unit_from_symbol): New function.
(window_body_height, window_body_width): Take a 'body_unit' argument.
(window-body-height, window-body-width): Accept 'remap' as a UNIT.
(window-lines-pixel-dimensions, window_change_record_windows,
run_window_change_functions, resize_frame_windows, grow_mini_window,
shrink_mini_window, scroll-left, scroll-right): Update calls to
'window_body_height' and 'window_body_width'.

* src/indent.c (compute_motion): Update calls to 'window_body_width'.

* lisp/window.el (window-max-lines): New function.

* lisp/eshell/esh-var.el (eshell-variable-aliases-list): Use
'window-body-width' and 'window-body-height'.

* lisp/eshell/em-ls.el (eshell-ls-find-column-widths)
(eshell-ls-find-column-lengths): Use 'window-body-width'.

* test/lisp/eshell/esh-var-tests.el (esh-var-test/window-height)
(esh-var-test/window-width): Rename to...
(esh-var-test/lines-var, esh-var-test/columns-var): ... and update
expected value.

* doc/lispref/windows.texi (Window Sizes): Document UNIT argument for
'window-body-width' and 'window-body-height'.

* etc/NEWS: Announce this change (bug#55696).
---
 doc/lispref/windows.texi          |  40 +++++----
 etc/NEWS                          |   6 ++
 lisp/eshell/em-ls.el              |   4 +-
 lisp/eshell/esh-var.el            |   4 +-
 src/indent.c                      |   4 +-
 src/window.c                      | 139 ++++++++++++++++++++----------
 src/window.h                      |   7 +-
 test/lisp/eshell/esh-var-tests.el |  16 ++--
 8 files changed, 143 insertions(+), 77 deletions(-)

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..e7a594ac41 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -824,19 +824,22 @@ Window Sizes
 does not include any of its top or bottom decorations (@pxref{Basic
 Windows}).
 
-@defun window-body-height &optional window pixelwise
+@defun window-body-height &optional window unit
 This function returns the height, in lines, of the body of window
 @var{window}.  If @var{window} is omitted or @code{nil}, it defaults to
 the selected window; otherwise it must be a live window.
 
-If the optional argument @var{pixelwise} is non-@code{nil}, this
-function returns the body height of @var{window} counted in pixels.
+The optional argument @var{unit} defines the units to use for the
+height.  If @code{nil}, return the body width of @var{window} in
+characters, rounded down to the nearest integer, if necessary.  This
+means that if a line at the bottom of the text area is only partially
+visible, that line is not counted.  It also means that the height of a
+window's body can never exceed its total height as returned by
+@code{window-total-height}.
 
-If @var{pixelwise} is @code{nil}, the return value is rounded down to
-the nearest integer, if necessary.  This means that if a line at the
-bottom of the text area is only partially visible, that line is not
-counted.  It also means that the height of a window's body can never
-exceed its total height as returned by @code{window-total-height}.
+If @var{unit} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character height.  For any other value, return the height in pixels.
 @end defun
 
 @cindex window body width
@@ -852,19 +855,22 @@ Window Sizes
 @code{window-max-chars-per-line}, described below, takes this
 peculiarity into account.)
 
-@defun window-body-width &optional window pixelwise
+@defun window-body-width &optional window unit
 This function returns the width, in columns, of the body of window
 @var{window}.  If @var{window} is omitted or @code{nil}, it defaults to
 the selected window; otherwise it must be a live window.
 
-If the optional argument @var{pixelwise} is non-@code{nil}, this
-function returns the body width of @var{window} in units of pixels.
-
-If @var{pixelwise} is @code{nil}, the return value is rounded down to
-the nearest integer, if necessary.  This means that if a column on the
-right of the text area is only partially visible, that column is not
-counted.  It also means that the width of a window's body can never
-exceed its total width as returned by @code{window-total-width}.
+The optional argument @var{unit} defines the units to use for the
+width.  If @code{nil}, return the body width of @var{window} in
+characters, rounded down to the nearest integer, if necessary.  This
+means that if a column on the right of the text area is only partially
+visible, that column is not counted.  It also means that the width of
+a window's body can never exceed its total width as returned by
+@code{window-total-width}.
+
+If @var{unit} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character width.  For any other value, return the width in pixels.
 @end defun
 
 @cindex window body size
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..10abb3c1b6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2398,6 +2398,12 @@ dimensions.
 Specifying a cons as the FROM argument allows to start measuring text
 from a specified amount of pixels above or below a position.
 
++++
+** 'window-body-width' and 'window-body-height' can use remapped faces.
+Specifying 'remap' as the UNIT argument now checks if the default face
+was remapped, and if so, uses the remapped face to determine the
+character width/height.
+
 +++
 ** 'set-window-vscroll' now accepts a new argument PRESERVE-VSCROLL-P.
 This means the vscroll will not be reset when set on a window that is
diff --git a/lisp/eshell/em-ls.el b/lisp/eshell/em-ls.el
index 874591d250..bebb0d81b5 100644
--- a/lisp/eshell/em-ls.el
+++ b/lisp/eshell/em-ls.el
@@ -800,7 +800,7 @@ eshell-ls-find-column-widths
              (+ 2 (length (car file))))
 	   files))
 	 ;; must account for the added space...
-	 (max-width (+ (window-width) 2))
+	 (max-width (+ (window-body-width nil 'remap) 2))
 	 (best-width 0)
 	 col-widths)
 
@@ -845,7 +845,7 @@ eshell-ls-find-column-lengths
            (lambda (file)
              (+ 2 (length (car file))))
 	   files))
-	 (max-width (+ (window-width) 2))
+	 (max-width (+ (window-body-width nil 'remap) 2))
 	 col-widths
 	 colw)
 
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 186f6358bc..a54bee1a61 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -149,8 +149,8 @@ eshell-variable-name-regexp
 
 (defcustom eshell-variable-aliases-list
   `(;; for eshell.el
-    ("COLUMNS" ,(lambda (_indices) (window-width)) t)
-    ("LINES" ,(lambda (_indices) (window-height)) t)
+    ("COLUMNS" ,(lambda (_indices) (window-body-width nil 'remap)) t)
+    ("LINES" ,(lambda (_indices) (window-body-height nil 'remap)) t)
 
     ;; for eshell-cmd.el
     ("_" ,(lambda (indices)
diff --git a/src/indent.c b/src/indent.c
index acbb9dc972..bda7d4c63f 100644
--- a/src/indent.c
+++ b/src/indent.c
@@ -1204,7 +1204,7 @@ compute_motion (ptrdiff_t from, ptrdiff_t frombyte, EMACS_INT fromvpos,
   /* Negative width means use all available text columns.  */
   if (width < 0)
     {
-      width = window_body_width (win, 0);
+      width = window_body_width (win, BODY_IN_CANONICAL_CHARS);
       /* We must make room for continuation marks if we don't have fringes.  */
 #ifdef HAVE_WINDOW_SYSTEM
       if (!FRAME_WINDOW_P (XFRAME (win->frame)))
@@ -1814,7 +1814,7 @@ DEFUN ("compute-motion", Fcompute_motion, Scompute_motion, 7, 7, 0,
 			 ? window_internal_height (w)
 			 : XFIXNUM (XCDR (topos))),
 			(NILP (topos)
-			 ? (window_body_width (w, 0)
+			 ? (window_body_width (w, BODY_IN_CANONICAL_CHARS)
 			    - (
 #ifdef HAVE_WINDOW_SYSTEM
 			       FRAME_WINDOW_P (XFRAME (w->frame)) ? 0 :
diff --git a/src/window.c b/src/window.c
index eba1390fed..89d891febf 100644
--- a/src/window.c
+++ b/src/window.c
@@ -1014,11 +1014,20 @@ DEFUN ("window-top-line", Fwindow_top_line, Swindow_top_line, 0, 1, 0,
   return make_fixnum (decode_valid_window (window)->top_line);
 }
 
+static enum body_unit
+window_body_unit_from_symbol(Lisp_Object unit)
+{
+  return
+    (unit == intern("remap") ? BODY_IN_REMAPPED_CHARS
+     : NILP (unit) ? BODY_IN_CANONICAL_CHARS
+     : BODY_IN_PIXELS);
+}
+
 /* Return the number of lines/pixels of W's body.  Don't count any mode
    or header line or horizontal divider of W.  Rounds down to nearest
    integer when not working pixelwise. */
 static int
-window_body_height (struct window *w, bool pixelwise)
+window_body_height (struct window *w, enum body_unit unit)
 {
   int height = (w->pixel_height
 		- WINDOW_TAB_LINE_HEIGHT (w)
@@ -1029,11 +1038,22 @@ window_body_height (struct window *w, bool pixelwise)
 		- WINDOW_MODE_LINE_HEIGHT (w)
 		- WINDOW_BOTTOM_DIVIDER_WIDTH (w));
 
+  int denom = 1;
+  if (unit == BODY_IN_CANONICAL_CHARS)
+    {
+      denom = FRAME_LINE_HEIGHT (WINDOW_XFRAME (w));
+    }
+  else if (unit == BODY_IN_REMAPPED_CHARS)
+    {
+      struct frame *f = XFRAME (WINDOW_FRAME (w));
+      int face_id = lookup_named_face (NULL, f, Qdefault, true);
+      struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);
+      if (face && face->font && face->font->height)
+	denom = face->font->height;
+    }
+
   /* Don't return a negative value.  */
-  return max (pixelwise
-	      ? height
-	      : height / FRAME_LINE_HEIGHT (WINDOW_XFRAME (w)),
-	      0);
+  return max (height / denom, 0);
 }
 
 /* Return the number of columns/pixels of W's body.  Don't count columns
@@ -1042,7 +1062,7 @@ window_body_height (struct window *w, bool pixelwise)
    fringes either.  Round down to nearest integer when not working
    pixelwise.  */
 int
-window_body_width (struct window *w, bool pixelwise)
+window_body_width (struct window *w, enum body_unit unit)
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
@@ -1059,50 +1079,71 @@ window_body_width (struct window *w, bool pixelwise)
 		   ? WINDOW_FRINGES_WIDTH (w)
 		   : 0));
 
+  int denom = 1;
+  if (unit == BODY_IN_CANONICAL_CHARS)
+    {
+      denom = FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w));
+    }
+  else if (unit == BODY_IN_REMAPPED_CHARS)
+    {
+      int face_id = lookup_named_face (NULL, f, Qdefault, true);
+      struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);
+      if (face && face->font)
+	{
+	  if (face->font->average_width)
+	    denom = face->font->average_width;
+	  else if (face->font->space_width)
+	    denom = face->font->space_width;
+	}
+    }
+
   /* Don't return a negative value.  */
-  return max (pixelwise
-	      ? width
-	      : width / FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w)),
-	      0);
+  return max (width / denom, 0);
 }
 
 DEFUN ("window-body-width", Fwindow_body_width, Swindow_body_width, 0, 2, 0,
        doc: /* Return the width of WINDOW's text area.
-WINDOW must be a live window and defaults to the selected one.  Optional
-argument PIXELWISE non-nil means return the width in pixels.  The return
-value does not include any vertical dividers, fringes or marginal areas,
-or scroll bars.
-
-If PIXELWISE is nil, return the largest integer smaller than WINDOW's
-pixel width divided by the character width of WINDOW's frame.  This
-means that if a column at the right of the text area is only partially
-visible, that column is not counted.
+WINDOW must be a live window and defaults to the selected one.  The
+return value does not include any vertical dividers, fringes or
+marginal areas, or scroll bars.
+
+The optional argument UNIT defines the units to use for the width.  If
+nil, return the largest integer smaller than WINDOW's pixel width
+divided by the character width of WINDOW's frame.  This means that if
+a column at the right of the text area is only partially visible, that
+column is not counted.  If UNIT is `remap' and the default face is
+remapped (see `face-remapping-alist'), use the remapped face to
+determine the character width.  For any other value, return the width
+in pixels.
 
 Note that the returned value includes the column reserved for the
 continuation glyph.
 
 Also see `window-max-chars-per-line'.  */)
-  (Lisp_Object window, Lisp_Object pixelwise)
+  (Lisp_Object window, Lisp_Object unit)
 {
   return make_fixnum (window_body_width (decode_live_window (window),
-					 !NILP (pixelwise)));
+					 window_body_unit_from_symbol (unit)));
 }
 
 DEFUN ("window-body-height", Fwindow_body_height, Swindow_body_height, 0, 2, 0,
        doc: /* Return the height of WINDOW's text area.
-WINDOW must be a live window and defaults to the selected one.  Optional
-argument PIXELWISE non-nil means return the height of WINDOW's text area
-in pixels.  The return value does not include the mode line or header
-line or any horizontal divider.
-
-If PIXELWISE is nil, return the largest integer smaller than WINDOW's
-pixel height divided by the character height of WINDOW's frame.  This
-means that if a line at the bottom of the text area is only partially
-visible, that line is not counted.  */)
-  (Lisp_Object window, Lisp_Object pixelwise)
+WINDOW must be a live window and defaults to the selected one.  The
+return value does not include the mode line or header line or any
+horizontal divider.
+
+The optional argument UNIT defines the units to use for the height.
+If nil, return the largest integer smaller than WINDOW's pixel height
+divided by the character height of WINDOW's frame.  This means that if
+a line at the bottom of the text area is only partially visible, that
+line is not counted.  If UNIT is `remap' and the default face is
+remapped (see `face-remapping-alist'), use the remapped face to
+determine the character height.  For any other value, return the
+height in pixels.  */)
+  (Lisp_Object window, Lisp_Object unit)
 {
   return make_fixnum (window_body_height (decode_live_window (window),
-					  !NILP (pixelwise)));
+					  window_body_unit_from_symbol (unit)));
 }
 
 DEFUN ("window-old-body-pixel-width",
@@ -2124,7 +2165,8 @@ DEFUN ("window-lines-pixel-dimensions", Fwindow_lines_pixel_dimensions, Swindow_
   struct glyph_row *row, *end_row;
   int max_y = NILP (body) ? WINDOW_PIXEL_HEIGHT (w) : window_text_bottom_y (w);
   Lisp_Object rows = Qnil;
-  int window_width = NILP (body) ? w->pixel_width : window_body_width (w, true);
+  int window_width = NILP (body)
+    ? w->pixel_width : window_body_width (w, BODY_IN_PIXELS);
   int tab_line_height = WINDOW_TAB_LINE_HEIGHT (w);
   int header_line_height = WINDOW_HEADER_LINE_HEIGHT (w);
   int subtract = NILP (body) ? 0 : (tab_line_height + header_line_height);
@@ -3657,8 +3699,8 @@ window_change_record_windows (Lisp_Object window, int stamp, ptrdiff_t number)
 	  wset_old_buffer (w, w->contents);
 	  w->old_pixel_width = w->pixel_width;
 	  w->old_pixel_height = w->pixel_height;
-	  w->old_body_pixel_width = window_body_width (w, true);
-	  w->old_body_pixel_height = window_body_height (w, true);
+	  w->old_body_pixel_width = window_body_width (w, BODY_IN_PIXELS);
+	  w->old_body_pixel_height = window_body_height (w, BODY_IN_PIXELS);
 	}
 
       w = NILP (w->next) ? 0 : XWINDOW (w->next);
@@ -3903,8 +3945,10 @@ run_window_change_functions (void)
 	     && (window_buffer_change
 		 || w->pixel_width != w->old_pixel_width
 		 || w->pixel_height != w->old_pixel_height
-		 || window_body_width (w, true) != w->old_body_pixel_width
-		 || window_body_height (w, true) != w->old_body_pixel_height));
+		 || (window_body_width (w, BODY_IN_PIXELS)
+		     != w->old_body_pixel_width)
+		 || (window_body_height (w, BODY_IN_PIXELS)
+		     != w->old_body_pixel_height)));
 
 	  /* The following two are needed when running the default
 	     values for this frame below.  */
@@ -4768,7 +4812,8 @@ resize_frame_windows (struct frame *f, int size, bool horflag)
   Lisp_Object mini = f->minibuffer_window;
   struct window *m = WINDOWP (mini) ? XWINDOW (mini) : NULL;
   int mini_height = ((FRAME_HAS_MINIBUF_P (f) && !FRAME_MINIBUF_ONLY_P (f))
-		     ? unit + m->pixel_height - window_body_height (m, true)
+		     ? (unit + m->pixel_height
+			- window_body_height (m, BODY_IN_PIXELS))
 		     : 0);
 
   new_pixel_size = max (horflag ? size : size - mini_height, unit);
@@ -5255,7 +5300,7 @@ resize_mini_window_apply (struct window *w, int delta)
 grow_mini_window (struct window *w, int delta)
 {
   struct frame *f = XFRAME (w->frame);
-  int old_height = window_body_height (w, true);
+  int old_height = window_body_height (w, BODY_IN_PIXELS);
   int min_height = FRAME_LINE_HEIGHT (f);
 
   eassert (MINI_WINDOW_P (w));
@@ -5289,7 +5334,7 @@ grow_mini_window (struct window *w, int delta)
 shrink_mini_window (struct window *w)
 {
   struct frame *f = XFRAME (w->frame);
-  int delta = window_body_height (w, true) - FRAME_LINE_HEIGHT (f);
+  int delta = window_body_height (w, BODY_IN_PIXELS) - FRAME_LINE_HEIGHT (f);
 
   eassert (MINI_WINDOW_P (w));
 
@@ -6356,9 +6401,10 @@ DEFUN ("scroll-left", Fscroll_left, Sscroll_left, 0, 2, "^P\np",
   (register Lisp_Object arg, Lisp_Object set_minimum)
 {
   struct window *w = XWINDOW (selected_window);
-  EMACS_INT requested_arg = (NILP (arg)
-			     ? window_body_width (w, 0) - 2
-			     : XFIXNUM (Fprefix_numeric_value (arg)));
+  EMACS_INT requested_arg =
+    (NILP (arg)
+     ? window_body_width (w, BODY_IN_CANONICAL_CHARS) - 2
+     : XFIXNUM (Fprefix_numeric_value (arg)));
   Lisp_Object result = set_window_hscroll (w, w->hscroll + requested_arg);
 
   if (!NILP (set_minimum))
@@ -6381,9 +6427,10 @@ DEFUN ("scroll-right", Fscroll_right, Sscroll_right, 0, 2, "^P\np",
   (register Lisp_Object arg, Lisp_Object set_minimum)
 {
   struct window *w = XWINDOW (selected_window);
-  EMACS_INT requested_arg = (NILP (arg)
-			     ? window_body_width (w, 0) - 2
-			     : XFIXNUM (Fprefix_numeric_value (arg)));
+  EMACS_INT requested_arg =
+    (NILP (arg)
+     ? window_body_width (w, BODY_IN_CANONICAL_CHARS) - 2
+     : XFIXNUM (Fprefix_numeric_value (arg)));
   Lisp_Object result = set_window_hscroll (w, w->hscroll - requested_arg);
 
   if (!NILP (set_minimum))
diff --git a/src/window.h b/src/window.h
index 7f7de58846..32e8b3b978 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1186,7 +1186,12 @@ #define CHECK_LIVE_WINDOW(WINDOW)				\
 extern bool window_wants_header_line (struct window *);
 extern bool window_wants_tab_line (struct window *);
 extern int window_internal_height (struct window *);
-extern int window_body_width (struct window *w, bool);
+enum body_unit {
+  BODY_IN_CANONICAL_CHARS,
+  BODY_IN_PIXELS,
+  BODY_IN_REMAPPED_CHARS
+};
+extern int window_body_width (struct window *w, enum body_unit);
 enum margin_unit { MARGIN_IN_LINES, MARGIN_IN_PIXELS };
 extern int window_scroll_margin (struct window *, enum margin_unit);
 extern void temp_output_buffer_show (Lisp_Object);
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 4e2a18861e..7b8acfcc7d 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -469,13 +469,15 @@ esh-var-test/quoted-interp-convert-cmd-split-indices
 \f
 ;; Built-in variables
 
-(ert-deftest esh-var-test/window-height ()
-  "$LINES should equal (window-height)"
-  (should (eshell-test-command-result "= $LINES (window-height)")))
-
-(ert-deftest esh-var-test/window-width ()
-  "$COLUMNS should equal (window-width)"
-  (should (eshell-test-command-result "= $COLUMNS (window-width)")))
+(ert-deftest esh-var-test/lines-var ()
+  "$LINES should equal (window-body-height nil 'remap)"
+  (should (equal (eshell-test-command-result "echo $LINES")
+                 (window-body-height nil 'remap))))
+
+(ert-deftest esh-var-test/columns-var ()
+  "$COLUMNS should equal (window-body-width nil 'remap)"
+  (should (equal (eshell-test-command-result "echo $COLUMNS")
+                 (window-body-width nil 'remap))))
 
 (ert-deftest esh-var-test/last-result-var ()
   "Test using the \"last result\" ($$) variable"
-- 
2.25.1