bug#66676: 29.1; Should some aspects of shr rendering be configurable

[Rahguzar]

Hi Eli,

Please disregard the patches in my last email. A last minute change
introduced an pretty bad error into them. Basically I had noticed that
the first time a page is fetched the spacing between inline images is
not write. So I tried some changes and I don't know how but they seemed
to work so I sent them. However they caused an error when starting a new
Emacs session. I have not found the correct fix and done it in such a
way that if the option for maximum size of inline images is not set the
old code path is followed, so existing behavior will not be changed by
default. I have used this fix for couple of days and it works as
expected.

I have attached updated set of patches with this email.

Thank you,
Rahguzar

Rahguzar <rahguzar@zohomail.eu> writes:

> Hi Eli,
>
> Eli Zaretskii <eliz@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
>
> [2. text/x-patch; 
> 0001-Make-some-aspects-of-shr-rendering-customizable.patch]...
>
> [3. text/x-patch; 0002-Allow-displaying-images-inline.patch]...
>
> [4. text/x-patch; 0003-Outline-support-for-shr-rendered-documents.patch]...
>
> [5. text/x-patch; 
> 0004-Optionally-turn-on-visual-line-mode-outline-support.patch]...
>
> [6. text/x-patch; 0005-Don-t-insert-subscript-on-a-newline.patch]...

0001-Make-some-aspects-of-shr-rendering-customizable.patch
Description: Text Data

0002-Allow-displaying-images-inline.patch
Description: Text Data

0003-Outline-support-for-shr-rendered-documents.patch
Description: Text Data

0004-Optionally-turn-on-visual-line-mode-outline-support.patch
Description: Text Data

0005-Don-t-insert-subscript-on-a-newline.patch
Description: Text Data