[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#66676: 29.1; Should some aspects of shr rendering be configurable
From: |
Eli Zaretskii |
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@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, rahguzar@zohomail.eu,
> 66676@debbugs.gnu.org
> Date: Sat, 04 Nov 2023 13:05:25 +0100
>
> Eli Zaretskii <eliz@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.