GNU bug report logs -
#69555
30.0.50; shr - Preserve indentation when shr-fill-text is nil
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 69555 in the body.
You can then email your comments to 69555 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
rahguzar <at> zohomail.eu, bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Mon, 04 Mar 2024 22:04:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
:
New bug report received and forwarded. Copy sent to
rahguzar <at> zohomail.eu, bug-gnu-emacs <at> gnu.org
.
(Mon, 04 Mar 2024 22:04:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hello,
I am a grateful and happy user of the new shr-fill-text option; one nit
I have with its current implementation is that, since setting it to nil
essentially short-circuits shr-fill-lines, we skip shr-fill-line, which
actually handles both filling *and* indentation.
This is very noticeable when reading HTML emails with Gnus: citations in
blockquotes become indistinguishable from the actual reply. See
eww-current.png for a demonstration with EWW:
* left is shr-fill-text t (default),
* right is shr-fill-text nil and visual-line-fringe-indicators
'(left-curly-arrow right-curly-arrow).
The attached patch makes it so that shr applies indentation before it
consult the shr-fill-text option and skips filling. See eww-patch.png
for a demonstration:
* left is shr-fill-text nil and visual-line-fringe-indicators
'(left-curly-arrow right-curly-arrow),
* right is the same, plus visual-wrap-prefix-mode,
* not pictured (unchanged): shr-fill-text t (default).
This patch also adds a rendering testcase, and teaches the rendering
test to re-run testcases while setting extra user options to make sure
they do not impact rendering.
Curious to hear your thoughts. Will post an updated patch once I have a
bug number to replace the "bug#TODO" placeholders.
Thanks!
[0001-Keep-indenting-text-when-shr-fill-text-is-nil-bug-TO.patch (text/x-patch, attachment)]
[eww-current.png (image/png, attachment)]
[eww-patch.png (image/png, attachment)]
[Message part 5 (text/plain, inline)]
In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.39, cairo version 1.18.0) of 2024-01-06 built on amdahl30
Repository revision: 657275529e31226bbc6c92eb7f7af887474a0bb8
Repository branch: master
Windowing system distributor 'SUSE LINUX', version 11.0.12302004
System Description: openSUSE Tumbleweed
Configured using:
'configure --prefix=/home/peniblec/apps/emacs --with-cairo
--with-sqlite3 --with-xinput2'
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB
Important settings:
value of $LANG: en_US.UTF-8
locale-coding-system: utf-8-unix
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Mon, 04 Mar 2024 22:11:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 69555 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
> Will post an updated patch once I have a
> bug number to replace the "bug#TODO" placeholders.
See attached.
[0001-Keep-indenting-text-when-shr-fill-text-is-nil-bug-69.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Tue, 05 Mar 2024 12:10:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 69555 <at> debbugs.gnu.org (full text, mbox):
> Cc: Rahguzar <rahguzar <at> zohomail.eu>
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Date: Mon, 04 Mar 2024 23:08:20 +0100
>
> Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
>
> > Will post an updated patch once I have a
> > bug number to replace the "bug#TODO" placeholders.
>
> See attached.
The change in behavior is unconditional, AFAIU. Should we have a
separate knob for that? Perhaps someone out there doesn't want text
to be indented when shr-fill-text is nil?
Also, please time the code on some substantially large body of text,
with and without shr-fill-text, and compare that with the current
version. I think performance is an important aspect of any change in
this area.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Tue, 05 Mar 2024 19:25:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Cc: Rahguzar <rahguzar <at> zohomail.eu>
>> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
>> Date: Mon, 04 Mar 2024 23:08:20 +0100
>>
>> Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
>>
>> > Will post an updated patch once I have a
>> > bug number to replace the "bug#TODO" placeholders.
>>
>> See attached.
>
> The change in behavior is unconditional, AFAIU. Should we have a
> separate knob for that? Perhaps someone out there doesn't want text
> to be indented when shr-fill-text is nil?
Perhaps; I could look into adding that knob. FWIW nothing in the
original feature request (bug#66676) gives me the impression that this
new option should affect indentation; the stated motivation was:
> 1) Using `visual-line-mode` for line wrapping. I think this is more
> natural for html and makes resizing windows work more nicely.
That's why I interpret the indentation change as a side-effect; curious
to hear Rahguzar's thoughts though.
Another reason I'm not overly fond of adding another knob is figuring
out what should happen if a user keeps shr-fill-text set to t, but sets
that hypothetical indentation option to nil. Should we support that?
Or should shr-fill-text become a tri-state? (nil, 'indent-only, t)
I'm open to biting the bullet, but I'd also like to make sure we don't
sophisticate shr.el further than we want to.
> Also, please time the code on some substantially large body of text,
> with and without shr-fill-text, and compare that with the current
> version. I think performance is an important aspect of any change in
> this area.
Can do; would "(elisp) Profiling" be the starting point? (For all my
list lurking, I confess my eyes have always glossed over discussions
involving benchmarks)
(Also wondering if we have any "standard" or preferred HTML documents or
websites to throw at shr.el for benchmarking purposes; if not, I guess
I'll peruse <https://en.wikipedia.org/wiki/Special:LongPages> 🤔)
Thanks for the review!
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Tue, 05 Mar 2024 19:49:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 69555 <at> debbugs.gnu.org (full text, mbox):
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: 69555 <at> debbugs.gnu.org, rahguzar <at> zohomail.eu
> Date: Tue, 05 Mar 2024 20:22:52 +0100
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Also, please time the code on some substantially large body of text,
> > with and without shr-fill-text, and compare that with the current
> > version. I think performance is an important aspect of any change in
> > this area.
>
> Can do; would "(elisp) Profiling" be the starting point?
I think benchmark-run is a better starting point.
> (Also wondering if we have any "standard" or preferred HTML documents or
> websites to throw at shr.el for benchmarking purposes; if not, I guess
> I'll peruse <https://en.wikipedia.org/wiki/Special:LongPages> 🤔)
Actually, something with a lot of text in large paragraphs, sometimes
indented, would be better. Those Wikipedia pages are basically long
lists, but there's not much opportunity there to perform filling and
indentation of large amounts of text, which is what is sought here.
Here's one possible candidate:
https://debbugs.gnu.org/Developer.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Tue, 05 Mar 2024 23:19:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 69555 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
>> Cc: 69555 <at> debbugs.gnu.org, rahguzar <at> zohomail.eu
>> Date: Tue, 05 Mar 2024 20:22:52 +0100
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> > Also, please time the code on some substantially large body of text,
>> > with and without shr-fill-text, and compare that with the current
>> > version. I think performance is an important aspect of any change in
>> > this area.
>>
>> Can do; would "(elisp) Profiling" be the starting point?
>
> I think benchmark-run is a better starting point.
>
>> (Also wondering if we have any "standard" or preferred HTML documents or
>> websites to throw at shr.el for benchmarking purposes; if not, I guess
>> I'll peruse <https://en.wikipedia.org/wiki/Special:LongPages> 🤔)
>
> Actually, something with a lot of text in large paragraphs, sometimes
> indented, would be better. Those Wikipedia pages are basically long
> lists, but there's not much opportunity there to perform filling and
> indentation of large amounts of text, which is what is sought here.
> Here's one possible candidate:
>
> https://debbugs.gnu.org/Developer.html
Thanks for the pointers. Attaching the over-engineered scripts I built
from that, which with 1000 REPETITIONS yield:
2024-03-05; 33976ecf244; 30.0.50; master shr-fill-text=nil ( 3.013 23 0.313)
2024-03-04; b06916cb218; 30.0.50; shr-blockquote shr-fill-text=nil ( 3.121 24 0.328)
2024-03-05; 33976ecf244; 30.0.50; master shr-fill-text=t (32.331 65 0.904)
2024-03-04; b06916cb218; 30.0.50; shr-blockquote shr-fill-text=t (32.045 65 0.902)
I can bump up REPETITIONS if that would help; sending the scripts &
results as-is before hitting the hay since I figure they might have some
glaring methodology issues, or there is more information I might not
have thought of reporting.
If not, the tentative conclusion would be "shr-fill-text nil gets 4%
slower; shr-fill-text t is none the worse for wear; nil still runs
circles around t"?
[bench.el (text/x-emacs-lisp, attachment)]
[bench.sh (application/x-shellscript, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 06 Mar 2024 07:32:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Hi Kévin,
Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
> Perhaps; I could look into adding that knob. FWIW nothing in the
> original feature request (bug#66676) gives me the impression that this
> new option should affect indentation; the stated motivation was:
>
> > 1) Using `visual-line-mode` for line wrapping. I think this is more
> > natural for html and makes resizing windows work more nicely.
>
> That's why I interpret the indentation change as a side-effect; curious
> to hear Rahguzar's thoughts though.
You are right that losing indentation was not intentional and an
oversight on my part. Thanks for the fix. I will test it soon.
Rahguzar
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 06 Mar 2024 11:55:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 69555 <at> debbugs.gnu.org (full text, mbox):
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: 69555 <at> debbugs.gnu.org, rahguzar <at> zohomail.eu
> Date: Wed, 06 Mar 2024 00:16:53 +0100
>
> If not, the tentative conclusion would be "shr-fill-text nil gets 4%
> slower; shr-fill-text t is none the worse for wear; nil still runs
> circles around t"?
Thanks, I think 4% slowdown is negligible in this case.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 06 Mar 2024 21:20:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
>> Cc: 69555 <at> debbugs.gnu.org, rahguzar <at> zohomail.eu
>> Date: Wed, 06 Mar 2024 00:16:53 +0100
>>
>> If not, the tentative conclusion would be "shr-fill-text nil gets 4%
>> slower; shr-fill-text t is none the worse for wear; nil still runs
>> circles around t"?
>
> Thanks, I think 4% slowdown is negligible in this case.
ACK.
Re. indentation being done unconditionally…
Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> The change in behavior is unconditional, AFAIU. Should we have a
>> separate knob for that? Perhaps someone out there doesn't want text
>> to be indented when shr-fill-text is nil?
>
> Perhaps; I could look into adding that knob. FWIW nothing in the
> original feature request (bug#66676) gives me the impression that this
> new option should affect indentation; the stated motivation was:
>
> > 1) Using `visual-line-mode` for line wrapping. I think this is more
> > natural for html and makes resizing windows work more nicely.
>
> That's why I interpret the indentation change as a side-effect; curious
> to hear Rahguzar's thoughts though.
… considering Rahguzar's reply (thanks for that, and for the offer to
try the patch 🙏)…
> Another reason I'm not overly fond of adding another knob is figuring
> out what should happen if a user keeps shr-fill-text set to t, but sets
> that hypothetical indentation option to nil. Should we support that?
> Or should shr-fill-text become a tri-state? (nil, 'indent-only, t)
>
> I'm open to biting the bullet, but I'd also like to make sure we don't
> sophisticate shr.el further than we want to.
… do we think there is a case for an option to disable indentation then?
Or should we leave that for a future feature request?
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Thu, 07 Mar 2024 06:40:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 69555 <at> debbugs.gnu.org (full text, mbox):
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: 69555 <at> debbugs.gnu.org, rahguzar <at> zohomail.eu
> Date: Wed, 06 Mar 2024 22:18:19 +0100
>
> … do we think there is a case for an option to disable indentation then?
I guess not.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 13 Mar 2024 19:57:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Hey Rahguzar,
Rahguzar <rahguzar <at> zohomail.eu> writes:
> Hi Kévin,
>
> Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
>
>> Perhaps; I could look into adding that knob. FWIW nothing in the
>> original feature request (bug#66676) gives me the impression that this
>> new option should affect indentation; the stated motivation was:
>>
>> > 1) Using `visual-line-mode` for line wrapping. I think this is more
>> > natural for html and makes resizing windows work more nicely.
>>
>> That's why I interpret the indentation change as a side-effect; curious
>> to hear Rahguzar's thoughts though.
>
> You are right that losing indentation was not intentional and an
> oversight on my part. Thanks for the fix. I will test it soon.
No rush or anything; it just occurs to me that if you're busy with
Things™, please do not hesitate to delegate the testing: if you have
specific use-cases or webpages in mind that you'd want to check the
patch against, don't hesitate to tell me about them, I can take a look
and report how things pan out.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 13 Mar 2024 20:32:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Hi Kévin,
Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
> No rush or anything; it just occurs to me that if you're busy with
> Things™, please do not hesitate to delegate the testing: if you have
> specific use-cases or webpages in mind that you'd want to check the
> patch against, don't hesitate to tell me about them, I can take a look
> and report how things pan out.
Sorry, this just slipped out of my mind. But I have looked at the patch
and also tested it and it is a definite improvement. I hadn't realized
why some html emails looked a bit jumbled up so restoring the
indentation fixes them. Thanks a lot for it.
Rahguzar
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Wed, 13 Mar 2024 21:44:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 69555 <at> debbugs.gnu.org (full text, mbox):
Rahguzar <rahguzar <at> zohomail.eu> writes:
> Hi Kévin,
>
> Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:
>
>> No rush or anything; it just occurs to me that if you're busy with
>> Things™, please do not hesitate to delegate the testing: if you have
>> specific use-cases or webpages in mind that you'd want to check the
>> patch against, don't hesitate to tell me about them, I can take a look
>> and report how things pan out.
>
> Sorry, this just slipped out of my mind.
No worries 👌
> But I have looked at the patch
> and also tested it and it is a definite improvement. I hadn't realized
> why some html emails looked a bit jumbled up so restoring the
> indentation fixes them.
Yay!
> Thanks a lot for it.
Thank _you_ for getting the ball rolling with shr-fill-text in the first
place!
Eli, am I clear to push & close?
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Thu, 14 Mar 2024 05:06:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 69555 <at> debbugs.gnu.org (full text, mbox):
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: 69555 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> Date: Wed, 13 Mar 2024 22:41:32 +0100
>
> Eli, am I clear to push & close?
Yes, but please fix the following nits before installing:
. use only ASCII characters in the commit log message
. make sure lines in the commit log message do not exceed 65 columns
Thanks.
Reply sent
to
Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
:
You have taken responsibility.
(Fri, 15 Mar 2024 07:12:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
:
bug acknowledged by developer.
(Fri, 15 Mar 2024 07:12:02 GMT)
Full text and
rfc822 format available.
Message #49 received at 69555-done <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
>> Cc: 69555 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>> Date: Wed, 13 Mar 2024 22:41:32 +0100
>>
>> Eli, am I clear to push & close?
>
> Yes, but please fix the following nits before installing:
>
> . use only ASCII characters in the commit log message
(ACK; FWIW I did check the repo log for a precedent when adding those
ellipses. Wondering if build-aux/git-hooks/commit-msg ought to report
this? It does seem to explicitly check for non-UTF-8)
> . make sure lines in the commit log message do not exceed 65 columns
(ACK; can't remember where I got my 70-odd filling came from, CONTRIBUTE
does explicitly mention "63". Maybe I got confused with the concurrent
discussion on emacs-devel)
> Thanks.
Nits picked, patch pushed, closing. Thank you both for the review!
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#69555
; Package
emacs
.
(Fri, 15 Mar 2024 08:50:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 69555-done <at> debbugs.gnu.org (full text, mbox):
> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: rahguzar <at> zohomail.eu, 69555-done <at> debbugs.gnu.org
> Date: Fri, 15 Mar 2024 08:10:09 +0100
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > . use only ASCII characters in the commit log message
>
> (ACK; FWIW I did check the repo log for a precedent when adding those
> ellipses. Wondering if build-aux/git-hooks/commit-msg ought to report
> this? It does seem to explicitly check for non-UTF-8)
I'm against making our commit hooks more strict, as they are already
quite a nuisance in some situations, for example, when merging from
the release branch. It isn't a catastrophe when UTF-8 encoded
non-ASCII characters snick into our logs, so it's a soft preference,
not something we should enforce by rejecting commits.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Fri, 12 Apr 2024 11:24:26 GMT)
Full text and
rfc822 format available.
This bug report was last modified 1 year and 126 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.