Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix

From: Kévin Le Gouguec <[email-masked]>
Date: Mon, 22 Jun 2020 00:01:20 +0200

"Basil L. Contovounesios" <[email-masked]> writes:

> Kévin Le Gouguec <[email-masked]> writes:
>
>> +(defun adaptive-wrap--prefix-face (fcp beg end)
>> +  (or (get-text-property 0 'face fcp)
>> +      <snip>
>> +      (when (= (char-before end) ?\n)
>> +        (let ((eol-face (get-text-property (1- end) 'face)))
>
> Is it guaranteed that (< (point-min) end (1+ (point-max)))?
> Otherwise = and get-text-property will barf.

I think I managed to convince myself that there's no risk; if I'm
reading adaptive-wrap-prefix-function (and jit-lock.el) correctly, end
is always strictly greater than beg (which is at least (point-min)), and
never goes past (point-max).

>> +          (when (and eol-face (adaptive-wrap--face-extends eol-face))
>> +            eol-face)))))
>
> Nit: Can't the when+and be replaced with a single and?

Sure.

>> +                      ?\ ))))
>
> Please change this to ?\s regardless of whether the second patch is
> installed.

Can do.

> Apart from the redundant when, I think it's fine.  If you really want
> to shave off some forms you can write e.g.

Thanks, I'll go with (cond …).

Updated patches:

>From a6c18e102e80ea495675a06ae065610fa9642255 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <[email-masked]>
Date: Mon, 15 Jun 2020 23:02:08 +0200
Subject: [PATCH 1/2] Fontify adaptive-wrap's wrap-prefix

This attempts to fix at least two suboptimal situations:

1. when extra-indent is positive, and the padding character is not a
space: the extra-indent characters were not fontified, so they clashed
between the fontified prefix returned by fill-context-prefix and the
fontified continuation line.

2. when the wrapped line has a background that extends beyond
end-of-line (e.g. removed/added lines in diff-mode): the unfontified
wrap-prefixes looked like "holes" in the otherwise uniform background.

See bug#41810 for motivating use-cases and implementation rationale.

* packages/adaptive-wrap/adaptive-wrap.el
(adaptive-wrap--face-extends): Compatibility shim for Emacs < 27.
(adaptive-wrap--prefix-face): New function to determine what face to
apply to the wrap-prefix.
(adaptive-wrap--prefix): New function to compute the full wrap-prefix,
extracted verbatim from adaptive-wrap-fill-context-prefix.
(adaptive-wrap-fill-context-prefix): Call the new functions.
---
 packages/adaptive-wrap/adaptive-wrap.el | 63 ++++++++++++++++++-------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/packages/adaptive-wrap/adaptive-wrap.el b/packages/adaptive-wrap/adaptive-wrap.el
index f8d89ac69..1021bb2a9 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -59,6 +59,46 @@ extra indent = 2
   :group 'visual-line)
 (make-variable-buffer-local 'adaptive-wrap-extra-indent)
 
+(defun adaptive-wrap--face-extends (face)
+  (if (fboundp 'face-extend-p)
+      (face-extend-p face nil t)
+    ;; Before Emacs 27, faces always extended beyond EOL.  Check for a
+    ;; non-default background.
+    (face-background face nil t)))
+
+(defun adaptive-wrap--prefix-face (fcp beg end)
+  (cond ((get-text-property 0 'face fcp))
+        ;; If the last character is a newline and has a face that
+        ;; extends beyond EOL, assume that this face spans the whole
+        ;; line and apply it to the prefix to preserve the "block"
+        ;; visual effect.
+        ;; NB: the face might not actually span the whole line: see for
+        ;; example removed lines in diff-mode, where the first character
+        ;; has the diff-indicator-removed face, while the rest of the
+        ;; line has the diff-removed face.
+        ((= (char-before end) ?\n)
+         (let ((eol-face (get-text-property (1- end) 'face)))
+           (and eol-face (adaptive-wrap--face-extends eol-face) eol-face)))))
+
+(defun adaptive-wrap--prefix (fcp)
+  (let ((fcp-len (string-width fcp)))
+    (cond
+     ((= 0 adaptive-wrap-extra-indent)
+      fcp)
+     ((< 0 adaptive-wrap-extra-indent)
+      (concat
+       fcp
+       (make-string adaptive-wrap-extra-indent
+                    (if (< 0 fcp-len)
+                        (string-to-char (substring fcp -1))
+                      ?\s))))
+     ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
+      (substring fcp
+                 0
+                 (+ adaptive-wrap-extra-indent fcp-len)))
+     (t
+      ""))))
+
 (defun adaptive-wrap-fill-context-prefix (beg end)
   "Like `fill-context-prefix', but with length adjusted by `adaptive-wrap-extra-indent'."
   (let* ((fcp
@@ -72,23 +112,12 @@ extra indent = 2
                     (fill-context-prefix beg end))
                   ;; Note: fill-context-prefix may return nil; See:
                   ;; http://article.gmane.org/gmane.emacs.devel/156285
-                  ""))
-         (fcp-len (string-width fcp))
-         (fill-char (if (< 0 fcp-len)
-                        (string-to-char (substring fcp -1))
-                      ?\ )))
-    (cond
-     ((= 0 adaptive-wrap-extra-indent)
-      fcp)
-     ((< 0 adaptive-wrap-extra-indent)
-      (concat fcp
-              (make-string adaptive-wrap-extra-indent fill-char)))
-     ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
-      (substring fcp
-                 0
-                 (+ adaptive-wrap-extra-indent fcp-len)))
-     (t
-      ""))))
+              ""))
+         (prefix (adaptive-wrap--prefix fcp))
+         (face (adaptive-wrap--prefix-face fcp beg end)))
+    (if face
+        (propertize prefix 'face face)
+      prefix)))
 
 (defun adaptive-wrap-prefix-function (beg end)
   "Indent the region between BEG and END with adaptive filling."
-- 
2.27.0

>From 4757eb8dccec07257ebad3261c2dd847c47afca0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <[email-masked]>
Date: Sun, 21 Jun 2020 12:43:34 +0200
Subject: [PATCH 2/2] Always use spaces for extra-indent in adaptive-wrap
 (bug#41810)

* packages/adaptive-wrap/adaptive-wrap.el (adaptive-wrap--prefix): Use
spaces; ignore the string returned by fill-context-prefix.
---
 packages/adaptive-wrap/adaptive-wrap.el | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/packages/adaptive-wrap/adaptive-wrap.el b/packages/adaptive-wrap/adaptive-wrap.el
index 1021bb2a9..b075b3b7e 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -86,12 +86,7 @@ extra indent = 2
      ((= 0 adaptive-wrap-extra-indent)
       fcp)
      ((< 0 adaptive-wrap-extra-indent)
-      (concat
-       fcp
-       (make-string adaptive-wrap-extra-indent
-                    (if (< 0 fcp-len)
-                        (string-to-char (substring fcp -1))
-                      ?\s))))
+      (concat fcp (make-string adaptive-wrap-extra-indent ?\s)))
      ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
       (substring fcp
                  0
-- 
2.27.0

Thank you for the review!