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

[Eli Zaretskii]

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