bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

I noticed the behavior due to company-posframe, which uses the posframe
package to show a completion popup frame.  The frame size is set using
window.el's `fit-frame-to-buffer'.  Some time ago, the call was changed
to provide the explicit frame max-height in terms of lines, which
exposed the following behavior:

If (default-value line-spacing) is the usual 0, the height of the popup
frame is correct, ie, all lines are fully shown.  If, however,
(default-value line-spacing) is 1 or larger (as in my setup), the bottom
of the popup frame cuts off parts of the lowest line.

I do not understand all the details, but it here's what my debugging
shows: 

1) When max-height is provided, the actual frame height is calculated in
ll 8736ff of window.el (as of commit
5746202c182a9c69c732beb29b8507a6e6364799), and that just multiplies by
the char-height, which excludes the line-spacing.  This is the buggy
case.

2) When max-height is NOT provided, the above lines set max-height to
the total (parent frame) height, and the popup frame size is calculated
only when reaching l 8767.  This yields a larger height, correctly
accounting for the line-spacing.

Hope this helps and can be easily fixed.  Thanks!

Ingo


In GNU Emacs 27.0.50 (build 24, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2019-02-16 built on kenko
Repository revision: aff0c585060b7cc92d52a32978c6aa64cf7e2a5e
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux bullseye/sid

Recent messages:
1
Evaluating...
Showing all blocks ... done
nil [3 times]
Evaluating...
Showing all blocks ... done
nil [2 times]
t
Saving file /home/il/Documents/emacs/emacs.org...
Wrote /home/il/Documents/emacs/emacs.org

Configured using:
 'configure --with-imagemagick --without-gsettings --without-gconf
 --with-x-toolkit=lucid --with-modules'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG SOUND GPM DBUS GLIB NOTIFY INOTIFY ACL
LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS PDUMPER GMP

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

Major mode: Org

Minor modes in effect:
  semantic-minor-modes-format: ((:eval (if (or semantic-highlight-edits-mode semantic-show-unmatched-syntax-mode)  S)))
  TeX-PDF-mode: t
  TeX-source-correlate-mode: t
  magit-todos-mode: t
  global-magit-file-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  server-mode: t
  buffer-face-mode: t
  org-indent-mode: t
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  global-flycheck-mode: t
  dired-async-mode: t
  mimuma-global-mode: t
  global-hl-todo-mode: t
  pollen-global-mode: t
  counsel-mode: t
  ivy-mode: t
  amx-mode: t
  minibuffer-depth-indicate-mode: t
  savehist-mode: t
  enchive-mode: t
  xterm-mouse-mode: t
  ws-butler-global-mode: t
  ws-butler-mode: t
  global-auto-revert-mode: t
  my/window-number-mode: t
  desktop-save-mode: t
  company-posframe-mode: t
  company-statistics-mode: t
  global-company-mode: t
  company-mode: t
  show-paren-mode: t
  electric-pair-mode: t
  beginend-global-mode: t
  beginend-org-mode: t
  beginend-outline-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  delete-selection-mode: t
  guide-key-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  global-prettify-symbols-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
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: org-auto-fill-function
  transient-mark-mode: t

Load-path shadows:
[... elided ...]

Features:
(shadow sort emacsbug sendmail mail-extr edebug timezone dired-x
mule-diag ox-org ox-odt ox-latex ox-icalendar ox-html table ox-ascii
ox-publish ox my-grep cl-print ielm quail systemd preview prv-emacs
reftex-dcr reftex-auc reftex-toc-patch reftex-toc reftex-config reftex
reftex-loaddefs reftex-vars tex-buf font-latex latex-config my-fill
latex-addons latex-mode-expansions latex latex-flymake tex-ispell
tex-style tex-config tex tex-mode hippie-expand-config my-mode-groups
hippie-expand-addons my-flex-search hippie-exp pkg-info epl
network-stream cider cider-debug cider-inspector cider-browse-ns
cider-repl-history two-column iso-transl org-attach ess-r-mode
ess-r-flymake ess-r-xref ess-trns ess-r-package ess-r-completion
ess-roxy ess-r-syntax ess-rd ess-s-lang ess-help ess-mode ess-inf
ess-tracebug ess ess-utils ess-custom magit-bookmark bookmark
org-capture cal-move cs-mode cc-langs sql-config sql-addons sql rng-xsd
xsd-regexp rng-cmpct nxml-addons nxml-mode-expansions rng-nxml rng-valid
rng-loc rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn
nxml-ns nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok solar
cal-dst holidays hol-loaddefs cal-iso org-addons window-addons
my-window-funs pulse misearch multi-isearch shell-addons my-completion
magit-extras magit-addons magit-config magit-bundle magit-todos pcre2el
rxt re-builder f wgrep-patch wgrep grep-config grep forge-config
forge-list forge-commands forge-semi forge-bitbucket buck forge-gogs
gogs forge-gitea gtea forge-gitlab glab forge-github ghub-graphql treepy
gsexp ghub let-alist gnutls forge-notify forge-revnote forge-pullreq
forge-issue forge-topic bug-reference forge-post forge-repo forge
forge-core forge-db closql emacsql-sqlite emacsql emacsql-compiler
url-http url-auth url-gw nsm magit-submodule magit-obsolete magit-blame
magit-stash magit-reflog magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-files magit-refs magit-status magit magit-repos-addons tablist
tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet magit-repos
magit-apply magit-wip magit-log which-func magit-diff smerge-config
smerge-mode magit-core magit-autorevert magit-margin magit-transient
magit-process magit-mode transient git-commit magit-git magit-section
magit-utils log-edit pcvs-util add-log with-editor async-bytecomp
hi-lock eieio-opt speedbar sb-image ezimage dframe em-unix em-term term
ehelp em-script em-prompt em-ls em-hist em-pred em-glob em-dirs em-cmpl
em-basic em-banner em-alias windmove tabify server time notmuch-config
notmuch-addons notmuch-patch notmuch hl-line notmuch-hello notmuch-tree
notmuch-show notmuch-print notmuch-crypto notmuch-mua notmuch-message
notmuch-draft notmuch-maildir-fcc notmuch-address notmuch-company
notmuch-parser notmuch-wash coolj notmuch-query icalendar diary-lib
diary-loaddefs notmuch-tag crm notmuch-lib notmuch-version
notmuch-compat cl message rmc rfc822 mml mailabbrev gmm-utils mailheader
mm-view mml-smime mml-sec epa epg smime dig mm-decode mm-bodies
mm-encode mail-parse rfc2231 wconf racket-mode racket-bug-report
racket-collection racket-stepper racket-logger racket-profile
racket-imenu racket-edit racket-complete racket-repl ido racket-common
racket-parens racket-indent racket-font-lock racket-util racket-ppss
racket-keywords-and-builtins racket-custom vimrc-mode ngo make-mode
gitconfig-mode macrostep-c cmacexp my/publish-blog fish-mode mhtml-mode
skewer-html sh-flymake-backends sh-script executable skewer-css
company-skewer skewer-addons skewer-repl skewer-mode cache-table
js2-mode-expansions js2-mode js-config js-patch js-mode-expansions js
cc-mode-expansions cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs simple-httpd css-mode-expansions
css-mode smie html-mode-expansions sgml-mode eww mm-url gnus nnheader
gnus-util rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums mail-utils
cus-edit cus-start cus-load wid-edit url-queue shr text-property-search
puny svg dom conf-mode markdown-mode edit-indirect pyvenv-config pyvenv
esh-var esh-cmd esh-opt esh-io esh-ext esh-proc esh-arg esh-groups
eshell-config eshell esh-module esh-mode esh-util python-config
python-el-fgallina-expansions python tramp-sh geiser-config geiser-mode
geiser-xref geiser-compile geiser-debug geiser-patch geiser-gambit
geiser-chibi geiser-mit geiser-chez geiser-chicken geiser-racket
geiser-guile info-look geiser-repl geiser-image geiser-company
geiser-doc geiser-menu geiser-edit geiser-completion geiser-autodoc
geiser-eval geiser-connection tq geiser-syntax geiser-log geiser-popup
view scheme cider-mode cider-completion cider-profile cider-eval
cider-repl cider-resolve cider-eldoc cider-test cider-stacktrace
cider-doc cider-browse-spec cider-clojuredocs cider-popup cider-overlays
cider-client cider-common cider-util cider-connection sesman-browser
nrepl-client tramp tramp-loaddefs trampver tramp-compat ucs-normalize
parse-time queue nrepl-dict cider-compat spinner parseedn
parseclj-parser parseclj-lex a clojure-mode-expansions sesman
clojure-mode lisp-mnt align org-duration slime-config slime-fancy
slime-trace-dialog slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree slime-scratch slime-presentations bridge
slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-addons slime-repl slime-parse
slime arc-mode archive-mode hyperspec browse-url imenu go-config go-guru
go-flymake-backends my-flymake-addons go-mode derived url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
mailcap find-file ffap face-remap org-indent image-file org-table
disp-table vc-hg vc-src vc-svn my-misc-funs elisp-addons subword-patch
subword-mode-expansions cap-words superword subword goto-addr
idle-highlight-mode rainbow-delimiters hideshow outshine
outshine-org-cmds outorg org-capture-config org-config my-org-contacts
org-protocol org-irc org-info org-id org-docview doc-view jka-compr
image-mode org-bibtex org-element avl-tree bibtex-config
bibtex-clean-addons bibtex ob-scheme geiser-impl help-fns radix-tree
geiser-custom geiser-base geiser ob-dot ob-js ob-sql ob-latex ob-python
ob-shell shell-config shell org-clock org-agenda the-org-mode-expansions
org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-footnote
org-src ob-comint ob-keys org-pcomplete pcomplete org-list org-faces
org-entities time-date org-version ob-emacs-lisp ob-core ob-eval
org-compat org-macs org-loaddefs format-spec calendar-config cal-menu
calendar cal-loaddefs noutline outline vc-git diff-mode flycheck-config
flycheck faces-config ui-config comint-config dired-guess dired-subtree
dired-hacks-utils dired-async async dired-aux dired-config
modeline-config dired-patch ls-lisp-config ls-lisp-patch ls-lisp
mini-multi-major xref-addons my-eval-result-overlays my-solarized-colors
hl-todo flymake-config help-at-pt eglot-config eglot-patch eglot jsonrpc
array ert pp ewoc debug backtrace subr-x flymake-proc flymake warnings
url-util pollen compile-config compile-addons locals-patch my-project vc
vc-dispatcher counsel-addons counsel xdg dired dired-loaddefs compile
comint ansi-color swiper-addons swiper ivy-config ivy-hydra ivy flx
colir color ivy-overlay amx mb-depth savehist secrets dbus xml
enchive-mode xt-mouse ws-butler autorevert filenotify my-file-funs
desktop-config my-window-numbers display-actions desktop frameset
company-posframe posframe company-statistics company-keywords
company-dabbrev-code company-dabbrev etags-addons company-etags etags
fileloop generator xref project company-gtags company-template
company-capf company-elisp find-func company-config company sexp-addons
pcase paren elec-pair occur-config my-aux-funs easy-mmode beginend
ace-link avy multiple-cursors mc-hide-unmatched-lines-mode
mc-separate-operations rectangular-region-mode mc-mark-pop mc-mark-more
mc-cycle-cursors mc-edit-lines multiple-cursors-core rect expand-region
text-mode-expansions er-basic-expansions thingatpt expand-region-core
expand-region-custom undo-tree-patch undo-tree diff delsel indent-config
indent-addons mm-util mail-prsvr diminish guide-key advice s popwin dash
my-loaddefs edmacro kmacro cl-extra help-mode hydra ring lv
my-setup-funs mule-util info tex-site slime-autoloads rx package
easymenu epg-config url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache json map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type 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 elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame 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 charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify dynamic-setting font-render-setting
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 5731969 544737)
 (symbols 48 74465 205)
 (strings 32 351286 127943)
 (string-bytes 1 10253755)
 (vectors 16 264560)
 (vector-slots 8 5833430 221798)
 (floats 8 1203 4037)
 (intervals 56 781876 5606)
 (buffers 992 301))

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

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

 > 1) When max-height is provided, the actual frame height is calculated in
 > ll 8736ff of window.el (as of commit
 > 5746202c182a9c69c732beb29b8507a6e6364799), and that just multiplies by
 > the char-height, which excludes the line-spacing.  This is the buggy
 > case.

I attached a fix.  Please try it.

Many thanks for the report, martin

[-- Attachment #2: fit-frame-to-buffer.diff --]
[-- Type: text/plain, Size: 3239 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index cf733153b8..3df463533c 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8494,6 +8494,40 @@ window-buffer-height
 			     (eobp)
 			     window))))
 
+(defun window-char-height (&optional window)
+  "Return number of pixels of the height of a character in WINDOW.
+WINDOW must be a live window and defaults to the selected one.
+The return value accounts for any remapping of the default face
+of WINDOW's frame."
+  ;; Code mostly stolen from simple.el's `default-font-height'.
+  (let* ((window (window-normalize-window window t))
+	 (frame (window-frame window))
+	 (default-font (face-font 'default frame)))
+    (cond
+     ((and (display-multi-font-p (frame-parameter frame 'display))
+	   (not (string-equal (frame-parameter frame 'font) default-font)))
+      (aref (font-info default-font frame) 3))
+     (t (frame-char-height frame)))))
+
+(defun window-line-height (&optional window)
+  "Return number of pixels of a text line in WINDOW.
+WINDOW must be a live window and defaults to the selected one.
+The return value includes any line spacing defined for WINDOW's
+buffer or frame."
+  ;; Code mostly stolen from simple.el's `default-line-height'.
+  (let* ((window (window-normalize-window window t))
+	 (char-height (window-char-height window))
+	 (buffer (window-buffer window))
+	 (frame (window-frame window))
+	 (space-height
+	  (or (and (display-graphic-p)
+		   (or (buffer-local-value 'line-spacing buffer)
+		       (frame-parameter frame 'line-spacing)))
+	      0)))
+    (when (floatp space-height)
+      (setq space-height (truncate (* char-height space-height))))
+    (+ char-height space-height)))
+
 ;;; Resizing windows and frames to fit their contents exactly.
 (defcustom fit-window-to-buffer-horizontally nil
   "Non-nil means `fit-window-to-buffer' can resize windows horizontally.
@@ -8636,6 +8670,7 @@ fit-frame-to-buffer
            (char-height (frame-char-height frame))
            ;; WINDOW is FRAME's root window.
            (window (frame-root-window frame))
+           (line-height (window-line-height window))
            (parent (frame-parent frame))
            (monitor-attributes
             (unless parent
@@ -8732,16 +8767,16 @@ fit-frame-to-buffer
            (max-height
             (min
              (cond
-              ((numberp max-height) (* max-height char-height))
-              ((numberp (nth 0 sizes)) (* (nth 0 sizes) char-height))
+              ((numberp max-height) (* max-height line-height))
+              ((numberp (nth 0 sizes)) (* (nth 0 sizes) line-height))
               (t parent-or-display-height))
              ;; The following is the maximum height that fits into the
              ;; top and bottom margins.
              (max (- bottom-margin top-margin outer-minus-body-height))))
            (min-height
             (cond
-             ((numberp min-height) (* min-height char-height))
-             ((numberp (nth 1 sizes)) (* (nth 1 sizes) char-height))
+             ((numberp min-height) (* min-height line-height))
+             ((numberp (nth 1 sizes)) (* (nth 1 sizes) line-height))
              (t (window-min-size window nil nil t))))
            (max-width
             (min

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > thanks for the quick reply!  The fix is working for me.  The separate
 > window-*-height functions are much better than what I sent a few
 > minutes ago, I missed all kinds of scenarios, of course.

For consistency, I would use these functions in 'fit-window-to-buffer'
as well.  But I haven't looked into all consequences yet.  There's
also Bug#14825 still sitting around the corner, awaiting a proper
solution.  It's somehow troubling that all these substitute canonical
character height with real line height fixes are inherently backward
incompatible.  What if someone did mean to use the canonical character
height there?

 > There's one thing from my patch, however, that I think is missing in
 > yours:

I think you're right but I need to see your patch first.  It's not
here yet.

 > In ll 8831ff, the height is rounded if frame-resize-pixelwise is
 > nil.  That also uses char-height, and I think it should be line-height
 > there as well.  In that case, char-height is no longer used in the
 > function.

Which certainly would be an asset.

Thanks, martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Tue, Oct 01 2019 10:10 (+0200), martin rudalics wrote:

>  > thanks for the quick reply!  The fix is working for me.  The separate
>  > window-*-height functions are much better than what I sent a few
>  > minutes ago, I missed all kinds of scenarios, of course.
>
> For consistency, I would use these functions in 'fit-window-to-buffer'
> as well.  But I haven't looked into all consequences yet.  There's
> also Bug#14825 still sitting around the corner, awaiting a proper
> solution.  It's somehow troubling that all these substitute canonical
> character height with real line height fixes are inherently backward
> incompatible.  What if someone did mean to use the canonical character
> height there?

I surely find the complexity of all this code jarring, so I have to
restrict myself to looking at a single issue; and here, it's clearly
fixing a bug, using the char height is simply wrong, IMO.

>
>  > There's one thing from my patch, however, that I think is missing in
>  > yours:
>
> I think you're right but I need to see your patch first.  It's not
> here yet.

Debbugs and the mailing list interaction is another thing I do not
really understand ;)  In any case, the patch would simply be confusing
now, here's the change on top of yours:

@@ -8794,8 +8828,8 @@ fit-frame-to-buffer
       ;; Fit height to constraints.
       (when height
         (unless frame-resize-pixelwise
-          (setq height (* (/ (+ height char-height -1) char-height)
-                          char-height)))
+          (setq height (* (/ (+ height line-height -1) line-height)
+                          line-height)))
         ;; The new outer height.
         (setq outer-height (+ height outer-minus-body-height))
         ;; Preserve margins.

And then char-height can be dropped.

Best,
Ingo

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > I surely find the complexity of all this code jarring, so I have to
 > restrict myself to looking at a single issue; and here, it's clearly
 > fixing a bug, using the char height is simply wrong, IMO.

Agreed.  But if I change 'fit-frame-to-buffer', then, for consistency,
I have to at least change 'fit-window-to-buffer' too.

 > +          (setq height (* (/ (+ height line-height -1) line-height)
 > +                          line-height)))
[...]
 > And then char-height can be dropped.

Right.

Thanks, martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > Agreed.  But if I change 'fit-frame-to-buffer', then, for consistency,
 > I have to at least change 'fit-window-to-buffer' too.
 >
 >  > +          (setq height (* (/ (+ height line-height -1) line-height)
 >  > +                          line-height)))
 > [...]
 >  > And then char-height can be dropped.
 >
 > Right.

Hmm...  Back to the roots, unfortunately.

When we are here, 'height' is the calculated height the window should
have in pixels.  When we want to communicate this value to the window
manager and 'frame-resize-pixelwise' is nil, we have to transform this
value (which already includes the pixels needed for line spacing) to a
multiple of the canonical character height of the frame and not the
line height we calculated earlier.  So using 'line-height' here is not
the TRT unless I'm missing something.  WDYT?

martin

PS: My 'window-line-height' must be renamed to not clash with the
homonymous function in window.c.  It will probably become something
like 'window-default-line-height' unless I manage to merge it into
'default-line-height' itself.

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Thu, Oct 03 2019 10:15 (+0200), martin rudalics wrote:

>  > Agreed.  But if I change 'fit-frame-to-buffer', then, for consistency,
>  > I have to at least change 'fit-window-to-buffer' too.
>  >
>  >  > +          (setq height (* (/ (+ height line-height -1) line-height)
>  >  > +                          line-height)))
>  > [...]
>  >  > And then char-height can be dropped.
>  >
>  > Right.
>
> Hmm...  Back to the roots, unfortunately.
>
> When we are here, 'height' is the calculated height the window should
> have in pixels.  When we want to communicate this value to the window
> manager and 'frame-resize-pixelwise' is nil, we have to transform this
> value (which already includes the pixels needed for line spacing) to a
> multiple of the canonical character height of the frame and not the
> line height we calculated earlier.  So using 'line-height' here is not
> the TRT unless I'm missing something.  WDYT?

Well, I don't really know :) I'm not totally sure about the meaning of
`frame-resize-pixelwise'.

1) I think you're right in the sense that rounding to the line height
(not the canonical char height) is NOT what the docstring says it does.
That's bad and should be fixed either way.

2) With the above code, rounding window height (incl line spacing) to a
multiple of the line height (incl line spacing), I do not see any effect
of the option value; because it does not change the height value at all
in my test cases (small popups).

3) BUT that's not generally true, IMO: If the height is restricted by
the screen, or the margin calculation changes it, the rounding will have
an effect.

4) *My interpretation* of `frame-resize-pixelwise' is that the default
value, nil, has a single intention: To make the frame height an exact
multiple of lines (and char width), mainly because of aesthetic reasons.
In that case, I suggest we change the doc-string (which has some
inaccurate phrasing and is a bit wordy, anyway) to say that it nil will
round to a multiple of the default line height (incl line spacing)..

Very briefly browsing the C code using the option seems to confirm that
interpretation.

So I think the code snippet as above is correct (using the line-height).
I will try to come up with an updated docstring for
`frame-resize-pixelwise', if you don't mind.  As for the consistency
changes that you mention, that sounds fine with me, you know the
relevant much(!) better than I do.

Ingo

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > 4) *My interpretation* of `frame-resize-pixelwise' is that the default
 > value, nil, has a single intention: To make the frame height an exact
 > multiple of lines (and char width), mainly because of aesthetic reasons.
 > In that case, I suggest we change the doc-string (which has some
 > inaccurate phrasing and is a bit wordy, anyway) to say that it nil will
 > round to a multiple of the default line height (incl line spacing)..

We cannot reliably do that.  Suppose a frame has two windows with
different line heights.  Which one would we choose for rounding the
frame size?  'frame-char-height' is canonical.

martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Thu, Oct 03 2019 20:10 (+0200), martin rudalics wrote:

>  > 4) *My interpretation* of `frame-resize-pixelwise' is that the default
>  > value, nil, has a single intention: To make the frame height an exact
>  > multiple of lines (and char width), mainly because of aesthetic reasons.
>  > In that case, I suggest we change the doc-string (which has some
>  > inaccurate phrasing and is a bit wordy, anyway) to say that it nil will
>  > round to a multiple of the default line height (incl line spacing)..
>
> We cannot reliably do that.  Suppose a frame has two windows with
> different line heights.  Which one would we choose for rounding the
> frame size?  'frame-char-height' is canonical.
>
> martin

Okay, then I truly don't know how to write that succinctly.  I suggest
to keep using the char-height (and the `frame-resize-pixelwise'
docstring) for the time being, that is, not change the code in the last
part of the function.

Personally, I will simple set `frame-resize-pixelwise' to t.

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > Okay, then I truly don't know how to write that succinctly.  I suggest
 > to keep using the char-height (and the `frame-resize-pixelwise'
 > docstring) for the time being, that is, not change the code in the last
 > part of the function.

If I don't use 'frame-char-height' here, any subsequent resize
requests for that frame (including resizing the frame by dragging its
boder with the mouse) might use the line height value.  In general,
this is certainly not TRT, for example, when another buffer gets
displayed in that frame's window.  That's also why I'm still reluctant
to use the line height concept more pervasively.

The particular problem could be resolved by adding a
'resize-pixelwise' frame parameter whose value, when set, would
override that of 'frame-resize-pixelwise'.  Provided your frame is
in some sense private to 'company-posframe'.

 > Personally, I will simple set `frame-resize-pixelwise' to t.

I don't think, however, that this problem is grave enough to warrant a
general recommendation.  Adding all sorts of window and frame
decorations to the value returned by 'window-text-pixel-size' will
often add some slack whitespace when 'frame-resize-pixelwise' is nil,
regardless of whether we round by character or line height.

martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Sat, Oct 05 2019 10:41 (+0200), martin rudalics wrote:

>  > Okay, then I truly don't know how to write that succinctly.  I suggest
>  > to keep using the char-height (and the `frame-resize-pixelwise'
>  > docstring) for the time being, that is, not change the code in the last
>  > part of the function.
>
> If I don't use 'frame-char-height' here, any subsequent resize
> requests for that frame (including resizing the frame by dragging its
> boder with the mouse) might use the line height value.  In general,
> this is certainly not TRT, for example, when another buffer gets
> displayed in that frame's window.  That's also why I'm still reluctant
> to use the line height concept more pervasively.

Just to be clear: I do not see any problem with keeping using
`char-height' (the result of `frame-char-height') here; I only thought
that it is unfortunate that we cannot remove at least some tiny bit of
complexity.

> The particular problem could be resolved by adding a
> 'resize-pixelwise' frame parameter whose value, when set, would
> override that of 'frame-resize-pixelwise'.  Provided your frame is
> in some sense private to 'company-posframe'.
>
>  > Personally, I will simple set `frame-resize-pixelwise' to t.
>
> I don't think, however, that this problem is grave enough to warrant a
> general recommendation.  Adding all sorts of window and frame
> decorations to the value returned by 'window-text-pixel-size' will
> often add some slack whitespace when 'frame-resize-pixelwise' is nil,
> regardless of whether we round by character or line height.

I think I understand better now, I was hampered by some weird debugging
artifacts in my current setup.  With the default
`frame-resize-pixelwise' of nil, and the otherwise bug-fixed code,
nothing is cut off, but there is some slack whitespace, indeed.  The
frame parameter solution could address that, but adds even more
complexity..

From your explanation I think we should just live with it for the time
being.  No need for a general recommendation also, it was just a "TIL".

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

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

I now came up with a fix for 'fit-window-to-buffer' too which had some
strange misbehavior with different size restricting arguments when the
window's text size did not change.  See attached diffs and Change Log
below.

 > I think I understand better now, I was hampered by some weird debugging
 > artifacts in my current setup.  With the default
 > `frame-resize-pixelwise' of nil, and the otherwise bug-fixed code,
 > nothing is cut off, but there is some slack whitespace, indeed.

I suppose that part of that whitespace comes from the fact that with
'line-spacing' greater zero, 'window-text-pixel-size' includes the
space below the last line of its text.  It would be nice to get rid of
that but ISTR that a line's text may now get centered within the space
reserved for it.  So I cannot just remove the entire line space of one
line from the return value but probably only half of the line spacing
value.  How would I know the right value?

martin


Fixes for fitting windows and frames to their buffers (Bug#37563)

* lisp/window.el (window-default-font-height)
(window-default-line-height): New functions.
(fit-frame-to-buffer): Interpret values of MAX-HEIGHT and
MIN-HEIGHT arguments in terms of WINDOW's default line height
(Bug#37563).
(fit-window-to-buffer): Obey size restricting arguments even
when size of WINDOW's text does not change.  Do not
temporarily select WINDOW and perform height/width related
calculations if and only if WINDOW is accordingly combined.
Interpret values of MAX-HEIGHT and MIN-HEIGHT arguments in
terms of WINDOW's default line height.

[-- Attachment #2: fit-to-buffer.diffs --]
[-- Type: text/plain, Size: 12028 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index d93ec0add6..fafb6f90ed 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8501,6 +8501,41 @@ window-buffer-height
 			     (eobp)
 			     window))))
 
+(defun window-default-font-height (&optional window)
+  "Return height in pixels of WINDOW's default face font.
+WINDOW must be a live window and defaults to the selected one.
+
+The return value accounts for any remapping of the default face
+font on WINDOW's frame."
+  (let* ((window (window-normalize-window window t))
+	 (frame (window-frame window))
+	 (default-font (face-font 'default frame)))
+    (if (and (display-multi-font-p (frame-parameter frame 'display))
+	     (not (string-equal (frame-parameter frame 'font) default-font)))
+        (aref (font-info default-font frame) 3)
+      (frame-char-height frame))))
+
+(defun window-default-line-height (&optional window)
+  "Return height in pixels of a text line in WINDOW.
+WINDOW must be a live window and defaults to the selected one.
+
+The return value includes any line spacing defined for WINDOW's
+buffer or frame and accounts for any remapping of the default
+face on WINDOW's frame."
+  (let* ((window (window-normalize-window window t))
+	 (font-height (window-default-font-height window))
+	 (frame (window-frame window))
+	 (buffer (window-buffer window))
+	 (space-height
+	  (or (and (display-graphic-p frame)
+		   (or (buffer-local-value 'line-spacing buffer)
+		       (frame-parameter frame 'line-spacing)))
+	      0)))
+    (+ font-height
+       (if (floatp space-height)
+           (truncate (* (frame-char-height frame) space-height))
+         space-height))))
+
 ;;; Resizing windows and frames to fit their contents exactly.
 (defcustom fit-window-to-buffer-horizontally nil
   "Non-nil means `fit-window-to-buffer' can resize windows horizontally.
@@ -8643,6 +8678,7 @@ fit-frame-to-buffer
            (char-height (frame-char-height frame))
            ;; WINDOW is FRAME's root window.
            (window (frame-root-window frame))
+           (line-height (window-default-line-height window))
            (parent (frame-parent frame))
            (monitor-attributes
             (unless parent
@@ -8739,16 +8775,16 @@ fit-frame-to-buffer
            (max-height
             (min
              (cond
-              ((numberp max-height) (* max-height char-height))
-              ((numberp (nth 0 sizes)) (* (nth 0 sizes) char-height))
+              ((numberp max-height) (* max-height line-height))
+              ((numberp (nth 0 sizes)) (* (nth 0 sizes) line-height))
               (t parent-or-display-height))
              ;; The following is the maximum height that fits into the
              ;; top and bottom margins.
              (max (- bottom-margin top-margin outer-minus-body-height))))
            (min-height
             (cond
-             ((numberp min-height) (* min-height char-height))
-             ((numberp (nth 1 sizes)) (* (nth 1 sizes) char-height))
+             ((numberp min-height) (* min-height line-height))
+             ((numberp (nth 1 sizes)) (* (nth 1 sizes) line-height))
              (t (window-min-size window nil nil t))))
            (max-width
             (min
@@ -8871,124 +8907,118 @@ fit-window-to-buffer
 	 max-height min-height max-width min-width
 	 (and (memq fit-frame-to-buffer '(vertically horizontally))
 	      fit-frame-to-buffer)))
-    (with-selected-window window
-      (let* ((pixelwise window-resize-pixelwise)
-	     (char-height (frame-char-height))
-	     (char-width (frame-char-width))
-	     (total-height (window-size window nil pixelwise))
-	     (body-height (window-body-height window pixelwise))
-	     (body-width (window-body-width window pixelwise))
-	     (min-height
-	      ;; Sanitize MIN-HEIGHT.
-	      (if (numberp min-height)
-		  ;; Can't get smaller than `window-safe-min-height'.
-		  (max (if pixelwise
-			   (* char-height min-height)
-			 min-height)
-		       (if pixelwise
-			   (window-safe-min-pixel-height window)
-			 window-safe-min-height))
-		;; Preserve header and mode line if present.
-		(max (if pixelwise
-			 (* char-height window-min-height)
-		       window-min-height)
-		     (window-min-size window nil window pixelwise))))
-	     (max-height
-	      ;; Sanitize MAX-HEIGHT.
-	      (if (numberp max-height)
-		  (min
-		   (+ total-height
-		      (window-max-delta
-		       window nil window nil t nil pixelwise))
-		   (if pixelwise
-		       (* char-height max-height)
-		     max-height))
-		(+ total-height (window-max-delta
-				 window nil window nil t nil pixelwise))))
-	     height)
-	(cond
-	 ;; If WINDOW is vertically combined, try to resize it
-	 ;; vertically.
-	 ((and (not (eq fit-window-to-buffer-horizontally 'only))
-	       (not (window-size-fixed-p window 'preserved))
-	       (window-combined-p))
+    (let* ((pixelwise window-resize-pixelwise)
+           (frame (window-frame window))
+           (char-height (frame-char-height frame)))
+      (cond
+       ;; If WINDOW is vertically combined, try to resize it
+       ;; vertically.
+       ((and (not (eq fit-window-to-buffer-horizontally 'only))
+	     (not (window-size-fixed-p window 'preserved))
+	     (window-combined-p))
+        (let* ((line-height (window-default-line-height window))
+	       (total-height (window-size window nil pixelwise))
+               (min-height
+	        ;; Sanitize MIN-HEIGHT.
+	        (if (numberp min-height)
+	            ;; Can't get smaller than `window-safe-min-height'.
+	            (max (if pixelwise
+			     (* line-height min-height)
+		           min-height)
+		         (if pixelwise
+			     (window-safe-min-pixel-height window)
+		           window-safe-min-height))
+	          ;; Preserve header and mode line if present.
+	          (max (if pixelwise
+		           (* line-height window-min-height)
+		         window-min-height)
+		       (window-min-size window nil window pixelwise))))
+	       (max-height
+	        ;; Sanitize MAX-HEIGHT.
+	        (if (numberp max-height)
+	            (min
+		     (+ total-height
+		        (window-max-delta
+		         window nil window nil t nil pixelwise))
+		     (if pixelwise
+		         (* line-height max-height)
+		       (/ (* line-height max-height) line-height)))
+	          (+ total-height (window-max-delta
+			           window nil window nil t nil pixelwise))))
+	       (height (+ (cdr (window-text-pixel-size
+			        window nil t nil (frame-pixel-height frame) t))
+		          (window-scroll-bar-height window)
+		          (window-bottom-divider-width window))))
 	  ;; Vertically we always want to fit the entire buffer.
 	  ;; WINDOW'S height can't get larger than its frame's pixel
 	  ;; height.  Its width remains fixed.
-	  (setq height (+ (cdr (window-text-pixel-size
-				nil nil t nil (frame-pixel-height) t))
-			  (window-scroll-bar-height window)
-			  (window-bottom-divider-width)))
 	  ;; Round height.
 	  (unless pixelwise
 	    (setq height (/ (+ height char-height -1) char-height)))
+          (setq height (max min-height (min max-height height)))
 	  (unless (= height total-height)
 	    (window-preserve-size window)
 	    (window-resize-no-error
-	     window
-	     (- (max min-height (min max-height height)) total-height)
-	     nil window pixelwise)
+	     window (- height total-height) nil window pixelwise)
 	    (when preserve-size
-	      (window-preserve-size window nil t))))
-	 ;; If WINDOW is horizontally combined, try to resize it
-	 ;; horizontally.
-	 ((and fit-window-to-buffer-horizontally
-	       (not (window-size-fixed-p window t 'preserved))
-	       (window-combined-p nil t))
-	  (let* ((total-width (window-size window t pixelwise))
-		 (min-width
-		  ;; Sanitize MIN-WIDTH.
-		  (if (numberp min-width)
-		      ;; Can't get smaller than `window-safe-min-width'.
-		      (max (if pixelwise
-			       (* char-width min-width)
-			     min-width)
-			   (if pixelwise
-			       (window-safe-min-pixel-width)
-			     window-safe-min-width))
-		    ;; Preserve fringes, margins, scrollbars if present.
+	      (window-preserve-size window nil t)))))
+       ;; If WINDOW is horizontally combined, try to resize it
+       ;; horizontally.
+       ((and fit-window-to-buffer-horizontally
+	     (not (window-size-fixed-p window t 'preserved))
+	     (window-combined-p window t))
+	(let* ((char-width (frame-char-width frame))
+               (total-width (window-size window t pixelwise))
+	       (min-width
+		;; Sanitize MIN-WIDTH.
+		(if (numberp min-width)
+		    ;; Can't get smaller than `window-safe-min-width'.
 		    (max (if pixelwise
-			     (* char-width window-min-width)
-			   window-min-width)
-			 (window-min-size nil nil window pixelwise))))
-		 (max-width
-		  ;; Sanitize MAX-WIDTH.
-		  (if (numberp max-width)
-		      (min (+ total-width
-			      (window-max-delta
-			       window t window nil t nil pixelwise))
-			   (if pixelwise
-			       (* char-width max-width)
-			     max-width))
-		    (+ total-width (window-max-delta
-				    window t window nil t nil pixelwise))))
-		 ;; When fitting horizontally, assume that WINDOW's
-		 ;; start position remains unaltered.  WINDOW can't get
-		 ;; wider than its frame's pixel width, its height
-		 ;; remains unaltered.
-		 (width (+ (car (window-text-pixel-size
-				 nil (window-start) (point-max)
-				 (frame-pixel-width)
-				 ;; Add one char-height to assure that
-				 ;; we're on the safe side.  This
-				 ;; overshoots when the first line below
-				 ;; the bottom is wider than the window.
-				 (* body-height
-				    (if pixelwise 1 char-height))))
-			   (window-right-divider-width))))
-	    (unless pixelwise
-	      (setq width (/ (+ width char-width -1) char-width)))
-	    (unless (= width body-width)
-	      (window-preserve-size window t)
-	      (window-resize-no-error
-	       window
-	       (- (max min-width
-		       (min max-width
-			    (+ total-width (- width body-width))))
-		  total-width)
-	       t window pixelwise)
-	      (when preserve-size
-		(window-preserve-size window t t))))))))))
+			     (* char-width min-width)
+			   min-width)
+			 (if pixelwise
+			     (window-safe-min-pixel-width window)
+			   window-safe-min-width))
+		  ;; Preserve fringes, margins, scrollbars if present.
+		  (max (if pixelwise
+			   (* char-width window-min-width)
+			 window-min-width)
+		       (window-min-size window nil window pixelwise))))
+	       (max-width
+		;; Sanitize MAX-WIDTH.
+		(if (numberp max-width)
+		    (min (+ total-width
+			    (window-max-delta
+			     window t window nil t nil pixelwise))
+			 (if pixelwise
+			     (* char-width max-width)
+			   max-width))
+		  (+ total-width (window-max-delta
+				  window t window nil t nil pixelwise))))
+	       ;; When fitting horizontally, assume that WINDOW's
+	       ;; start position remains unaltered.  WINDOW can't get
+	       ;; wider than its frame's pixel width, its height
+	       ;; remains unaltered.
+	       (width (+ (car (window-text-pixel-size
+			       window (window-start) (point-max)
+			       (frame-pixel-width)
+			       ;; Add one line-height to assure that
+			       ;; we're on the safe side.  This
+			       ;; overshoots when the first line below
+			       ;; the bottom is wider than the window.
+			       (* (window-body-height window pixelwise)
+				  (if pixelwise 1 char-height))))
+                         (- total-width
+                            (window-body-width window pixelwise)))))
+	  (unless pixelwise
+	    (setq width (/ (+ width char-width -1) char-width)))
+          (setq width (max min-width (min max-width width)))
+	  (unless (= width total-width)
+	    (window-preserve-size window t)
+	    (window-resize-no-error
+             window (- width total-width) t window pixelwise)
+	    (when preserve-size
+	      (window-preserve-size window t t)))))))))
 
 (defun window-safely-shrinkable-p (&optional window)
   "Return t if WINDOW can be shrunk without shrinking other windows.

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Mon, Oct 07 2019 11:25 (+0200), martin rudalics wrote:

> I now came up with a fix for 'fit-window-to-buffer' too which had some
> strange misbehavior with different size restricting arguments when the
> window's text size did not change.  See attached diffs and Change Log
> below.
>
>  > I think I understand better now, I was hampered by some weird debugging
>  > artifacts in my current setup.  With the default
>  > `frame-resize-pixelwise' of nil, and the otherwise bug-fixed code,
>  > nothing is cut off, but there is some slack whitespace, indeed.
>
> I suppose that part of that whitespace comes from the fact that with
> 'line-spacing' greater zero, 'window-text-pixel-size' includes the
> space below the last line of its text.  It would be nice to get rid of
> that but ISTR that a line's text may now get centered within the space
> reserved for it.  So I cannot just remove the entire line space of one
> line from the return value but probably only half of the line spacing
> value.  How would I know the right value?

I think we're talking about different things.  I was talking about the
(correct) rounding to a char-height multiple at the end of
`fit-frame-to-buffer'.  This is unaffected by the line-spacing.

Example (line-spacing 1, frame-resize-pixelwise nil):

I had 8 lines of char-height 14, leading to 8*(14+1) = 120 pixels (from
window-text-pixel-size, AFAICS) before the rounding.  Rounding to a
multiple of 14, but at least as large leads to height 126, and these 6
extra pixels are what I saw.

As an aside, the posframe pkg explicitly sets `frame-resize-pixelwise'
to t, presumable to avoid the rounding, which is the correct approach
for this package, I think.

In any case, I am happy that this will be fixed, and, coming from the
application side, have probably not much else to contribute :)

Ingo

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > I think we're talking about different things.

Yes.  What I'm talking about is the slack space at the bottom of the
window provoked by the line space of the very last line displayed in
that window regardless of whether 'frame-resize-pixelwise' is on or
not.

As mentioned in my previous mail, it should be possible to remove that
minor nuisance but it will work only if the display engine draws the
line space _after_ drawing the line text.  Otherwise, some empty space
will appear at the top of the window and the last text line will get
truncated which is clearly more of a nuisance.

 > In any case, I am happy that this will be fixed, and, coming from the
 > application side, have probably not much else to contribute :)

If there are no objections, I'll install the last patch I sent in a
couple of days.

martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > If there are no objections, I'll install the last patch I sent in a
 > couple of days.

Installed.  Please close the bug if you do not see further problems.

Thanks, martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Fri, Oct 11 2019 10:16 (+0200), martin rudalics wrote:

>  > If there are no objections, I'll install the last patch I sent in a
>  > couple of days.
>
> Installed.  Please close the bug if you do not see further problems.
>
> Thanks, martin

Thanks, that all seems to work fine!

For the "user side", unfortunately, I am now becoming aware that
`company-posframe' also uses the C code's `set-frame-size', and that
suffers from the same problem :)  I will post that to emacs-devel first,
not sure if this is a bug at all..

Thanks again,
Ingo

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

Oh, one more thing.

The meaning of "line height" in the C code (and related lisp functions)
seems to be "line, without line-spacing", for better or worse.  Having
line height mean sth different in window.el will make the situation
worse, I guess :)

Maybe we should go for an ugly, but different name like
"line-height-w-space" instead?

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Robert Pluim]

>>>>> On Thu, 03 Oct 2019 10:56:28 +0200, Ingo Lohmar <ingo.lohmar@posteo.net> said:

    Ingo> Oh, one more thing.
    Ingo> The meaning of "line height" in the C code (and related lisp functions)
    Ingo> seems to be "line, without line-spacing", for better or worse.  Having
    Ingo> line height mean sth different in window.el will make the situation
    Ingo> worse, I guess :)

    Ingo> Maybe we should go for an ugly, but different name like
    Ingo> "line-height-w-space" instead?

'line-display-height' ?

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Eli Zaretskii]

> From: Ingo Lohmar <ingo.lohmar@posteo.net>
> Date: Thu, 03 Oct 2019 10:56:28 +0200
> Cc: 37563@debbugs.gnu.org
> 
> Maybe we should go for an ugly, but different name like
> "line-height-w-space" instead?

line-vertical-space?  (I'm lousy with finding good names, FWIW.)

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[martin rudalics]

 > The meaning of "line height" in the C code (and related lisp functions)
 > seems to be "line, without line-spacing", for better or worse.  Having
 > line height mean sth different in window.el will make the situation
 > worse, I guess :)
 >
 > Maybe we should go for an ugly, but different name like
 > "line-height-w-space" instead?

This ship has sailed with the advent of 'default-line-height' in
simple.el, more or less.

martin

bug#37563: 27.0.50; fit-frame-to-buffer does not account for line-spacing

[Ingo Lohmar]

On Thu, Oct 03 2019 20:10 (+0200), martin rudalics wrote:

>  > The meaning of "line height" in the C code (and related lisp functions)
>  > seems to be "line, without line-spacing", for better or worse.  Having
>  > line height mean sth different in window.el will make the situation
>  > worse, I guess :)
>  >
>  > Maybe we should go for an ugly, but different name like
>  > "line-height-w-space" instead?
>
> This ship has sailed with the advent of 'default-line-height' in
> simple.el, more or less.
>
> martin

Understood.  Just goes to show my ignorance of many of these details.

bug#37563: [PATCH] please review

[Ingo Lohmar]

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

This has an obvious fix after sleeping on it: Just increase the
char-height by the line-spacing.  The local var `char-height' is used
once again in the function, but also with the correct meaning.

Actually it should be renamed to `line-height', should I do that?
Other than that, waiting for some kind of review before I commit..


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-frame-height-calculation-from-max-height.patch --]
[-- Type: text/x-diff, Size: 1341 bytes --]

From 5b9ee5b913b031fca6f424170d9b61addb090294 Mon Sep 17 00:00:00 2001
From: Ingo Lohmar <ingo.lohmar@github.com>
Date: Tue, 1 Oct 2019 09:27:55 +0200
Subject: [PATCH] Fix frame height calculation from max-height

When `fit-frame-to-buffer' is given a non-nil `max-height' argument,
we need to work not with the char-height, but with the line-height
including `line-spacing' (Bug#37563).

* lisp/window.el (fit-frame-to-buffer): Account for line-spacing in height.
---
 lisp/window.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/window.el b/lisp/window.el
index 620eacdd29..21b58495fa 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8637,7 +8637,10 @@ fit-frame-to-buffer
   (setq frame (window-normalize-frame frame))
   (when (window-live-p (frame-root-window frame))
     (let* ((char-width (frame-char-width frame))
-           (char-height (frame-char-height frame))
+           (char-height (+ (frame-char-height frame)
+                           ;; Add line-spacing in the selected window's buffer.
+                           (buffer-local-value 'line-spacing
+                                               (window-buffer (car (window-list))))))
            ;; WINDOW is FRAME's root window.
            (window (frame-root-window frame))
            (parent (frame-parent frame))
-- 
2.23.0

bug#37563: [PATCH] please review

[martin rudalics]

 > This has an obvious fix after sleeping on it: Just increase the
 > char-height by the line-spacing.  The local var `char-height' is used
 > once again in the function, but also with the correct meaning.
 >
 > Actually it should be renamed to `line-height', should I do that?
 > Other than that, waiting for some kind of review before I commit..

Received, finally.  Would be interesting to know what kept it so long.

martin

bug#37563: Acknowledgement (27.0.50; fit-frame-to-buffer does not account for line-spacing)

[Ingo Lohmar]