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

Previous Next

Full log

Message #40 received at 66676 <at> debbugs.gnu.org (full text, mbox):

From: Rahguzar <rahguzar <at> zohomail.eu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 66676 <at> debbugs.gnu.org,
 Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Subject: Re: bug#66676: 29.1; Should some aspects of shr rendering be
 configurable
Date: Sun, 19 Nov 2023 13:07:26 +0100

[Message part 1 (text/plain, inline)]

Hi Eli,

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks.  Rahguzar, any followup to these comments?

I replied to the comments. Please let me know what is the preferred way
of preferred way of proceeding in that case.

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

Thanks! I have reworded all log messages. Hopefully they confirm to the
standards better now.

>> +(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.

Fixed.

>> +(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.

Fixed.

>> +(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"

Fixed.

>> +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?

I meant 'trailing space'. Fixed now.

Sorry for the typos and thanks for catching them.

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

Fixed.

> Thanks.

I have attached the updated patches.

Thank you,
Rahguzar

[0001-Make-some-aspects-of-shr-rendering-customizable.patch (text/x-patch, attachment)]

[0002-Allow-displaying-images-inline.patch (text/x-patch, attachment)]

[0003-Outline-support-for-shr-rendered-documents.patch (text/x-patch, attachment)]

[0004-Optionally-turn-on-visual-line-mode-outline-support.patch (text/x-patch, attachment)]

[0005-Don-t-insert-subscript-on-a-newline.patch (text/x-patch, attachment)]

Previous Next

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