all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#55696: 28.1; eshell fails to respect text-scale-increase
@ 2022-05-29  3:43 Jeff Kowalski
  2022-06-03  4:41 ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Jeff Kowalski @ 2022-05-29  3:43 UTC (permalink / raw)
  To: 55696

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

To reproduce:

emacs -Q

M-x eshell
ls /etc  # to get a listing of many files

Note at this point, the listing is well-wrapped to the width of the
window.  Now,

M-x text-scale-increase
ls /etc

At this point, the listing is very poorly wrapped.  It appears to ignore
the increased width of the font relative to the width of the window

Thank you for your engagement on this issue, and let me know how I might be
of assistance.
Jeff Kowalski
---------------------------------------------------------
Environment details:
In GNU Emacs 28.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20,
cairo version 1.16.0)
 of 2022-05-16 built on lcy02-amd64-040
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Linux Mint 20.3

Configured using:
 'configure --build=x86_64-linux-gnu --prefix=/usr
 '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
 '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
 --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
 '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
 --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/28.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/28.1/site-lisp:/usr/share/emacs/site-lisp
 --program-suffix=28 --with-modules --with-file-notification=inotify
 --with-mailutils --with-harfbuzz --with-json --with-x=yes
 --with-x-toolkit=gtk3 --with-lcms2 --with-cairo --with-xpm=yes
 --with-gif=yes --with-gnutls=yes --with-jpeg=yes --with-png=yes
 --with-tiff=yes --with-xwidgets 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs28-QewFgN/emacs28-28.1~1.git5a223c7f2e=.
-fstack-protector-strong
 -Wformat -Werror=format-security -no-pie' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro
 -no-pie''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Eshell

Minor modes in effect:
  text-scale-mode: t
  shell-dirtrack-mode: t
  eshell-prompt-mode: t
  eshell-hist-mode: t
  eshell-pred-mode: t
  eshell-cmpl-mode: t
  eshell-proc-mode: t
  eshell-arg-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
time-date subr-x face-remap em-unix em-term term disp-table shell ehelp
em-script em-prompt em-ls cl-loaddefs cl-lib em-hist em-pred em-glob
em-cmpl em-dirs esh-var pcomplete comint ansi-color ring em-basic
em-banner em-alias esh-mode eshell esh-cmd esh-ext esh-opt esh-proc
esh-io esh-arg esh-module esh-groups esh-util seq byte-opt gv bytecomp
byte-compile cconv iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads xwidget-internal dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 74174 11384)
 (symbols 48 8713 1)
 (strings 32 24436 2193)
 (string-bytes 1 817517)
 (vectors 16 17317)
 (vector-slots 8 228302 8608)
 (floats 8 30 59)
 (intervals 56 2258 588)
 (buffers 992 12))

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-05-29  3:43 bug#55696: 28.1; eshell fails to respect text-scale-increase Jeff Kowalski
@ 2022-06-03  4:41 ` Jim Porter
  2022-06-03  6:31   ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-03  4:41 UTC (permalink / raw)
  To: Jeff Kowalski, 55696

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

On 5/28/2022 8:43 PM, Jeff Kowalski wrote:
> To reproduce:
> 
> emacs -Q
> 
> M-x eshell
> ls /etc  # to get a listing of many files
> 
> Note at this point, the listing is well-wrapped to the width of the 
> window.  Now,
> 
> M-x text-scale-increase
> ls /etc
> 
> At this point, the listing is very poorly wrapped.  It appears to ignore 
> the increased width of the font relative to the width of the window

Thanks for the bug report. I think this patch fixes things. I added 
`window-max-lines' as the Y-axis correspondent to the X-axis version 
`window-max-chars-per-line'; maybe there's a function for this already, 
but I didn't see one after a bit of searching.

Note that the values of $LINES and $COLUMNS are sometimes different now, 
even without text-scale-increase. I think this new behavior is more in 
line with what people (and programs) would actually expect; they're the 
sizes that would actually be visible without any scrolling.

However, these differences could be surprising to people. For example, 
in a terminal, $COLUMNS might normally be 80, but within Emacs/Eshell, 
it would be 79 (subtracting one to account for the reserved continuation 
glyph). Maybe we want to do something different so that it remains 80...

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

From 23804c403b5a4ec7ba810a2fa7c1b084682d2cba 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

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

* lisp/eshell/esh-var.el (eshell-variable-aliases-list): Use it (and
'window-max-chars-per-line').

* lisp/eshell/em-ls.el (eshell-ls-find-column-lengths): Use
'window-max-chars-per-line'.

* 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
'window-max-lines'.

* etc/NEWS: Announce addition of 'window-max-lines' (bug#55696).
---
 doc/lispref/windows.texi          | 29 ++++++++++++++++++++++-------
 etc/NEWS                          |  7 +++++++
 lisp/eshell/em-ls.el              |  2 +-
 lisp/eshell/esh-var.el            |  4 ++--
 lisp/window.el                    | 16 ++++++++++++++++
 test/lisp/eshell/esh-var-tests.el | 16 +++++++++-------
 6 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..4d9515c029 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -907,15 +907,15 @@ Window Sizes
 described in the corresponding sections.
 
 If your Lisp program needs to make layout decisions, you will find the
-following function useful:
+following functions useful:
 
 @defun window-max-chars-per-line &optional window face
-This function returns the number of characters displayed in the
-specified face @var{face} in the specified window @var{window} (which
-must be a live window).  If @var{face} was remapped (@pxref{Face
-Remapping}), the information is returned for the remapped face.  If
-omitted or @code{nil}, @var{face} defaults to the default face, and
-@var{window} defaults to the selected window.
+This function returns the number of characters can be displayed in the
+specified face @var{face} in a single line in the specified window
+@var{window} (which must be a live window).  If @var{face} was
+remapped (@pxref{Face Remapping}), the information is returned for the
+remapped face.  If omitted or @code{nil}, @var{face} defaults to the
+default face, and @var{window} defaults to the selected window.
 
 Unlike @code{window-body-width}, this function accounts for the actual
 size of @var{face}'s font, instead of working in units of the canonical
@@ -924,6 +924,21 @@ Window Sizes
 one or both of its fringes.
 @end defun
 
+@defun window-max-lines &optional window face
+This function returns the number of lines that can be displayed at
+once in the specified face @var{face} in the specified window
+@var{window} (which must be a live window).  As with
+@code{window-max-chars-per-line}, if @var{face} was remapped
+(@pxref{Face Remapping}), the information is returned for the remapped
+face.  If omitted or @code{nil}, @var{face} defaults to the default
+face, and @var{window} defaults to the selected window.
+
+Unlike @code{window-body-height}, this function accounts for the
+actual size of @var{face}'s font, instead of working in units of the
+canonical character width of @var{window}'s frame (@pxref{Frame
+Font}).
+@end defun
+
 @cindex fixed-size window
 @vindex window-min-height
 @vindex window-min-width
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..36fc33fd1a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2028,6 +2028,13 @@ The new 'x-show-tooltip-timeout' variable allows the user to alter
 this for packages that don't use 'tooltip-show', but instead call the
 lower level function directly.
 
++++
+** New function 'window-max-lines'.
+This function returns the number of lines that can be displayed at
+once in the specified face and window.  Unlike 'window-body-height',
+it accounts for the actual size of the text, and respect face
+remapping.
+
 ---
 ** New function 'current-cpu-time'.
 It gives access to the CPU time used by the Emacs process, for
diff --git a/lisp/eshell/em-ls.el b/lisp/eshell/em-ls.el
index 874591d250..fcd5f46c18 100644
--- a/lisp/eshell/em-ls.el
+++ b/lisp/eshell/em-ls.el
@@ -845,7 +845,7 @@ eshell-ls-find-column-lengths
            (lambda (file)
              (+ 2 (length (car file))))
 	   files))
-	 (max-width (+ (window-width) 2))
+	 (max-width (+ (window-max-chars-per-line) 2))
 	 col-widths
 	 colw)
 
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 186f6358bc..57db2249fb 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-max-chars-per-line)) t)
+    ("LINES" ,(lambda (_indices) (window-max-lines)) t)
 
     ;; for eshell-cmd.el
     ("_" ,(lambda (indices)
diff --git a/lisp/window.el b/lisp/window.el
index 5da867715f..f62640f209 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -2181,6 +2181,22 @@ window-max-chars-per-line
         ;; truncation glyphs, not one.
 	(1- ncols)))))
 
+(defun window-max-lines (&optional window face)
+  "Return the number of lines that can be displayed at once in WINDOW.
+WINDOW must be a live window and defaults to the selected one.
+
+The character height of FACE is used for the calculation.  If FACE
+is nil or omitted, the default face is used.  If FACE is
+remapped (see `face-remapping-alist'), the function uses the
+remapped face.
+
+This function is different from `window-body-height' in that it
+accounts for the size of the font."
+  (with-selected-window (window-normalize-window window t)
+    (let ((window-height (window-body-height window t))
+	  (font-height (window-font-height window face)))
+      (/ window-height font-height))))
+
 (defun window-current-scroll-bars (&optional window)
   "Return the current scroll bar types for WINDOW.
 WINDOW must be a live window and defaults to the selected one.
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 4e2a18861e..a99624de03 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-max-lines)"
+  (should (equal (eshell-test-command-result "echo $LINES")
+                 (window-max-lines))))
+
+(ert-deftest esh-var-test/columns-var ()
+  "$COLUMNS should equal (window-max-chars-per-line)"
+  (should (equal (eshell-test-command-result "echo $COLUMNS")
+                 (window-max-chars-per-line))))
 
 (ert-deftest esh-var-test/last-result-var ()
   "Test using the \"last result\" ($$) variable"
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-03  4:41 ` Jim Porter
@ 2022-06-03  6:31   ` Eli Zaretskii
  2022-06-03 20:30     ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-03  6:31 UTC (permalink / raw)
  To: Jim Porter; +Cc: 55696, jeff.kowalski

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 2 Jun 2022 21:41:33 -0700
> 
> > M-x text-scale-increase
> > ls /etc
> > 
> > At this point, the listing is very poorly wrapped.  It appears to ignore 
> > the increased width of the font relative to the width of the window
> 
> Thanks for the bug report. I think this patch fixes things. I added 
> `window-max-lines' as the Y-axis correspondent to the X-axis version 
> `window-max-chars-per-line'; maybe there's a function for this already, 
> but I didn't see one after a bit of searching.

Thanks, but I'm not too happy with adding such a function to Emacs.  I
understand why it could make sense for Eshell, or any other package
that uses fixed-size characters, like terminal emulators in term.el.
But in general, it makes no sense in Emacs to ask how many lines of a
given non-default font can fit in a window, because you cannot
guarantee that a single font will be used in the entire window -- it
is enough to have some of the text having the bold or italic face
attribute, or have some non-ASCII characters that will be displayed in
a different font, to disrupt the results.

We could perhaps make window-body-width and window-body-height
optionally pay attention to the remapping of the default face, which
should handle the case such as this one.  Would that be acceptable for
you?

> +  (with-selected-window (window-normalize-window window t)
> +    (let ((window-height (window-body-height window t))
> +	  (font-height (window-font-height window face)))
> +      (/ window-height font-height))))

Is integer division here really TRT?  Shouldn't we round to the
nearest integer instead of truncating?





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-03  6:31   ` Eli Zaretskii
@ 2022-06-03 20:30     ` Jim Porter
  2022-06-04  6:20       ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-03 20:30 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 55696, jeff.kowalski

On 6/2/2022 11:31 PM, Eli Zaretskii wrote:
> Thanks, but I'm not too happy with adding such a function to Emacs.  I
> understand why it could make sense for Eshell, or any other package
> that uses fixed-size characters, like terminal emulators in term.el.
> But in general, it makes no sense in Emacs to ask how many lines of a
> given non-default font can fit in a window, because you cannot
> guarantee that a single font will be used in the entire window -- it
> is enough to have some of the text having the bold or italic face
> attribute, or have some non-ASCII characters that will be displayed in
> a different font, to disrupt the results.

While I don't have a strong opinion that this function should exist, I 
think the scenario where it *could* be useful is to "make layout 
decisions", as the Lisp manual says (for `window-max-chars-per-line'). 
If you were making some UI in a Lisp program that wanted things to fit 
in a window all at once, you'd likely know what face would get used (and 
it might not be the default face). Then, you could use both 
`window-max-chars-per-line' and `window-max-lines' to figure out how 
much space you have.

If that's not convincing, then let's do something more like what you 
suggest below.

> We could perhaps make window-body-width and window-body-height
> optionally pay attention to the remapping of the default face, which
> should handle the case such as this one.  Would that be acceptable for
> you?

That works fine for me. I was actually surprised that these functions 
didn't account for face remapping.

Since window-body-(width|height) are C functions, it would be a bit more 
work to implement this, but I'm sure someone familiar with writing 
C-based Lisp functions wouldn't have much trouble. I could probably 
figure out how to do it myself with a bit of digging too.

>> +  (with-selected-window (window-normalize-window window t)
>> +    (let ((window-height (window-body-height window t))
>> +	  (font-height (window-font-height window face)))
>> +      (/ window-height font-height))))
> 
> Is integer division here really TRT?  Shouldn't we round to the
> nearest integer instead of truncating?

I think integer division is right, but there's an argument either way. I 
see this function as asking the question, "How many lines can fully fit 
in this window without scrolling?" If I had a GUI terminal window open 
and it could fit 20.5 lines, I'd expect the $LINES variable in my shell 
to be 20. That way, a program that consults that variable would know 
that the 21st line is at least partly off-screen.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-03 20:30     ` Jim Porter
@ 2022-06-04  6:20       ` Eli Zaretskii
  2022-06-05  4:49         ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-04  6:20 UTC (permalink / raw)
  To: Jim Porter; +Cc: 55696, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 3 Jun 2022 13:30:01 -0700
> 
> On 6/2/2022 11:31 PM, Eli Zaretskii wrote:
> > Thanks, but I'm not too happy with adding such a function to Emacs.  I
> > understand why it could make sense for Eshell, or any other package
> > that uses fixed-size characters, like terminal emulators in term.el.
> > But in general, it makes no sense in Emacs to ask how many lines of a
> > given non-default font can fit in a window, because you cannot
> > guarantee that a single font will be used in the entire window -- it
> > is enough to have some of the text having the bold or italic face
> > attribute, or have some non-ASCII characters that will be displayed in
> > a different font, to disrupt the results.
> 
> While I don't have a strong opinion that this function should exist, I 
> think the scenario where it *could* be useful is to "make layout 
> decisions", as the Lisp manual says (for `window-max-chars-per-line'). 
> If you were making some UI in a Lisp program that wanted things to fit 
> in a window all at once, you'd likely know what face would get used (and 
> it might not be the default face). Then, you could use both 
> `window-max-chars-per-line' and `window-max-lines' to figure out how 
> much space you have.

Layout decisions for a single line are much simpler, and more probable
to succeed, than similar layout decisions for the entire window.  The
latter are all but impossible to do from Lisp, definitely not with a
simplistic assumption of a single typeface being used for all the text
in the window.  We have window-text-pixel-size for the general case,
for that very reason.  But I think that function doesn't solve your
problem, because you don't have text to measure when you make these
decisions.

> > We could perhaps make window-body-width and window-body-height
> > optionally pay attention to the remapping of the default face, which
> > should handle the case such as this one.  Would that be acceptable for
> > you?
> 
> That works fine for me. I was actually surprised that these functions 
> didn't account for face remapping.

There's no need to take face remapping into account as long as you
measure in units of canonical character width and height.  Face
remapping is local to buffers, so it usually makes no sense to take
the remapping into account by default, because a window can display
any buffer.

> Since window-body-(width|height) are C functions, it would be a bit more 
> work to implement this, but I'm sure someone familiar with writing 
> C-based Lisp functions wouldn't have much trouble. I could probably 
> figure out how to do it myself with a bit of digging too.

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.

> >> +  (with-selected-window (window-normalize-window window t)
> >> +    (let ((window-height (window-body-height window t))
> >> +	  (font-height (window-font-height window face)))
> >> +      (/ window-height font-height))))
> > 
> > Is integer division here really TRT?  Shouldn't we round to the
> > nearest integer instead of truncating?
> 
> I think integer division is right, but there's an argument either way. I 
> see this function as asking the question, "How many lines can fully fit 
> in this window without scrolling?" If I had a GUI terminal window open 
> and it could fit 20.5 lines, I'd expect the $LINES variable in my shell 
> to be 20. That way, a program that consults that variable would know 
> that the 21st line is at least partly off-screen.

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.

Thanks.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-04  6:20       ` Eli Zaretskii
@ 2022-06-05  4:49         ` Jim Porter
  2022-06-05  9:13           ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-05  4:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 55696, jeff.kowalski

[-- 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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-05  4:49         ` Jim Porter
@ 2022-06-05  9:13           ` Eli Zaretskii
  2022-06-05 18:12             ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-05  9:13 UTC (permalink / raw)
  To: Jim Porter, martin rudalics; +Cc: 55696, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 4 Jun 2022 21:49:40 -0700
> 
> 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.

Thanks.

I'd prefer to avoid the renaming of the argument, as there's nothing
wrong with PIXELWISE being not a boolean value.  We have this
elsewhere; e.g., see line-number-display-width.

> > 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.

OK.

> +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.
                      ^^^^^^^^^^^^^^^^^^^
"For any other non-@code{nil} value" avoids potential confusion here.

> +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.

Same here.

> +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);

Our style in C is to leave one space between the name of a symbol and
the following left parenthesis.  So:

  window_body_unit_from_symbol (Lisp_Object unit)

  intern ("remap")

> @@ -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;
> +    }

I think we should optimize the frequent case where
Vface_remapping_alist is nil, in which case BODY_IN_REMAPPED_CHARS is
the same as BODY_IN_CANONICAL_CHARS.  lookup_named_face is not a
trivial function, so it is best to avoid calling it whenever possible.

> +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.

I prefer saying "in units of ..." rather than 'divided by ...".  The
latter is slightly harder to grasp, IME.

>                                                     This means that if
> +a column at the right of the text area is only partially visible, that
> +column is not counted.

I think this sentence doesn't have to be in the doc string; it's
enough to have this explanation in the manual.  (Please make the same
change in other similar places.)

Martin, any further comments?





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-05  9:13           ` Eli Zaretskii
@ 2022-06-05 18:12             ` Jim Porter
  2022-06-05 19:00               ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-05 18:12 UTC (permalink / raw)
  To: Eli Zaretskii, martin rudalics; +Cc: 55696, jeff.kowalski

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

Thanks for taking a look at the patch. Here's a new version with the 
requested changes.

On 6/5/2022 2:13 AM, Eli Zaretskii wrote:
> I'd prefer to avoid the renaming of the argument, as there's nothing
> wrong with PIXELWISE being not a boolean value.  We have this
> elsewhere; e.g., see line-number-display-width.

Ok, I reverted the name to PIXELWISE (though the enum is still named 
`body_unit').

>> +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.
>                        ^^^^^^^^^^^^^^^^^^^
> "For any other non-@code{nil} value" avoids potential confusion here.

Fixed (in both places).

>> +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);
> 
> Our style in C is to leave one space between the name of a symbol and
> the following left parenthesis.

Fixed.

> I think we should optimize the frequent case where
> Vface_remapping_alist is nil, in which case BODY_IN_REMAPPED_CHARS is
> the same as BODY_IN_CANONICAL_CHARS.  lookup_named_face is not a
> trivial function, so it is best to avoid calling it whenever possible.

Good point. How does the updated implementation look?

>> +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.
> 
> I prefer saying "in units of ..." rather than 'divided by ...".  The
> latter is slightly harder to grasp, IME.

Fixed.

>>                                                      This means that if
>> +a column at the right of the text area is only partially visible, that
>> +column is not counted.
> 
> I think this sentence doesn't have to be in the doc string; it's
> enough to have this explanation in the manual.  (Please make the same
> change in other similar places.)

Ok, I removed that bit.

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

From dd206ffec83b392d0672178764dd2aa56bf1073f 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): Make PIXELWISE a 'body_unit'.
(window-body-height, window-body-width): Accept 'remap' for PIXELWISE.
(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 new behavior of
PIXELWISE argument for 'window-body-width' and 'window-body-height'.

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

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..704ed30366 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -829,14 +829,18 @@ Window Sizes
 @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{pixelwise} defines the units to use for the
+height.  If @code{nil}, return the body height 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character height.  For any other non-@code{nil} value, return the
+height in pixels.
 @end defun
 
 @cindex window body width
@@ -857,14 +861,18 @@ Window Sizes
 @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{pixelwise} 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character width.  For any other non-@code{nil} value, return the width
+in pixels.
 @end defun
 
 @cindex window body size
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..79f99dd822 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 PIXELWISE 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..fa0d34389f 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 pixelwise)
 {
   int height = (w->pixel_height
 		- WINDOW_TAB_LINE_HEIGHT (w)
@@ -1029,11 +1038,27 @@ window_body_height (struct window *w, bool pixelwise)
 		- WINDOW_MODE_LINE_HEIGHT (w)
 		- WINDOW_BOTTOM_DIVIDER_WIDTH (w));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (Vface_remapping_alist)
+	{
+	  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;
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_LINE_HEIGHT (WINDOW_XFRAME (w));
+
   /* 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 +1067,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 pixelwise)
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
@@ -1059,24 +1084,45 @@ window_body_width (struct window *w, bool pixelwise)
 		   ? WINDOW_FRINGES_WIDTH (w)
 		   : 0));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (Vface_remapping_alist)
+	{
+	  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;
+	    }
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w));
+
   /* 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.
+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.
 
-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.
+The optional argument PIXELWISE defines the units to use for the
+width.  If nil, return the largest integer smaller than WINDOW's pixel
+width in units of the character width of WINDOW's frame.  If PIXELWISE
+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.
@@ -1084,25 +1130,29 @@ DEFUN ("window-body-width", Fwindow_body_width, Swindow_body_width, 0, 2, 0,
 Also see `window-max-chars-per-line'.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_width (decode_live_window (window),
-					 !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_width (decode_live_window (window),
+			      window_body_unit_from_symbol (pixelwise))));
 }
 
 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.  */)
+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 PIXELWISE defines the units to use for the
+height.  If nil, return the largest integer smaller than WINDOW's
+pixel height in units of the character height of WINDOW's frame.  If
+PIXELWISE 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 pixelwise)
 {
-  return make_fixnum (window_body_height (decode_live_window (window),
-					  !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_height (decode_live_window (window),
+			       window_body_unit_from_symbol (pixelwise))));
 }
 
 DEFUN ("window-old-body-pixel-width",
@@ -2124,7 +2174,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 +3708,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 +3954,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 +4821,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 +5309,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 +5343,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 +6410,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 +6436,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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-05 18:12             ` Jim Porter
@ 2022-06-05 19:00               ` Eli Zaretskii
  2022-06-05 19:59                 ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-05 19:00 UTC (permalink / raw)
  To: Jim Porter; +Cc: rudalics, 55696, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sun, 5 Jun 2022 11:12:50 -0700
> 
> > I think we should optimize the frequent case where
> > Vface_remapping_alist is nil, in which case BODY_IN_REMAPPED_CHARS is
> > the same as BODY_IN_CANONICAL_CHARS.  lookup_named_face is not a
> > trivial function, so it is best to avoid calling it whenever possible.
> 
> Good point. How does the updated implementation look?

Looks okay, except for one thing:

> +  if (pixelwise == BODY_IN_REMAPPED_CHARS)
> +    {
> +      if (Vface_remapping_alist)

This should be

         if (!NILP (Vface_remapping_alist))

because the implementation of nil is not necessarily an integer zero.
You will get compilation errors if you configure with the
"--enable-check-lisp-object-type" option.

I will do a more thorough review tomorrow.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-05 19:00               ` Eli Zaretskii
@ 2022-06-05 19:59                 ` Jim Porter
  2022-06-06 12:43                   ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-05 19:59 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: rudalics, 55696, jeff.kowalski

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

On 6/5/2022 12:00 PM, Eli Zaretskii wrote:
> Looks okay, except for one thing:
> 
>> +  if (pixelwise == BODY_IN_REMAPPED_CHARS)
>> +    {
>> +      if (Vface_remapping_alist)
> 
> This should be
> 
>           if (!NILP (Vface_remapping_alist))

Thanks, fixed.

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

From df01a80a22183edbb9c67864c271095881a6b8b4 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): Make PIXELWISE a 'body_unit'.
(window-body-height, window-body-width): Accept 'remap' for PIXELWISE.
(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 new behavior of
PIXELWISE argument for 'window-body-width' and 'window-body-height'.

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

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..704ed30366 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -829,14 +829,18 @@ Window Sizes
 @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{pixelwise} defines the units to use for the
+height.  If @code{nil}, return the body height 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character height.  For any other non-@code{nil} value, return the
+height in pixels.
 @end defun
 
 @cindex window body width
@@ -857,14 +861,18 @@ Window Sizes
 @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{pixelwise} 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character width.  For any other non-@code{nil} value, return the width
+in pixels.
 @end defun
 
 @cindex window body size
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..79f99dd822 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 PIXELWISE 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..966b0fc490 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 pixelwise)
 {
   int height = (w->pixel_height
 		- WINDOW_TAB_LINE_HEIGHT (w)
@@ -1029,11 +1038,27 @@ window_body_height (struct window *w, bool pixelwise)
 		- WINDOW_MODE_LINE_HEIGHT (w)
 		- WINDOW_BOTTOM_DIVIDER_WIDTH (w));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_LINE_HEIGHT (WINDOW_XFRAME (w));
+
   /* 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 +1067,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 pixelwise)
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
@@ -1059,24 +1084,45 @@ window_body_width (struct window *w, bool pixelwise)
 		   ? WINDOW_FRINGES_WIDTH (w)
 		   : 0));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	    }
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w));
+
   /* 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.
+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.
 
-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.
+The optional argument PIXELWISE defines the units to use for the
+width.  If nil, return the largest integer smaller than WINDOW's pixel
+width in units of the character width of WINDOW's frame.  If PIXELWISE
+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.
@@ -1084,25 +1130,29 @@ DEFUN ("window-body-width", Fwindow_body_width, Swindow_body_width, 0, 2, 0,
 Also see `window-max-chars-per-line'.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_width (decode_live_window (window),
-					 !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_width (decode_live_window (window),
+			      window_body_unit_from_symbol (pixelwise))));
 }
 
 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.  */)
+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 PIXELWISE defines the units to use for the
+height.  If nil, return the largest integer smaller than WINDOW's
+pixel height in units of the character height of WINDOW's frame.  If
+PIXELWISE 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 pixelwise)
 {
-  return make_fixnum (window_body_height (decode_live_window (window),
-					  !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_height (decode_live_window (window),
+			       window_body_unit_from_symbol (pixelwise))));
 }
 
 DEFUN ("window-old-body-pixel-width",
@@ -2124,7 +2174,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 +3708,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 +3954,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 +4821,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 +5309,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 +5343,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 +6410,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 +6436,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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-05 19:59                 ` Jim Porter
@ 2022-06-06 12:43                   ` Eli Zaretskii
  2022-06-07  3:04                     ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-06 12:43 UTC (permalink / raw)
  To: Jim Porter; +Cc: rudalics, 55696, jeff.kowalski

> Cc: rudalics@gmx.at, 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sun, 5 Jun 2022 12:59:22 -0700
> 
> >> +      if (Vface_remapping_alist)
> > 
> > This should be
> > 
> >           if (!NILP (Vface_remapping_alist))
> 
> Thanks, fixed.

A couple more nits:

> +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);
> +}

We already have the Qremap symbol in our sources, so you could use
that instead of calling 'intern' at run time.

> +The optional argument PIXELWISE defines the units to use for the
> +width.  If nil, return the largest integer smaller than WINDOW's pixel
> +width in units of the character width of WINDOW's frame.  If PIXELWISE
> +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.
                     ^^^^^^^^^^^^^^^^^^^
"For any other non-nil value...", like in the manual.

> +The optional argument PIXELWISE defines the units to use for the
> +height.  If nil, return the largest integer smaller than WINDOW's
> +pixel height in units of the character height of WINDOW's frame.  If
> +PIXELWISE 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.  */)

Same here.

I have no further comments; let's wait for Martin to chime in.

Thanks.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-06 12:43                   ` Eli Zaretskii
@ 2022-06-07  3:04                     ` Jim Porter
  2022-06-07 10:55                       ` Eli Zaretskii
  2022-06-08  8:07                       ` martin rudalics
  0 siblings, 2 replies; 23+ messages in thread
From: Jim Porter @ 2022-06-07  3:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: rudalics, 55696, jeff.kowalski

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

On 6/6/2022 5:43 AM, Eli Zaretskii wrote:
> We already have the Qremap symbol in our sources, so you could use
> that instead of calling 'intern' at run time.

Fixed. I didn't realize that the Qfoo names could be used in other 
source files. Good to know.

>> +The optional argument PIXELWISE defines the units to use for the
>> +width.  If nil, return the largest integer smaller than WINDOW's pixel
>> +width in units of the character width of WINDOW's frame.  If PIXELWISE
>> +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.
>                       ^^^^^^^^^^^^^^^^^^^
> "For any other non-nil value...", like in the manual.

Fixed (and in the other function, too).

Thanks again for the reviews.

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

From c7d91d6341d84f59b3981af4d47cb2ff78095a4f 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): Make PIXELWISE a 'body_unit'.
(window-body-height, window-body-width): Accept 'remap' for PIXELWISE.
(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/eshell/em-ls.el (eshell-ls-find-column-widths)
(eshell-ls-find-column-lengths): Use 'window-body-width'.

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

* 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 new behavior of
PIXELWISE argument for 'window-body-width' and 'window-body-height'.

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

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..704ed30366 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -829,14 +829,18 @@ Window Sizes
 @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{pixelwise} defines the units to use for the
+height.  If @code{nil}, return the body height 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character height.  For any other non-@code{nil} value, return the
+height in pixels.
 @end defun
 
 @cindex window body width
@@ -857,14 +861,18 @@ Window Sizes
 @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{pixelwise} 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character width.  For any other non-@code{nil} value, return the width
+in pixels.
 @end defun
 
 @cindex window body size
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..79f99dd822 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 PIXELWISE 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..766b6b8694 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 == Qremap ? 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 pixelwise)
 {
   int height = (w->pixel_height
 		- WINDOW_TAB_LINE_HEIGHT (w)
@@ -1029,11 +1038,27 @@ window_body_height (struct window *w, bool pixelwise)
 		- WINDOW_MODE_LINE_HEIGHT (w)
 		- WINDOW_BOTTOM_DIVIDER_WIDTH (w));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_LINE_HEIGHT (WINDOW_XFRAME (w));
+
   /* 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 +1067,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 pixelwise)
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
@@ -1059,24 +1084,46 @@ window_body_width (struct window *w, bool pixelwise)
 		   ? WINDOW_FRINGES_WIDTH (w)
 		   : 0));
 
+  int denom = 1;
+  if (pixelwise == BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	    }
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w));
+
   /* 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.
+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.
 
-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.
+The optional argument PIXELWISE defines the units to use for the
+width.  If nil, return the largest integer smaller than WINDOW's pixel
+width in units of the character width of WINDOW's frame.  If PIXELWISE
+is `remap' and the default face is remapped (see
+`face-remapping-alist'), use the remapped face to determine the
+character width.  For any other non-nil value, return the width in
+pixels.
 
 Note that the returned value includes the column reserved for the
 continuation glyph.
@@ -1084,25 +1131,29 @@ DEFUN ("window-body-width", Fwindow_body_width, Swindow_body_width, 0, 2, 0,
 Also see `window-max-chars-per-line'.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_width (decode_live_window (window),
-					 !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_width (decode_live_window (window),
+			      window_body_unit_from_symbol (pixelwise))));
 }
 
 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.  */)
+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 PIXELWISE defines the units to use for the
+height.  If nil, return the largest integer smaller than WINDOW's
+pixel height in units of the character height of WINDOW's frame.  If
+PIXELWISE is `remap' and the default face is remapped (see
+`face-remapping-alist'), use the remapped face to determine the
+character height.  For any other non-nil value, return the height in
+pixels.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_height (decode_live_window (window),
-					  !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_height (decode_live_window (window),
+			       window_body_unit_from_symbol (pixelwise))));
 }
 
 DEFUN ("window-old-body-pixel-width",
@@ -2124,7 +2175,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 +3709,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 +3955,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 +4822,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 +5310,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 +5344,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 +6411,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 +6437,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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-07  3:04                     ` Jim Porter
@ 2022-06-07 10:55                       ` Eli Zaretskii
  2022-06-08  8:07                       ` martin rudalics
  1 sibling, 0 replies; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-07 10:55 UTC (permalink / raw)
  To: Jim Porter; +Cc: rudalics, 55696, jeff.kowalski

> Cc: rudalics@gmx.at, 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 6 Jun 2022 20:04:54 -0700
> 
> > We already have the Qremap symbol in our sources, so you could use
> > that instead of calling 'intern' at run time.
> 
> Fixed. I didn't realize that the Qfoo names could be used in other 
> source files. Good to know.

The DEFSYMs are collected by make-docfile and written to globals.h,
where they are accessible to all source files.

> > "For any other non-nil value...", like in the manual.
> 
> Fixed (and in the other function, too).
> 
> Thanks again for the reviews.

Thanks.  Let's wait a bit for Martin to chime in.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-07  3:04                     ` Jim Porter
  2022-06-07 10:55                       ` Eli Zaretskii
@ 2022-06-08  8:07                       ` martin rudalics
  2022-06-08 23:20                         ` Jim Porter
  1 sibling, 1 reply; 23+ messages in thread
From: martin rudalics @ 2022-06-08  8:07 UTC (permalink / raw)
  To: Jim Porter, Eli Zaretskii; +Cc: 55696, jeff.kowalski

 > Fixed (and in the other function, too).

Looks good to me.  In window.h writing

enum window_body_unit
   {
     WINDOW_BODY_IN_CANONICAL_CHARS,
     WINDOW_BODY_IN_PIXELS,
     WINDOW_BODY_IN_REMAPPED_CHARS
   };

might prevent future ambiguities.

martin





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-08  8:07                       ` martin rudalics
@ 2022-06-08 23:20                         ` Jim Porter
  2022-06-09  7:24                           ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-08 23:20 UTC (permalink / raw)
  To: martin rudalics, Eli Zaretskii; +Cc: 55696, jeff.kowalski

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

On 6/8/2022 1:07 AM, martin rudalics wrote:
>  > Fixed (and in the other function, too).
> 
> Looks good to me.  In window.h writing
> 
> enum window_body_unit
>    {
>      WINDOW_BODY_IN_CANONICAL_CHARS,
>      WINDOW_BODY_IN_PIXELS,
>      WINDOW_BODY_IN_REMAPPED_CHARS
>    };
> 
> might prevent future ambiguities.

Ok, done.

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

From 2356f59164607b17c8285a8ec7429a39231febab 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 (window_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): Make PIXELWISE a
'window_body_unit'.
(window-body-height, window-body-width): Accept 'remap' for PIXELWISE.
(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/eshell/em-ls.el (eshell-ls-find-column-widths)
(eshell-ls-find-column-lengths): Use 'window-body-width'.

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

* 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 new behavior of
PIXELWISE argument for 'window-body-width' and 'window-body-height'.

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

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index 0d285b2ad4..704ed30366 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -829,14 +829,18 @@ Window Sizes
 @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{pixelwise} defines the units to use for the
+height.  If @code{nil}, return the body height 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character height.  For any other non-@code{nil} value, return the
+height in pixels.
 @end defun
 
 @cindex window body width
@@ -857,14 +861,18 @@ Window Sizes
 @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{pixelwise} 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{pixelwise} is @code{remap} and the default face is remapped
+(@pxref{Face Remapping}), use the remapped face to determine the
+character width.  For any other non-@code{nil} value, return the width
+in pixels.
 @end defun
 
 @cindex window body size
diff --git a/etc/NEWS b/etc/NEWS
index 3870e937df..79f99dd822 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 PIXELWISE 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..51f6f414de 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, WINDOW_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, WINDOW_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..e5666ce38e 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 window_body_unit
+window_body_unit_from_symbol (Lisp_Object unit)
+{
+  return
+    (unit == Qremap ? WINDOW_BODY_IN_REMAPPED_CHARS
+     : NILP (unit) ? WINDOW_BODY_IN_CANONICAL_CHARS
+     : WINDOW_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 window_body_unit pixelwise)
 {
   int height = (w->pixel_height
 		- WINDOW_TAB_LINE_HEIGHT (w)
@@ -1029,11 +1038,27 @@ window_body_height (struct window *w, bool pixelwise)
 		- WINDOW_MODE_LINE_HEIGHT (w)
 		- WINDOW_BOTTOM_DIVIDER_WIDTH (w));
 
+  int denom = 1;
+  if (pixelwise == WINDOW_BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = WINDOW_BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == WINDOW_BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_LINE_HEIGHT (WINDOW_XFRAME (w));
+
   /* 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 +1067,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 window_body_unit pixelwise)
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
@@ -1059,24 +1084,46 @@ window_body_width (struct window *w, bool pixelwise)
 		   ? WINDOW_FRINGES_WIDTH (w)
 		   : 0));
 
+  int denom = 1;
+  if (pixelwise == WINDOW_BODY_IN_REMAPPED_CHARS)
+    {
+      if (!NILP (Vface_remapping_alist))
+	{
+	  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;
+	    }
+	}
+      /* For performance, use canonical chars if no face remapping.  */
+      else
+	pixelwise = WINDOW_BODY_IN_CANONICAL_CHARS;
+    }
+
+  if (pixelwise == WINDOW_BODY_IN_CANONICAL_CHARS)
+    denom = FRAME_COLUMN_WIDTH (WINDOW_XFRAME (w));
+
   /* 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.
+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.
 
-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.
+The optional argument PIXELWISE defines the units to use for the
+width.  If nil, return the largest integer smaller than WINDOW's pixel
+width in units of the character width of WINDOW's frame.  If PIXELWISE
+is `remap' and the default face is remapped (see
+`face-remapping-alist'), use the remapped face to determine the
+character width.  For any other non-nil value, return the width in
+pixels.
 
 Note that the returned value includes the column reserved for the
 continuation glyph.
@@ -1084,25 +1131,29 @@ DEFUN ("window-body-width", Fwindow_body_width, Swindow_body_width, 0, 2, 0,
 Also see `window-max-chars-per-line'.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_width (decode_live_window (window),
-					 !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_width (decode_live_window (window),
+			      window_body_unit_from_symbol (pixelwise))));
 }
 
 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.  */)
+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 PIXELWISE defines the units to use for the
+height.  If nil, return the largest integer smaller than WINDOW's
+pixel height in units of the character height of WINDOW's frame.  If
+PIXELWISE is `remap' and the default face is remapped (see
+`face-remapping-alist'), use the remapped face to determine the
+character height.  For any other non-nil value, return the height in
+pixels.  */)
   (Lisp_Object window, Lisp_Object pixelwise)
 {
-  return make_fixnum (window_body_height (decode_live_window (window),
-					  !NILP (pixelwise)));
+  return (make_fixnum
+	  (window_body_height (decode_live_window (window),
+			       window_body_unit_from_symbol (pixelwise))));
 }
 
 DEFUN ("window-old-body-pixel-width",
@@ -2124,7 +2175,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, WINDOW_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 +3709,10 @@ 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, WINDOW_BODY_IN_PIXELS);
+	  w->old_body_pixel_height
+	    = window_body_height (w, WINDOW_BODY_IN_PIXELS);
 	}
 
       w = NILP (w->next) ? 0 : XWINDOW (w->next);
@@ -3903,8 +3957,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, WINDOW_BODY_IN_PIXELS)
+		     != w->old_body_pixel_width)
+		 || (window_body_height (w, WINDOW_BODY_IN_PIXELS)
+		     != w->old_body_pixel_height)));
 
 	  /* The following two are needed when running the default
 	     values for this frame below.  */
@@ -4768,7 +4824,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, WINDOW_BODY_IN_PIXELS))
 		     : 0);
 
   new_pixel_size = max (horflag ? size : size - mini_height, unit);
@@ -5255,7 +5312,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, WINDOW_BODY_IN_PIXELS);
   int min_height = FRAME_LINE_HEIGHT (f);
 
   eassert (MINI_WINDOW_P (w));
@@ -5289,7 +5346,8 @@ 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, WINDOW_BODY_IN_PIXELS)
+	       - FRAME_LINE_HEIGHT (f));
 
   eassert (MINI_WINDOW_P (w));
 
@@ -6356,9 +6414,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, WINDOW_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 +6440,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, WINDOW_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..298a80a536 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1186,7 +1186,13 @@ #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 window_body_unit
+  {
+    WINDOW_BODY_IN_CANONICAL_CHARS,
+    WINDOW_BODY_IN_PIXELS,
+    WINDOW_BODY_IN_REMAPPED_CHARS
+  };
+extern int window_body_width (struct window *w, enum window_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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-08 23:20                         ` Jim Porter
@ 2022-06-09  7:24                           ` Eli Zaretskii
  2022-06-09 15:55                             ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-09  7:24 UTC (permalink / raw)
  To: Jim Porter; +Cc: rudalics, 55696-done, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Wed, 8 Jun 2022 16:20:10 -0700
> 
> On 6/8/2022 1:07 AM, martin rudalics wrote:
> >  > Fixed (and in the other function, too).
> > 
> > Looks good to me.  In window.h writing
> > 
> > enum window_body_unit
> >    {
> >      WINDOW_BODY_IN_CANONICAL_CHARS,
> >      WINDOW_BODY_IN_PIXELS,
> >      WINDOW_BODY_IN_REMAPPED_CHARS
> >    };
> > 
> > might prevent future ambiguities.
> 
> Ok, done.

Thanks, installed.

Perhaps unrelated to this, esh-var-tests hangs on my system in two
tests: esh-var-test/interp-cmd-indices and
esh-var-test/quoted-interp-cmd-indices.  Both of those invoke a
command called "list", which exists on my MS-Windows system, but is
probably not what the test expects, so it infloops.

What is that "list" command?  I cannot find it on the GNU/Linux system
to which I have access?  Can we modify these tests not to rely on
non-standard commands?





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-09  7:24                           ` Eli Zaretskii
@ 2022-06-09 15:55                             ` Jim Porter
  2022-06-09 16:48                               ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-09 15:55 UTC (permalink / raw)
  To: 55696, eliz, jeff.kowalski

On 6/9/2022 12:24 AM, Eli Zaretskii wrote:
> Thanks, installed.

Thanks.

> Perhaps unrelated to this, esh-var-tests hangs on my system in two
> tests: esh-var-test/interp-cmd-indices and
> esh-var-test/quoted-interp-cmd-indices.  Both of those invoke a
> command called "list", which exists on my MS-Windows system, but is
> probably not what the test expects, so it infloops.
> 
> What is that "list" command?  I cannot find it on the GNU/Linux system
> to which I have access?  Can we modify these tests not to rely on
> non-standard commands?

That's actually supposed to be the `list' function from Elisp. Since 
there's no such command on GNU/Linux systems, Eshell uses the Elisp 
function of the same name, but as you found, that's not the right thing 
to do in general (some systems may have an external "list" program, in 
which case Eshell would use that instead).

Do the tests work if you replace "list" in those two functions with 
"listify"? (`eshell/listify' is a built-in Eshell alias function, which 
is the highest priority for Eshell commands, so it should always work.)





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-09 15:55                             ` Jim Porter
@ 2022-06-09 16:48                               ` Eli Zaretskii
  2022-06-09 22:14                                 ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-09 16:48 UTC (permalink / raw)
  To: Jim Porter; +Cc: 55696, jeff.kowalski

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 9 Jun 2022 08:55:36 -0700
> 
> Do the tests work if you replace "list" in those two functions with 
> "listify"? (`eshell/listify' is a built-in Eshell alias function, which 
> is the highest priority for Eshell commands, so it should always work.)

If I make this replacement in 4 tests that use "list", then it doesn't
hang, but 2 tests fail:

  Test esh-var-test/interp-lisp-indices backtrace:
    (listify 1 2)
    eval((listify 1 2))
    eshell-exec-lisp(eshell-print eshell-error (listify 1 2) nil t)
    eshell-lisp-command((listify 1 2))
    eval((eshell-lisp-command '(listify 1 2)))
    eshell-do-eval((eshell-lisp-command '(listify 1 2)) t)
    eshell-do-eval((progn (eshell-lisp-command '(listify 1 2)) (symbol-v
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-temp .
    eval((let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-t
    eshell-do-eval((let ((eshell-current-handles '[nil (eshell-temp . 1)
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcall '#f
    eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcal
    eshell-do-eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil
    eshell-do-eval((eshell-apply-indices (let ((value 'eshell-temp) (esh
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((indices '((#("1" 0 1 (number t)))))) (funcall '#f(compiled-fu
    eval((let ((indices '((#("1" 0 1 ...))))) (funcall '#f(compiled-func
    eshell-do-eval((let ((indices '((#("1" 0 1 ...))))) (eshell-apply-in
    eshell-do-eval((eshell-escape-arg (let ((indices '((...)))) (eshell-
    eshell-do-eval((list (eshell-escape-arg (let ((indices '(...))) (esh
    eshell-do-eval((eshell-named-command '"+" (list (eshell-escape-arg (
    eshell-do-eval((prog1 (eshell-named-command '"+" (list (eshell-escap
    (condition-case err (eshell-do-eval '(prog1 (eshell-named-command '"
    eval((condition-case err (eshell-do-eval '(prog1 (eshell-named-comma
    eshell-do-eval((condition-case err (eshell-do-eval '(prog1 (eshell-n
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-this-command-hook '(ignore))) (funcall '#f(compiled-fu
    eval((let ((eshell-this-command-hook '(ignore))) (funcall '#f(compil
    eshell-do-eval((let ((eshell-this-command-hook '(ignore))) (conditio
    eshell-do-eval((progn (let ((eshell-this-command-hook '(ignore))) (c
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-temp .
    eval((let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-t
    eshell-do-eval((let ((eshell-current-handles '[nil (eshell-temp . 1)
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcall '#f
    eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcal
    eshell-do-eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (t . 1) (t . 1)]) (eshell-curren
    eval((let ((eshell-current-handles '[nil (t . 1) (t . 1)]) (eshell-c
    eshell-do-eval((let ((eshell-current-handles '[nil (t . 1) (t . 1)])
    eshell-command-result("+ $(listify 1 2)[1] 3")
    (let ((eshell-history-file-name nil)) (eshell-command-result command
    (progn (let ((eshell-history-file-name nil)) (eshell-command-result 
    (unwind-protect (progn (let ((eshell-history-file-name nil)) (eshell
    (let* ((coding-system-for-write nil) (temp-file (file-name-as-direct
    eshell-test-command-result("+ $(listify 1 2)[1] 3")
    (list (eshell-test-command-result "+ $(listify 1 2)[1] 3") 5)
    (let ((signal-hook-function #'ert--should-signal-hook)) (list (eshel
    (condition-case err (let ((signal-hook-function #'ert--should-signal
    (let* ((fn-140 #'equal) (args-141 (condition-case err (let ((signal-
    (closure (t) nil (let* ((fn-140 #'equal) (args-141 (condition-case e
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name esh-var-test/interp-lisp-indices :doc
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/esh-var-tests
    command-line()
    normal-top-level()
  Test esh-var-test/interp-lisp-indices condition:
      (void-function listify)
     FAILED  20/60  esh-var-test/interp-lisp-indices (0.015625 sec) at lisp/eshell/esh-var-tests.el:140
  [...]
  Test esh-var-test/quoted-interp-lisp-indices backtrace:
    (listify 1 2)
    eval((listify 1 2))
    eshell-exec-lisp(eshell-print eshell-error (listify 1 2) nil t)
    eshell-lisp-command((listify 1 2))
    eval((eshell-lisp-command '(listify 1 2)))
    eshell-do-eval((eshell-lisp-command '(listify 1 2)) t)
    eshell-do-eval((progn (eshell-lisp-command '(listify 1 2)) (symbol-v
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-temp .
    eval((let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-t
    eshell-do-eval((let ((eshell-current-handles '[nil (eshell-temp . 1)
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcall '#f
    eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcal
    eshell-do-eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil
    eshell-do-eval((eshell-apply-indices (let ((value 'eshell-temp) (esh
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((indices '((#("1" 0 1 (number t)))))) (funcall '#f(compiled-fu
    eval((let ((indices '((#("1" 0 1 ...))))) (funcall '#f(compiled-func
    eshell-do-eval((let ((indices '((#("1" 0 1 ...))))) (eshell-apply-in
    eshell-do-eval((eshell-stringify (let ((indices '((...)))) (eshell-a
    eshell-do-eval((eshell-escape-arg (eshell-stringify (let ((indices '
    eshell-do-eval((eshell-escape-arg (eshell-escape-arg (eshell-stringi
    eshell-do-eval((list (eshell-escape-arg (eshell-escape-arg (eshell-s
    eshell-do-eval((eshell-named-command '"concat" (list (eshell-escape-
    eshell-do-eval((prog1 (eshell-named-command '"concat" (list (eshell-
    (condition-case err (eshell-do-eval '(prog1 (eshell-named-command '"
    eval((condition-case err (eshell-do-eval '(prog1 (eshell-named-comma
    eshell-do-eval((condition-case err (eshell-do-eval '(prog1 (eshell-n
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-this-command-hook '(ignore))) (funcall '#f(compiled-fu
    eval((let ((eshell-this-command-hook '(ignore))) (funcall '#f(compil
    eshell-do-eval((let ((eshell-this-command-hook '(ignore))) (conditio
    eshell-do-eval((progn (let ((eshell-this-command-hook '(ignore))) (c
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-temp .
    eval((let ((eshell-current-handles '[nil (eshell-temp . 1) (eshell-t
    eshell-do-eval((let ((eshell-current-handles '[nil (eshell-temp . 1)
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcall '#f
    eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil)) (funcal
    eshell-do-eval((let ((value 'eshell-temp) (eshell-in-pipeline-p 'nil
    #f(compiled-function () #<bytecode -0x12ccc80303566d4e>)()
    funcall(#f(compiled-function () #<bytecode -0x12ccc80303566d4e>))
    (let ((eshell-current-handles '[nil (t . 1) (t . 1)]) (eshell-curren
    eval((let ((eshell-current-handles '[nil (t . 1) (t . 1)]) (eshell-c
    eshell-do-eval((let ((eshell-current-handles '[nil (t . 1) (t . 1)])
    eshell-command-result("concat \"$(listify 1 2)[1]\" cool")
    (let ((eshell-history-file-name nil)) (eshell-command-result command
    (progn (let ((eshell-history-file-name nil)) (eshell-command-result 
    (unwind-protect (progn (let ((eshell-history-file-name nil)) (eshell
    (let* ((coding-system-for-write nil) (temp-file (file-name-as-direct
    eshell-test-command-result("concat \"$(listify 1 2)[1]\" cool")
    (list (eshell-test-command-result "concat \"$(listify 1 2)[1]\" cool
    (let ((signal-hook-function #'ert--should-signal-hook)) (list (eshel
    (condition-case err (let ((signal-hook-function #'ert--should-signal
    (let* ((fn-330 #'equal) (args-331 (condition-case err (let ((signal-
    (closure (t) nil (let* ((fn-330 #'equal) (args-331 (condition-case e
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name esh-var-test/quoted-interp-lisp-indic
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/esh-var-tests
    command-line()
    normal-top-level()
  Test esh-var-test/quoted-interp-lisp-indices condition:
      (void-function listify)
     FAILED  49/60  esh-var-test/quoted-interp-lisp-indices (0.015625 sec) at lisp/eshell/esh-var-tests.el:318





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-09 16:48                               ` Eli Zaretskii
@ 2022-06-09 22:14                                 ` Jim Porter
  2022-06-10  5:52                                   ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-09 22:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 55696, jeff.kowalski

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

On 6/9/2022 9:48 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Thu, 9 Jun 2022 08:55:36 -0700
>>
>> Do the tests work if you replace "list" in those two functions with
>> "listify"? (`eshell/listify' is a built-in Eshell alias function, which
>> is the highest priority for Eshell commands, so it should always work.)
> 
> If I make this replacement in 4 tests that use "list", then it doesn't
> hang, but 2 tests fail:

That makes sense. Those other tests that are now failing should stay 
unchanged. The difference is in the brace style of the commands.

For `esh-var-test/interp-lisp-indices': the expansion is '$(list 1 2)', 
which means "evaluate '(list 1 2)' as a Lisp sexpr". This doesn't need 
to change.

For `esh-var-test/interp-cmd-indices': the expansion is '${list 1 2}',
which means "evaluate 'list 1 2' as a shell-like command". In this case, 
if there's an external program named 'list', it will call that; 
otherwise, it will call the Lisp function `list'. (If there were a Lisp 
function named `eshell/list', Eshell would always prefer that.) Here, we 
should use 'listify', since it's the preferred Eshell way to make a list 
using command-style syntax.

Attached is a patch for this.

[-- Attachment #2: 0001-Don-t-use-list-command-in-Eshell-command-forms.patch --]
[-- Type: text/plain, Size: 1737 bytes --]

From 834cad502107371eea5db3edac0e1c6208fcbfa6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 9 Jun 2022 09:50:20 -0700
Subject: [PATCH] Don't use 'list' command in Eshell command forms

When executed like a command, 'list' looks for external programs named
'list' first before falling back to the Lisp function of the same
name.  This causes unexpected behavior, since the Lisp function is
what we want in these tests.

* test/lisp/eshell/esh-var-tests.el (esh-var-test/interp-cmd-indices)
(esh-var-test/quoted-interp-cmd-indices): Use 'listify' instead of
'list'.
---
 test/lisp/eshell/esh-var-tests.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index bee495eb6e..3180fe7a5f 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -147,7 +147,7 @@ esh-var-test/interp-cmd
 
 (ert-deftest esh-var-test/interp-cmd-indices ()
   "Interpolate command result with index"
-  (should (equal (eshell-test-command-result "+ ${list 1 2}[1] 3") 5)))
+  (should (equal (eshell-test-command-result "+ ${listify 1 2}[1] 3") 5)))
 
 (ert-deftest esh-var-test/interp-cmd-external ()
   "Interpolate command result from external command"
@@ -328,7 +328,8 @@ esh-var-test/quoted-interp-cmd
 
 (ert-deftest esh-var-test/quoted-interp-cmd-indices ()
   "Interpolate command result with index inside double-quotes"
-  (should (equal (eshell-test-command-result "concat \"${list 1 2}[1]\" cool")
+  (should (equal (eshell-test-command-result
+                  "concat \"${listify 1 2}[1]\" cool")
                  "2cool")))
 
 (ert-deftest esh-var-test/quoted-interp-temp-cmd ()
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-09 22:14                                 ` Jim Porter
@ 2022-06-10  5:52                                   ` Eli Zaretskii
  2022-06-10 15:44                                     ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-10  5:52 UTC (permalink / raw)
  To: Jim Porter; +Cc: 55696, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 9 Jun 2022 15:14:22 -0700
> 
> > If I make this replacement in 4 tests that use "list", then it doesn't
> > hang, but 2 tests fail:
> 
> That makes sense. Those other tests that are now failing should stay 
> unchanged. The difference is in the brace style of the commands.
> 
> For `esh-var-test/interp-lisp-indices': the expansion is '$(list 1 2)', 
> which means "evaluate '(list 1 2)' as a Lisp sexpr". This doesn't need 
> to change.
> 
> For `esh-var-test/interp-cmd-indices': the expansion is '${list 1 2}',
> which means "evaluate 'list 1 2' as a shell-like command". In this case, 
> if there's an external program named 'list', it will call that; 
> otherwise, it will call the Lisp function `list'. (If there were a Lisp 
> function named `eshell/list', Eshell would always prefer that.) Here, we 
> should use 'listify', since it's the preferred Eshell way to make a list 
> using command-style syntax.

What if there's an external command named 'listify' -- is there any
chance that the test will invoke it?  If so, I think we should come up
with a safer solution.

Otherwise, feel free to install the change, and thanks.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-10  5:52                                   ` Eli Zaretskii
@ 2022-06-10 15:44                                     ` Jim Porter
  2022-06-10 15:58                                       ` Eli Zaretskii
  0 siblings, 1 reply; 23+ messages in thread
From: Jim Porter @ 2022-06-10 15:44 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 55696, jeff.kowalski

On 6/9/2022 10:52 PM, Eli Zaretskii wrote:
> What if there's an external command named 'listify' -- is there any
> chance that the test will invoke it?  If so, I think we should come up
> with a safer solution.

No, that shouldn't be a problem. When Eshell executes something that 
looks like a shell command[1], the order of precedence is this:

1. If an alias for the command exists in .emacs.d/eshell/aliases, use 
that. (This won't happen in tests, since `eshell-test-command-result' 
binds `eshell-directory-name' to a temp dir.)

2. If there's a Lisp function named `eshell/COMMAND', use that.

3. If there's an external command named `COMMAND', use that.

4. If there's a Lisp function named `COMMAND', use that.

> Otherwise, feel free to install the change, and thanks.

Thanks. (I don't have direct commit access, so hopefully you or someone 
else with commit access can install it when you/they get the chance.)

[1] Something like 'COMMAND args' or 'echo ${COMMAND args}'.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-10 15:44                                     ` Jim Porter
@ 2022-06-10 15:58                                       ` Eli Zaretskii
  2022-06-10 16:44                                         ` Jim Porter
  0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2022-06-10 15:58 UTC (permalink / raw)
  To: Jim Porter; +Cc: 55696, jeff.kowalski

> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 10 Jun 2022 08:44:22 -0700
> 
> On 6/9/2022 10:52 PM, Eli Zaretskii wrote:
> > What if there's an external command named 'listify' -- is there any
> > chance that the test will invoke it?  If so, I think we should come up
> > with a safer solution.
> 
> No, that shouldn't be a problem. When Eshell executes something that 
> looks like a shell command[1], the order of precedence is this:
> 
> 1. If an alias for the command exists in .emacs.d/eshell/aliases, use 
> that. (This won't happen in tests, since `eshell-test-command-result' 
> binds `eshell-directory-name' to a temp dir.)
> 
> 2. If there's a Lisp function named `eshell/COMMAND', use that.
> 
> 3. If there's an external command named `COMMAND', use that.
> 
> 4. If there's a Lisp function named `COMMAND', use that.

So the problem with using 'list' was that there was no Lisp function
eshell/list, is that right?

> > Otherwise, feel free to install the change, and thanks.
> 
> Thanks. (I don't have direct commit access, so hopefully you or someone 
> else with commit access can install it when you/they get the chance.)

Done.





^ permalink raw reply	[flat|nested] 23+ messages in thread

* bug#55696: 28.1; eshell fails to respect text-scale-increase
  2022-06-10 15:58                                       ` Eli Zaretskii
@ 2022-06-10 16:44                                         ` Jim Porter
  0 siblings, 0 replies; 23+ messages in thread
From: Jim Porter @ 2022-06-10 16:44 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 55696, jeff.kowalski

On 6/10/2022 8:58 AM, Eli Zaretskii wrote:
>> Cc: 55696@debbugs.gnu.org, jeff.kowalski@gmail.com
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Fri, 10 Jun 2022 08:44:22 -0700
>>
>> On 6/9/2022 10:52 PM, Eli Zaretskii wrote:
>>> What if there's an external command named 'listify' -- is there any
>>> chance that the test will invoke it?  If so, I think we should come up
>>> with a safer solution.
>>
>> No, that shouldn't be a problem. When Eshell executes something that
>> looks like a shell command[1], the order of precedence is this:
>>
>> 1. If an alias for the command exists in .emacs.d/eshell/aliases, use
>> that. (This won't happen in tests, since `eshell-test-command-result'
>> binds `eshell-directory-name' to a temp dir.)
>>
>> 2. If there's a Lisp function named `eshell/COMMAND', use that.
>>
>> 3. If there's an external command named `COMMAND', use that.
>>
>> 4. If there's a Lisp function named `COMMAND', use that.
> 
> So the problem with using 'list' was that there was no Lisp function
> eshell/list, is that right?

Correct. I think `listify' (aka `eshell/listify') is the preferred 
Eshell way of expressing that for the command-style syntax.

Maybe an `eshell/list' function would be good to have, but I'm not sure 
this problem would come up outside of the unit tests. They're really 
just trying to exercise all the different ways to expand variables, so 
the actual commands aren't very practical.





^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-06-10 16:44 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-29  3:43 bug#55696: 28.1; eshell fails to respect text-scale-increase Jeff Kowalski
2022-06-03  4:41 ` Jim Porter
2022-06-03  6:31   ` Eli Zaretskii
2022-06-03 20:30     ` Jim Porter
2022-06-04  6:20       ` Eli Zaretskii
2022-06-05  4:49         ` Jim Porter
2022-06-05  9:13           ` Eli Zaretskii
2022-06-05 18:12             ` Jim Porter
2022-06-05 19:00               ` Eli Zaretskii
2022-06-05 19:59                 ` Jim Porter
2022-06-06 12:43                   ` Eli Zaretskii
2022-06-07  3:04                     ` Jim Porter
2022-06-07 10:55                       ` Eli Zaretskii
2022-06-08  8:07                       ` martin rudalics
2022-06-08 23:20                         ` Jim Porter
2022-06-09  7:24                           ` Eli Zaretskii
2022-06-09 15:55                             ` Jim Porter
2022-06-09 16:48                               ` Eli Zaretskii
2022-06-09 22:14                                 ` Jim Porter
2022-06-10  5:52                                   ` Eli Zaretskii
2022-06-10 15:44                                     ` Jim Porter
2022-06-10 15:58                                       ` Eli Zaretskii
2022-06-10 16:44                                         ` Jim Porter

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.