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

[Evgeny Zajcev <lg.zevlg@gmail.com>]

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

чт, 28 мар. 2024 г. в 13:06, 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?
>

Yes, this is intentional, because saying "height of the font" in docs, when
font's pixel size is used in code, is misleading and it took me some time
to understand why image renders smaller then font height if '(1 . em) is
specified as dimension modifier.  That's why I started using coefficient
(calculated with `my-em-height-ratio') to `em' specifier


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

noted

> +  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)));
>
>
Sure, thought about it, but I think the compiler should do such things,
value is not volatile.  I did not know about compilers that don't do basic
optimizations

will do

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

Yeah, will send an update soon.

Thank you

-- 
lg

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