Re: [PATCH] Add support for `ch' and `cw' dimension specifiers for the image

[Eli Zaretskii <eliz@gnu.org>]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Thu, 21 Mar 2024 22:14:51 +0300
> Cc: emacs-devel@gnu.org
> 
> чт, 21 мар. 2024 г. в 19:57, Eli Zaretskii <eliz@gnu.org>:
> 
>  > From: Evgeny Zajcev <lg.zevlg@gmail.com>
>  > Date: Thu, 21 Mar 2024 17:53:09 +0300
>  > 
>  > With applied patch and image specified as:
>  > 
>  >   (list 'image :type 'svg :file "file.svg" :scale 1.0 :ascent 'center
>  >          :width '(2 . cw)
>  >          :max-height '(1 . ch))
> 
>  ENOPATCH
> 
> :)) sorry, here it is

Thanks.

Alan, could you please take a look and comment on this?

I have a couple of minor stylistic comments below.

> diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> index 4dbb4afb20d..73671a21e7f 100644
> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -5788,8 +5788,11 @@ Image Descriptors
>  length in @dfn{ems}@footnote{In typography an em is a distance
>  equivalent to the height of the type.  For example when using 12 point
>  type 1 em is equal to 12 points.  Its use ensures distances and type
> -remain proportional.}.  One em is equivalent to the height of the font
> -and @var{value} may be an integer or a float.
> +remain proportional.}.  One em is equivalent to the size of the font
> +and @var{value} may be an integer or a float.  Also, dimension can be

Here, you changed the description of "em" from "height of the font" to
"size of the font".  Is this intentional, and if so, why it is better
to say "size" here?

> +  /* Details of the font used to calculate image size relative to the
> +     canonical character size, with `ch' and `cw' specifiers. */
                                                               ^^
Please leave two spaces after the last sentence of the comment.

> +  if (CONSP (value) && NUMBERP (CAR (value)))
> +    {
> +      if (EQ (Qem, CDR (value)))
> +        return scale_image_size (img->face_font_size,
> +                                 1, XFLOATINT (CAR (value)));
> +      if (EQ (Qch, CDR (value)))
> +        return scale_image_size (img->face_font_height,
> +                                 1, XFLOATINT (CAR (value)));
> +      if (EQ (Qcw, CDR (value)))
> +        return scale_image_size (img->face_font_width,
> +                                 1, XFLOATINT (CAR (value)));

Minor efficiency comment: it is better to compute CDR(value) just once
and store it in a temporary:

    if (CONSP (value) && NUMBERP (CAR (value)))
      {
        Lisp_Object dim = CDR (value);

        if (EQ (Qem, dim))
          return scale_image_size (img->face_font_size,
                                   1, XFLOATINT (CAR (value)));
        if (EQ (Qch, dim))
          return scale_image_size (img->face_font_height,
                                   1, XFLOATINT (CAR (value)));

etc.  (Optimizing compilers will do this automatically, but an
unoptimized build might become a tad faster.)

Finally, please include a ChangeLog-style commit log message for this
patch; see CONTRIBUTE for how we expect that to be formatted (and you
can use "git log" to see what we do in practice).