bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

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!



[-- Attachment #2: 0001-Keep-indenting-text-when-shr-fill-text-is-nil-bug-TO.patch --]
[-- Type: text/x-patch, Size: 8312 bytes --]

From b8f45f22c58b4500887c7aa352c1580402c34808 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 3 Mar 2024 17:20:56 +0100
Subject: [PATCH] Keep indenting text when 'shr-fill-text' is nil (bug#TODO)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The 'shr-fill-…' functions handle both hard-filling (adding newlines to
break long lines) and indentation.  Setting 'shr-fill-text' to nil
currently causes these functions to be short-circuited completely, so
e.g. blockquotes are no longer indented, whereas the intent of this user
option is only to prevent hard-filling to let visual-line-mode reflow
text.

* lisp/net/shr.el (shr-fill-lines): Document that the function handles
more than just filling; move the 'shr-fill-text' check…
(shr-fill-line): … here, after indentation has been taken care of.
* test/lisp/net/shr-resources/blockquote.html:
* test/lisp/net/shr-resources/blockquote.txt: New test resources.
* test/lisp/net/shr-tests.el (shr-test--rendering-check): Rename from
'shr-test', to make the relationship with the 'rendering' testcase
clearer; prefer 'file-name-concat' to 'format'; raise ERT failure if need
be, calling (ert-fail …) directly instead of (should (not (list …))).
(shr-test--rendering-extra-configs): New variable to easily check that
user customizations do not degrade rendering.
(rendering): Consult that new variable; delegate failure-raising to reduce
duplication.
---
 lisp/net/shr.el                             | 15 +++--
 test/lisp/net/shr-resources/blockquote.html |  2 +
 test/lisp/net/shr-resources/blockquote.txt  |  3 +
 test/lisp/net/shr-tests.el                  | 72 +++++++++++++++------
 4 files changed, 67 insertions(+), 25 deletions(-)
 create mode 100644 test/lisp/net/shr-resources/blockquote.html
 create mode 100644 test/lisp/net/shr-resources/blockquote.txt

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index e23fc6104d2..09df5f5a9bb 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -784,8 +784,9 @@ shr-insert
 			       (or shr-current-font 'shr-text)))))))))
 
 (defun shr-fill-lines (start end)
-  (if (or (not shr-fill-text) (<= shr-internal-width 0))
-      nil
+  "Indent and fill text from START to END.
+When `shr-fill-text' is nil, only indent."
+  (unless (<= shr-internal-width 0)
     (save-restriction
       (narrow-to-region start end)
       (goto-char start)
@@ -807,6 +808,8 @@ shr-vertical-motion
       (forward-char 1))))
 
 (defun shr-fill-line ()
+  "Indent and fill the current line.
+When `shr-fill-text' is nil, only indent."
   (let ((shr-indentation (or (get-text-property (point) 'shr-indentation)
                              shr-indentation))
 	(continuation (get-text-property
@@ -821,9 +824,11 @@ shr-fill-line
 			   `,(shr-face-background face))))
     (setq start (point))
     (setq shr-indentation (or continuation shr-indentation))
-    ;; If we have an indentation that's wider than the width we're
-    ;; trying to fill to, then just give up and don't do any filling.
-    (when (< shr-indentation shr-internal-width)
+    ;; Fill the current line, unless `shr-fill-text' is unset, or we
+    ;; have an indentation that's wider than the width we're trying to
+    ;; fill to.
+    (when (and shr-fill-text
+               (< shr-indentation shr-internal-width))
       (shr-vertical-motion shr-internal-width)
       (when (looking-at " $")
         (delete-region (point) (line-end-position)))
diff --git a/test/lisp/net/shr-resources/blockquote.html b/test/lisp/net/shr-resources/blockquote.html
new file mode 100644
index 00000000000..412caf8bae6
--- /dev/null
+++ b/test/lisp/net/shr-resources/blockquote.html
@@ -0,0 +1,2 @@
+<blockquote>Citation.</blockquote>
+<div>Reply.</div>
diff --git a/test/lisp/net/shr-resources/blockquote.txt b/test/lisp/net/shr-resources/blockquote.txt
new file mode 100644
index 00000000000..8ed610b8ea2
--- /dev/null
+++ b/test/lisp/net/shr-resources/blockquote.txt
@@ -0,0 +1,3 @@
+    Citation.
+
+Reply.
diff --git a/test/lisp/net/shr-tests.el b/test/lisp/net/shr-tests.el
index 0c6e2c091bf..a9d39eba639 100644
--- a/test/lisp/net/shr-tests.el
+++ b/test/lisp/net/shr-tests.el
@@ -29,30 +29,62 @@
 
 (declare-function libxml-parse-html-region "xml.c")
 
-(defun shr-test (name)
-  (with-temp-buffer
-    (insert-file-contents (format (concat (ert-resource-directory) "/%s.html") name))
-    (let ((dom (libxml-parse-html-region (point-min) (point-max)))
-          (shr-width 80)
-          (shr-use-fonts nil))
-      (erase-buffer)
-      (shr-insert-document dom)
-      (cons (buffer-substring-no-properties (point-min) (point-max))
-            (with-temp-buffer
-              (insert-file-contents
-               (format (concat (ert-resource-directory) "/%s.txt") name))
-              (while (re-search-forward "%\\([0-9A-F][0-9A-F]\\)" nil t)
-                (replace-match (string (string-to-number (match-string 1) 16))
-                               t t))
-              (buffer-string))))))
+(defun shr-test--rendering-check (name &optional context)
+  "Render NAME.html and compare it to NAME.txt.
+Raise a test failure if the rendered buffer does not match NAME.txt.
+Append CONTEXT to the failure data, if non-nil."
+  (let ((text-file (file-name-concat (ert-resource-directory) (concat name ".txt")))
+        (html-file (file-name-concat (ert-resource-directory) (concat name ".html")))
+        (description (if context (format "%s (%s)" name context) name)))
+    (with-temp-buffer
+      (insert-file-contents html-file)
+      (let ((dom (libxml-parse-html-region (point-min) (point-max)))
+            (shr-width 80)
+            (shr-use-fonts nil))
+        (erase-buffer)
+        (shr-insert-document dom)
+        (let ((result (buffer-substring-no-properties (point-min) (point-max)))
+              (expected
+               (with-temp-buffer
+                 (insert-file-contents text-file)
+                 (while (re-search-forward "%\\([0-9A-F][0-9A-F]\\)" nil t)
+                   (replace-match (string (string-to-number (match-string 1) 16))
+                                  t t))
+                 (buffer-string))))
+          (unless (equal result expected)
+            (ert-fail (list description result expected))))))))
+
+(defconst shr-test--rendering-extra-configs
+  '(("blockquote"
+     ;; Make sure blockquotes remain indented even when filling is
+     ;; disabled (bug#TODO).
+     . ((shr-fill-text . nil))))
+  "Extra customizations which can impact rendering.
+This is a list of (NAME . SETTINGS) pairs.  NAME is the basename of a
+set of txt/html files under shr-resources/, as passed to `shr-test'.
+SETTINGS is a list of (OPTION . VALUE) pairs that are interesting to
+validate for the NAME testcase.
+
+The `rendering' testcase will test NAME once without altering any
+settings, then once more for each (OPTION . VALUE) pair.")
 
 (ert-deftest rendering ()
   (skip-unless (fboundp 'libxml-parse-html-region))
   (dolist (file (directory-files (ert-resource-directory) nil "\\.html\\'"))
-    (let* ((name (replace-regexp-in-string "\\.html\\'" "" file))
-           (result (shr-test name)))
-      (unless (equal (car result) (cdr result))
-        (should (not (list name (car result) (cdr result))))))))
+    (let* ((name (string-remove-suffix ".html" file))
+           (extra-options (alist-get name shr-test--rendering-extra-configs
+                                     nil nil 'string=)))
+      ;; Test once with default settings.
+      (shr-test--rendering-check name)
+      ;; Test once more for every extra option for this specific NAME.
+      (pcase-dolist (`(,option-sym ,option-val)
+                     extra-options)
+        (let ((option-old (symbol-value option-sym)))
+          (set option-sym option-val)
+          (unwind-protect
+              (shr-test--rendering-check
+               name (format "with %s %s" option-sym option-val))
+            (set option-sym option-old)))))))
 
 (ert-deftest use-cookies ()
   (let ((shr-cookie-policy 'same-origin))
-- 
2.44.0


[-- Attachment #3: eww-current.png --]
[-- Type: image/png, Size: 211502 bytes --]

[-- Attachment #4: eww-patch.png --]
[-- Type: image/png, Size: 184192 bytes --]

[-- Attachment #5: Type: text/plain, Size: 819 bytes --]



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

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

>                                 Will post an updated patch once I have a
> bug number to replace the "bug#TODO" placeholders.

See attached.


[-- Attachment #2: 0001-Keep-indenting-text-when-shr-fill-text-is-nil-bug-69.patch --]
[-- Type: text/x-patch, Size: 8314 bytes --]

From ea41aa4ef92357f00bc7814d7020a81104e4b95f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 3 Mar 2024 17:20:56 +0100
Subject: [PATCH] Keep indenting text when 'shr-fill-text' is nil (bug#69555)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The 'shr-fill-…' functions handle both hard-filling (adding newlines to
break long lines) and indentation.  Setting 'shr-fill-text' to nil
currently causes these functions to be short-circuited completely, so
e.g. blockquotes are no longer indented, whereas the intent of this user
option is only to prevent hard-filling to let visual-line-mode reflow
text.

* lisp/net/shr.el (shr-fill-lines): Document that the function handles
more than just filling; move the 'shr-fill-text' check…
(shr-fill-line): … here, after indentation has been taken care of.
* test/lisp/net/shr-resources/blockquote.html:
* test/lisp/net/shr-resources/blockquote.txt: New test resources.
* test/lisp/net/shr-tests.el (shr-test--rendering-check): Rename from
'shr-test', to make the relationship with the 'rendering' testcase
clearer; prefer 'file-name-concat' to 'format'; raise ERT failure if need
be, calling (ert-fail …) directly instead of (should (not (list …))).
(shr-test--rendering-extra-configs): New variable to easily check that
user customizations do not degrade rendering.
(rendering): Consult that new variable; delegate failure-raising to reduce
duplication.
---
 lisp/net/shr.el                             | 15 +++--
 test/lisp/net/shr-resources/blockquote.html |  2 +
 test/lisp/net/shr-resources/blockquote.txt  |  3 +
 test/lisp/net/shr-tests.el                  | 72 +++++++++++++++------
 4 files changed, 67 insertions(+), 25 deletions(-)
 create mode 100644 test/lisp/net/shr-resources/blockquote.html
 create mode 100644 test/lisp/net/shr-resources/blockquote.txt

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index e23fc6104d2..09df5f5a9bb 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -784,8 +784,9 @@ shr-insert
 			       (or shr-current-font 'shr-text)))))))))
 
 (defun shr-fill-lines (start end)
-  (if (or (not shr-fill-text) (<= shr-internal-width 0))
-      nil
+  "Indent and fill text from START to END.
+When `shr-fill-text' is nil, only indent."
+  (unless (<= shr-internal-width 0)
     (save-restriction
       (narrow-to-region start end)
       (goto-char start)
@@ -807,6 +808,8 @@ shr-vertical-motion
       (forward-char 1))))
 
 (defun shr-fill-line ()
+  "Indent and fill the current line.
+When `shr-fill-text' is nil, only indent."
   (let ((shr-indentation (or (get-text-property (point) 'shr-indentation)
                              shr-indentation))
 	(continuation (get-text-property
@@ -821,9 +824,11 @@ shr-fill-line
 			   `,(shr-face-background face))))
     (setq start (point))
     (setq shr-indentation (or continuation shr-indentation))
-    ;; If we have an indentation that's wider than the width we're
-    ;; trying to fill to, then just give up and don't do any filling.
-    (when (< shr-indentation shr-internal-width)
+    ;; Fill the current line, unless `shr-fill-text' is unset, or we
+    ;; have an indentation that's wider than the width we're trying to
+    ;; fill to.
+    (when (and shr-fill-text
+               (< shr-indentation shr-internal-width))
       (shr-vertical-motion shr-internal-width)
       (when (looking-at " $")
         (delete-region (point) (line-end-position)))
diff --git a/test/lisp/net/shr-resources/blockquote.html b/test/lisp/net/shr-resources/blockquote.html
new file mode 100644
index 00000000000..412caf8bae6
--- /dev/null
+++ b/test/lisp/net/shr-resources/blockquote.html
@@ -0,0 +1,2 @@
+<blockquote>Citation.</blockquote>
+<div>Reply.</div>
diff --git a/test/lisp/net/shr-resources/blockquote.txt b/test/lisp/net/shr-resources/blockquote.txt
new file mode 100644
index 00000000000..8ed610b8ea2
--- /dev/null
+++ b/test/lisp/net/shr-resources/blockquote.txt
@@ -0,0 +1,3 @@
+    Citation.
+
+Reply.
diff --git a/test/lisp/net/shr-tests.el b/test/lisp/net/shr-tests.el
index 0c6e2c091bf..17138053450 100644
--- a/test/lisp/net/shr-tests.el
+++ b/test/lisp/net/shr-tests.el
@@ -29,30 +29,62 @@
 
 (declare-function libxml-parse-html-region "xml.c")
 
-(defun shr-test (name)
-  (with-temp-buffer
-    (insert-file-contents (format (concat (ert-resource-directory) "/%s.html") name))
-    (let ((dom (libxml-parse-html-region (point-min) (point-max)))
-          (shr-width 80)
-          (shr-use-fonts nil))
-      (erase-buffer)
-      (shr-insert-document dom)
-      (cons (buffer-substring-no-properties (point-min) (point-max))
-            (with-temp-buffer
-              (insert-file-contents
-               (format (concat (ert-resource-directory) "/%s.txt") name))
-              (while (re-search-forward "%\\([0-9A-F][0-9A-F]\\)" nil t)
-                (replace-match (string (string-to-number (match-string 1) 16))
-                               t t))
-              (buffer-string))))))
+(defun shr-test--rendering-check (name &optional context)
+  "Render NAME.html and compare it to NAME.txt.
+Raise a test failure if the rendered buffer does not match NAME.txt.
+Append CONTEXT to the failure data, if non-nil."
+  (let ((text-file (file-name-concat (ert-resource-directory) (concat name ".txt")))
+        (html-file (file-name-concat (ert-resource-directory) (concat name ".html")))
+        (description (if context (format "%s (%s)" name context) name)))
+    (with-temp-buffer
+      (insert-file-contents html-file)
+      (let ((dom (libxml-parse-html-region (point-min) (point-max)))
+            (shr-width 80)
+            (shr-use-fonts nil))
+        (erase-buffer)
+        (shr-insert-document dom)
+        (let ((result (buffer-substring-no-properties (point-min) (point-max)))
+              (expected
+               (with-temp-buffer
+                 (insert-file-contents text-file)
+                 (while (re-search-forward "%\\([0-9A-F][0-9A-F]\\)" nil t)
+                   (replace-match (string (string-to-number (match-string 1) 16))
+                                  t t))
+                 (buffer-string))))
+          (unless (equal result expected)
+            (ert-fail (list description result expected))))))))
+
+(defconst shr-test--rendering-extra-configs
+  '(("blockquote"
+     ;; Make sure blockquotes remain indented even when filling is
+     ;; disabled (bug#69555).
+     . ((shr-fill-text . nil))))
+  "Extra customizations which can impact rendering.
+This is a list of (NAME . SETTINGS) pairs.  NAME is the basename of a
+set of txt/html files under shr-resources/, as passed to `shr-test'.
+SETTINGS is a list of (OPTION . VALUE) pairs that are interesting to
+validate for the NAME testcase.
+
+The `rendering' testcase will test NAME once without altering any
+settings, then once more for each (OPTION . VALUE) pair.")
 
 (ert-deftest rendering ()
   (skip-unless (fboundp 'libxml-parse-html-region))
   (dolist (file (directory-files (ert-resource-directory) nil "\\.html\\'"))
-    (let* ((name (replace-regexp-in-string "\\.html\\'" "" file))
-           (result (shr-test name)))
-      (unless (equal (car result) (cdr result))
-        (should (not (list name (car result) (cdr result))))))))
+    (let* ((name (string-remove-suffix ".html" file))
+           (extra-options (alist-get name shr-test--rendering-extra-configs
+                                     nil nil 'string=)))
+      ;; Test once with default settings.
+      (shr-test--rendering-check name)
+      ;; Test once more for every extra option for this specific NAME.
+      (pcase-dolist (`(,option-sym ,option-val)
+                     extra-options)
+        (let ((option-old (symbol-value option-sym)))
+          (set option-sym option-val)
+          (unwind-protect
+              (shr-test--rendering-check
+               name (format "with %s %s" option-sym option-val))
+            (set option-sym option-old)))))))
 
 (ert-deftest use-cookies ()
   (let ((shr-cookie-policy 'same-origin))
-- 
2.44.0

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> Cc: Rahguzar <rahguzar@zohomail.eu>
> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Mon, 04 Mar 2024 23:08:20 +0100
> 
> Kévin Le Gouguec <kevin.legouguec@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.

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Rahguzar <rahguzar@zohomail.eu>
>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Date: Mon, 04 Mar 2024 23:08:20 +0100
>> 
>> Kévin Le Gouguec <kevin.legouguec@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!

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: 69555@debbugs.gnu.org,  rahguzar@zohomail.eu
> Date: Tue, 05 Mar 2024 20:22:52 +0100
> 
> Eli Zaretskii <eliz@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

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: 69555@debbugs.gnu.org,  rahguzar@zohomail.eu
>> Date: Tue, 05 Mar 2024 20:22:52 +0100
>> 
>> Eli Zaretskii <eliz@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"?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bench.el --]
[-- Type: text/x-emacs-lisp, Size: 1027 bytes --]

;; -*- lexical-binding: t; -*-

(require 'shr)

(defun bench/git (&rest args)
  (car (apply 'process-lines "git" "-C" source-directory args)))

(defun bench/describe-emacs ()
  (let* ((fork-point
          (bench/git
           "merge-base" "--fork-point" "origin/master"
           emacs-repository-version))
         (git-desc
          (bench/git
           "show" "--date=short" "--format=format:%cd; %h" fork-point)))
    (format "%s; %-7s; %s" git-desc emacs-version emacs-repository-branch)))

(let ((dom (with-temp-buffer
             (insert-file-contents "test.html")
             (libxml-parse-html-region)))
      (emacs-desc (bench/describe-emacs)))
  (dolist (val '(nil t))
    (setopt shr-fill-text val)
    (pcase-let ((`(,total-time ,gc-count ,gc-time)
                 (benchmark-run 1000
                   (with-temp-buffer (shr-insert-document dom)))))
      (message
       "%s\tshr-fill-text=%s\t(%6.3f %d %6.3f)"
       (bench/describe-emacs)
       shr-fill-text
       total-time gc-count gc-time))))

[-- Attachment #3: bench.sh --]
[-- Type: application/x-shellscript, Size: 210 bytes --]

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: 69555@debbugs.gnu.org,  rahguzar@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.

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: 69555@debbugs.gnu.org,  rahguzar@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@gmail.com> writes:

> Eli Zaretskii <eliz@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?

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: 69555@debbugs.gnu.org,  rahguzar@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.

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Rahguzar via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Kévin,

Kévin Le Gouguec <kevin.legouguec@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

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

Hey Rahguzar,

Rahguzar <rahguzar@zohomail.eu> writes:

> Hi Kévin,
>
> Kévin Le Gouguec <kevin.legouguec@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.

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Rahguzar via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Kévin,

Kévin Le Gouguec <kevin.legouguec@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

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

Rahguzar <rahguzar@zohomail.eu> writes:

> Hi Kévin,
>
> Kévin Le Gouguec <kevin.legouguec@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?

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: 69555@debbugs.gnu.org,  Eli Zaretskii <eliz@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.

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: 69555@debbugs.gnu.org,  Eli Zaretskii <eliz@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!

bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: rahguzar@zohomail.eu,  69555-done@debbugs.gnu.org
> Date: Fri, 15 Mar 2024 08:10:09 +0100
> 
> Eli Zaretskii <eliz@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.