From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Evgeny Zajcev Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Add support for `ch' and `cw' dimension specifiers for the image Date: Thu, 28 Mar 2024 13:50:37 +0300 Message-ID: References: <86sf0j1q0c.fsf@gnu.org> <86cyrehdre.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000dadd4d0614b64b74" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33483"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Alan Third , emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Mar 28 11:52:09 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rpnMf-0008Tk-GI for ged-emacs-devel@m.gmane-mx.org; Thu, 28 Mar 2024 11:52:09 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rpnLT-0001AV-NO; Thu, 28 Mar 2024 06:50:55 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rpnLS-00018H-AY for emacs-devel@gnu.org; Thu, 28 Mar 2024 06:50:54 -0400 Original-Received: from mail-pj1-x1032.google.com ([2607:f8b0:4864:20::1032]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rpnLQ-0004SV-A4; Thu, 28 Mar 2024 06:50:54 -0400 Original-Received: by mail-pj1-x1032.google.com with SMTP id 98e67ed59e1d1-29de2dd22d8so611814a91.2; Thu, 28 Mar 2024 03:50:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1711623049; x=1712227849; darn=gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=L8DbX+OrUDRLOg5gIUVo20hPkdLw3Xs9+f/6b/KdO68=; b=CeUInbQDlj2lVTA/OTxubXyXWUFa5gLA9hmXMCFV6Id1IFwHRdxMYKC33Iwc3akIgk iCrKzVVN0ZLd0x3d5fTzvAzsVZGCaEOWCUN/yS8Ci1aVcCfDSzoufXQhiZd8wuAsIsd+ fXey69DvVOVA0cEKYw72kmfBrZrwQ/CXU/iI0cA/apkA6j75lBYyfm7P9OdBSeRC27Sy oSHCgjb1Q9w3FlDpyHlRFacd/jyGNHvpXOiQdDXNg/MueMksb6cQI2VYCkpM55LzbWmw N1s7LDNsYu3W5FYzZO9ucyAvOr0nQ97RuKWdrFgIP4LZs6+8Owgv69okBYrcmadTX1wk 6N7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711623049; x=1712227849; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=L8DbX+OrUDRLOg5gIUVo20hPkdLw3Xs9+f/6b/KdO68=; b=s/Nz0gUeJ+JGPM9EEhOa6tHiNwAqFcBusJoga33KMl9oY+W7Cq3dgDrNNiZgExlPjP hUtJuaAF88IUdJNBNI77/y0SAqYyPx3mSwkho/vKtQdrvquM2wq3L+/jPNWVVk8vNsvB t9MXCvAhgt80dXNYNZgKU+QP4QzZUlwNn4Hi+eHv/L45zYOq9itwzsBQHt4ZXIVXYpxG jkWDFg5j20Q38vheTaiZV2KzkXhxpr7/jAlUfqzKDVwSv1bqZ2a1bnZvOCaDqhSsGSvd gKoL1pju6NnU7Vi+D9vX2fNLfzgIKvS5tCStsvtzTqk9nlaVdDtAK4BpSPvjcM9L8So1 5YKQ== X-Forwarded-Encrypted: i=1; AJvYcCXoBb6O0NSlZgmJWHXmo+DtHDBBE5i9kDSr4Bq8WFeOMDUnfFg1Qs16AygCp+BGTj1IfTfPGOU8yHvu2wi+zDRY75ZD X-Gm-Message-State: AOJu0YynwcT+XZVBWMtAtSdeuxfk7TTs/Ab+i/WqVQKGvMBpG5q3vIJh tcuRZuv67ieuvKvyqwg9qTcnzCDC37rQZUwHAex2z9n0jgPQP7LFYnJDGOl4kGC5hCZKdxnHrXG ARjMDfZz/wpOA7zeoQsDQgCKrmeIfw2gm X-Google-Smtp-Source: AGHT+IFEW7GiHIz//zv1nWd4UCiNKBt1BSJ9XVgSXeUhMWhKLbvn+ojE0BvMN5hWRdfkW/bsds0UyIF0a42210LMSIU= X-Received: by 2002:a17:90a:4586:b0:29c:7592:febf with SMTP id v6-20020a17090a458600b0029c7592febfmr1961817pjg.16.1711623049499; Thu, 28 Mar 2024 03:50:49 -0700 (PDT) In-Reply-To: <86cyrehdre.fsf@gnu.org> Received-SPF: pass client-ip=2607:f8b0:4864:20::1032; envelope-from=lg.zevlg@gmail.com; helo=mail-pj1-x1032.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:317356 Archived-At: --000000000000dadd4d0614b64b74 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=87=D1=82, 28 =D0=BC=D0=B0=D1=80. 2024=E2=80=AF=D0=B3. =D0=B2 13:06, Eli= Zaretskii : > > From: Evgeny Zajcev > > Date: Thu, 21 Mar 2024 22:14:51 +0300 > > Cc: emacs-devel@gnu.org > > > > =D1=87=D1=82, 21 =D0=BC=D0=B0=D1=80. 2024=E2=80=AF=D0=B3. =D0=B2 19:57,= Eli Zaretskii : > > > > > From: Evgeny Zajcev > > > 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 'cente= r > > > :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 =3D 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 --=20 lg --000000000000dadd4d0614b64b74 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
=D1=87=D1=82, 28 =D0=BC=D0=B0=D1=80. = 2024=E2=80=AF=D0=B3. =D0=B2 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-dev= el@gnu.org
>
> =D1=87=D1=82, 21 =D0=BC=D0=B0=D1=80. 2024=E2=80=AF=D0=B3. =D0=B2 19:57= , Eli Zaretskii <eliz@= gnu.org>:
>
>=C2=A0 > From: Evgeny Zajcev <lg.zevlg@gmail.com>
>=C2=A0 > Date: Thu, 21 Mar 2024 17:53:09 +0300
>=C2=A0 >
>=C2=A0 > With applied patch and image specified as:
>=C2=A0 >
>=C2=A0 >=C2=A0 =C2=A0(list 'image :type 'svg :file "fil= e.svg" :scale 1.0 :ascent 'center
>=C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 :width '(2 . cw)
>=C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 :max-height '(1 . ch)= )
>
>=C2=A0 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
>=C2=A0 length in @dfn{ems}@footnote{In typography an em is a distance >=C2=A0 equivalent to the height of the type.=C2=A0 For example when usi= ng 12 point
>=C2=A0 type 1 em is equal to 12 points.=C2=A0 Its use ensures distances= and type
> -remain proportional.}.=C2=A0 One em is equivalent to the height of th= e font
> -and @var{value} may be an integer or a float.
> +remain proportional.}.=C2=A0 One em is equivalent to the size of the = font
> +and @var{value} may be an integer or a float.=C2=A0 Also, dimension c= an be

Here, you changed the description of "em" from "height of th= e font" to
"size of the font".=C2=A0 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, whe= n 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.=C2=A0 That's why I started usin= g coefficient (calculated with `my-em-height-ratio') to `em' specif= ier


> +=C2=A0 /* Details of the font used to calculate image size relative t= o the
> +=C2=A0 =C2=A0 =C2=A0canonical character size, with `ch' and `cw&#= 39; specifiers. */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0^^
Please leave two spaces after the last sentence of the comment.

noted

> +=C2=A0 if (CONSP (value) && NUMBERP (CAR (value)))
> +=C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 if (EQ (Qem, CDR (value)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return scale_image_size (img->face_fon= t_size,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, XFLOATINT (CAR (value))= );
> +=C2=A0 =C2=A0 =C2=A0 if (EQ (Qch, CDR (value)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return scale_image_size (img->face_fon= t_height,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, XFLOATINT (CAR (value))= );
> +=C2=A0 =C2=A0 =C2=A0 if (EQ (Qcw, CDR (value)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return scale_image_size (img->face_fon= t_width,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, XFLOATINT (CAR (value))= );

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

=C2=A0 =C2=A0 if (CONSP (value) && NUMBERP (CAR (value)))
=C2=A0 =C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Lisp_Object dim =3D CDR (value);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (EQ (Qem, dim))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return scale_image_size (img->face_fo= nt_size,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, XFLOATINT (CAR (valu= e)));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (EQ (Qch, dim))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return scale_image_size (img->face_fo= nt_height,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01, XFLOATINT (CAR (valu= e)));


Sure, thought about it, but I think th= e compiler should do such things, value is not volatile.=C2=A0 I did not kn= ow about compilers that don't do basic optimizations

will do

etc.=C2=A0 (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
--000000000000dadd4d0614b64b74--