Re: master a876c4d7a17: Improve SHR/EWW support for 'visual-wrap-prefix-mode'

Re: master a876c4d7a17: Improve SHR/EWW support for 'visual-wrap-prefix-mode'

[Eshel Yaron]

Hello Jim,

Jim Porter <jporterbugs@gmail.com> writes:

> branch: master
> commit a876c4d7a17df152e3e78800c76ddf158f632ee5
> Author: Jim Porter <jporterbugs@gmail.com>
> Commit: Jim Porter <jporterbugs@gmail.com>
>
>     Improve SHR/EWW support for 'visual-wrap-prefix-mode'
>     
[...]

Thanks for this!

It seems like this change breaks Elfeed though, namely:

>  (defun shr-indent ()
>    (when (> shr-indentation 0)
> -    (if (not shr-use-fonts)
> -        (insert-char ?\s shr-indentation)
> -      (insert ?\s)
> -      (put-text-property (1- (point)) (point)
> -                         'display `(space :width (,shr-indentation))))))
> +    (let ((start (point))
> +          (prefix (or (get-text-property (point) 'shr-prefix-length) 0)))
> +      (if (not shr-use-fonts)
> +          (insert-char ?\s shr-indentation)
> +        (insert ?\s)
> +        (put-text-property
> +         (1- (point)) (point) 'display
> +         ;; Set the specified space width in terms of the default width
> +         ;; of the current face, like (N . width).  That way, the
> +         ;; indentation is calculated correctly when using
> +         ;; `text-scale-adjust'.
> +         `(space :width (,(if-let ((font (font-at (1- (point))))
> +                                   (info (query-font font)))
> +                              (/ (float shr-indentation) (aref info 7))
> +                            shr-indentation)
> +                         . width))))
> +      (put-text-property start (+ (point) prefix)
> +                         'shr-prefix-length (+ prefix (- (point) start))))))

This call to font-at throws an error unless the current buffer happens
to be displayed in the selected window.  This affects elfeed-show-entry,
for example.


Cheers,

Eshel

Re: master a876c4d7a17: Improve SHR/EWW support for 'visual-wrap-prefix-mode'

[Kévin Le Gouguec]

Eshel Yaron <me@eshelyaron.com> writes:

> Hello Jim,
>
> Jim Porter <jporterbugs@gmail.com> writes:
>
>> branch: master
>> commit a876c4d7a17df152e3e78800c76ddf158f632ee5
>> Author: Jim Porter <jporterbugs@gmail.com>
>> Commit: Jim Porter <jporterbugs@gmail.com>
>>
>>     Improve SHR/EWW support for 'visual-wrap-prefix-mode'
>>     
> [...]
>
> Thanks for this!
>
> It seems like this change breaks Elfeed though, namely:
>
>>  (defun shr-indent ()
>>    (when (> shr-indentation 0)
>> -    (if (not shr-use-fonts)
>> -        (insert-char ?\s shr-indentation)
>> -      (insert ?\s)
>> -      (put-text-property (1- (point)) (point)
>> -                         'display `(space :width (,shr-indentation))))))
>> +    (let ((start (point))
>> +          (prefix (or (get-text-property (point) 'shr-prefix-length) 0)))
>> +      (if (not shr-use-fonts)
>> +          (insert-char ?\s shr-indentation)
>> +        (insert ?\s)
>> +        (put-text-property
>> +         (1- (point)) (point) 'display
>> +         ;; Set the specified space width in terms of the default width
>> +         ;; of the current face, like (N . width).  That way, the
>> +         ;; indentation is calculated correctly when using
>> +         ;; `text-scale-adjust'.
>> +         `(space :width (,(if-let ((font (font-at (1- (point))))
>> +                                   (info (query-font font)))
>> +                              (/ (float shr-indentation) (aref info 7))
>> +                            shr-indentation)
>> +                         . width))))
>> +      (put-text-property start (+ (point) prefix)
>> +                         'shr-prefix-length (+ prefix (- (point) start))))))
>
> This call to font-at throws an error unless the current buffer happens
> to be displayed in the selected window.  This affects elfeed-show-entry,
> for example.

Thanks for raising this; FWIW I noticed a simliar issue with Gnus, where
I add visual-wrap-prefix-mode to gnus-article-prepare-hook.  Don't have
a backtrace handy, but had convinced myself that the faulty font-at was
this one in visual-wrap--content-prefix…

  (if-let ((font (font-at position))
               (info (query-font font)))
          …)

… from 2024-08-04 "Add support for variable-pitch fonts in
'visual-wrap-prefix-mode'" (f70a6ea0ea8); and so meant to submit this…

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 902a9e41c5e..975ccab3adb 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -164,7 +164,8 @@ visual-wrap--content-prefix
     ;; width of the first-line prefix in canonical-width characters.
     ;; This is useful if the first-line prefix uses some very-wide
     ;; characters.
-    (if-let ((font (font-at position))
+    (if-let ((window (get-buffer-window (current-buffer)))
+             (font (font-at position window))
              (info (query-font font)))
         (max (string-width prefix)
              (ceiling (string-pixel-width prefix (current-buffer))

… but meant to research & test more before submitting.

Re: master a876c4d7a17: Improve SHR/EWW support for 'visual-wrap-prefix-mode'

[Jim Porter]

On 8/21/2024 10:50 AM, Kévin Le Gouguec wrote:
> Eshel Yaron <me@eshelyaron.com> writes:
> 
>> Hello Jim,
>>
>> Jim Porter <jporterbugs@gmail.com> writes:
>>
>>> branch: master
>>> commit a876c4d7a17df152e3e78800c76ddf158f632ee5
>>> Author: Jim Porter <jporterbugs@gmail.com>
>>> Commit: Jim Porter <jporterbugs@gmail.com>
>>>
>>>      Improve SHR/EWW support for 'visual-wrap-prefix-mode'
>>>      
>> [...]
>>
>> Thanks for this!
>>
>> It seems like this change breaks Elfeed though, namely:
[snip]
>> This call to font-at throws an error unless the current buffer happens
>> to be displayed in the selected window.  This affects elfeed-show-entry,
>> for example.

Thanks for noticing this. It'd be good to make the "get the average 
width of the font" bit work properly in all cases.

That said, this is *also* a bug in Elfeed. SHR just works a lot better 
if the buffer is displayed in a window before rendering the HTML out. 
See <https://github.com/skeeto/elfeed/pull/521>.

> diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
> index 902a9e41c5e..975ccab3adb 100644
> --- a/lisp/visual-wrap.el
> +++ b/lisp/visual-wrap.el
> @@ -164,7 +164,8 @@ visual-wrap--content-prefix
>       ;; width of the first-line prefix in canonical-width characters.
>       ;; This is useful if the first-line prefix uses some very-wide
>       ;; characters.
> -    (if-let ((font (font-at position))
> +    (if-let ((window (get-buffer-window (current-buffer)))
> +             (font (font-at position window))
>                (info (query-font font)))
>           (max (string-width prefix)
>                (ceiling (string-pixel-width prefix (current-buffer))

Assuming this is working for you, I think this patch makes sense and you 
can feel free to merge it.