GNU bug report logs - #66676
29.1; Should some aspects of shr rendering be configurable

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: rahguzar <at> zohomail.eu, larsi <at> gnus.org, 66676 <at> debbugs.gnu.org
Subject: bug#66676: 29.1; Should some aspects of shr rendering be configurable
Date: Sat, 18 Nov 2023 10:59:23 +0200

> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: Lars Ingebrigtsen <larsi <at> gnus.org>,  rahguzar <at> zohomail.eu,
>   66676 <at> debbugs.gnu.org
> Date: Sat, 04 Nov 2023 13:05:25 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Lars, anybody else?  Any comments to these changes?
> 
> As an occasional user of shr (mainly when viewing HTML parts in Gnus)
> who frequently messes with window width and face height, I'm very much
> interested in better visual-line-mode integration.  Also intrigued by
> outline-mode support.  So closely following this thread, and
> enthusiastically thanking Rahguzar for tackling this 👏
> 
> Don't know if I have anything insightful to say re. the patches.  Idly
> wondering if '(null shr-fill-text) ⇒ (visual-line-mode)' is the right
> control flow; e.g. maybe
> 
>   (a) shr-fill-text could be set to 'visual as a more explicit hint for
>   major modes, or
> 
>   (b) major modes should grow new user options: e.g. eww-wrap-method ∈
>   {fill, visual}; 'visual would tell modes to set shr-fill-text to nil &
>   enable visual-line-mode.
> 
> There are already plenty of user knobs at the shr level though, so maybe
> no need to overthink this.

Thanks.  Rahguzar, any followup to these comments?

Please also see my minor comments below:

> * lisp/net/shr.el
> (shr-fill-text): New custom variable
> (shr-sup-raise-factor): New custom variable
> (shr-sub-raise-factor): New custom variable
> (shr-image-ascent): New custom variable
> (shr-fill-lines): Only fill if shr-fill-text is non nil
> (shr-put-image): Use shr-image-ascent as value of :ascent
> (shr-rescale-image): Use shr-image-ascent
> (shr-make-placeholder-image): Use shr-image-ascent
> (shr-tag-sup): use shr-sup-raise-factor
> (shr-tag-sub): use shr-sub-raise-factor

This doesn't follow our conventions:

  . identical entries should be grouped if possible (see below)
  . descriptions of changes should be complete sentences: start with a
  capital letter and end with a period
  . symbols should be quoted 'like this'

In this case, here's how to format the above descriptions:

* lisp/net/shr.el (shr-fill-text, shr-sup-raise-factor)
(shr-sub-raise-factor, shr-image-ascent): New custom variables.
(shr-fill-lines): Only fill if 'shr-fill-text' is non-nil.
(shr-put-image): Use 'shr-image-ascent' as value of :ascent.
(shr-rescale-image, shr-make-placeholder-image): Use
'shr-image-ascent'.
(shr-tag-sup, shr-tag-sub): Use 'shr-sub-raise-factor'.

Similar changes are needed in your other log messages.

> +(defcustom shr-fill-text t
> +  "Non-nil means to fill the text according to the width of the window.
> +If nil text is not filled and `visual-line-mode' can be used to reflow text."
         ^                  ^
Two commas missing there.

> +(defcustom shr-sup-raise-factor 0.2
> +  "The value of raise property for superscripts.
> +Should be a number between 0 and 1."

This is better:

  Should be a non-negative float number between 0 and 1.

> +(defcustom shr-sub-raise-factor -0.2
> +  "The value of raise property for subscripts.
> +Should be a number between 0 and -1."

Likewise here (but "non-positive" instead of "non-negative").

> +(defcustom shr-max-inline-image-size nil
> +  "If non-nil determines when the images can be displayed inline.
> +If nil images are never displayed inline.

Commas missing after "nil" in both sentences.

> +HEIGHT can be also be an integer or a floating point number.  If it is an
> +integer and the pixel height of an image exceeds it, the image image is
> +displyed on a separate line.  If it is an floating point, the limit is
                                          ^^^^^^^^^^^^^^^^^
"a floating point number"

> +interpreted as multiples of the height of default font."
               ^^^^^^^^^^^^
"as a multiple"

> @@ -1103,19 +1135,25 @@ shr-put-image
>                                             (plist-get flags :width)
>                                             (plist-get flags :height)))))))
>          (when image
> +          ;; The trailing confuse can confuse shr-insert into not
> +          ;; putting any space after inline images.

"The trailing confuse can confuse" sounds strange, and is probably a
typo of sorts.  What did you mean to say there?

> * lisp/net/shr.el (shr-tag-sub): see above

This is not a proper change description.  Either repeat the heading or
include the file and function in the heading line (if they fit; they
don't fit in this case, I think).

> +  ;; possible in Emacs. So we remove the newline in that case.
                         ^^
Our convention is to leave 2 spaces between sentences, not one.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.