bug#41810: Tweaking adaptive-wrap

bug#41810: Tweaking adaptive-wrap

[Kévin Le Gouguec]

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

Hello Emacs,

adaptive-wrap is a GNU ELPA package that adds a wrap-prefix property to
continuation lines to make them look like they have been filled with
M-q.  For example, it makes this…

    ; Here is a long Elisp comment
    with its continuation line.

… look like this:

    ; Here is a long Elisp comment
    ; with its continuation line.

The second "; " is not part of the buffer text: it is the wrap-prefix
computed by adaptive-wrap.  This prefix can be customized with the
integer variable adaptive-wrap-extra-indent, which adds (if positive) or
takes off (if negative) some extra padding.

In bug#41810, I tried to tackle the following issues:

1. The extra-indent character is not fontified:
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=1;bug=41810;filename=1-extra-indent.png;msg=5>

2. The prefix has no background, which makes lines with :extend'ed
   backgrounds visually jarring:
<https://debbugs.gnu.org/cgi/bugreport.cgi?msg=5;filename=2-extend-t-current.png;bug=41810;att=3>

I proposed (and Stefan and Basil kindly reviewed) two patches:

1. One to fix those two bugs:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch1.patch --]
[-- Type: text/x-patch, Size: 4524 bytes --]

From a6c18e102e80ea495675a06ae065610fa9642255 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
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


[-- Attachment #3: Type: text/plain, Size: 460 bytes --]


Screenshots:
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=3;bug=41810;filename=patch1-diff-1.png;msg=20>
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=4;bug=41810;filename=patch1-diff-2.png;msg=20>
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=5;bug=41810;filename=patch1-nospace-1.png;msg=20>
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=6;bug=41810;filename=patch1-nospace-2.png;msg=20>

2. One to only ever use spaces for positive extra-indentation:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: patch2.patch --]
[-- Type: text/x-patch, Size: 1231 bytes --]

From 4757eb8dccec07257ebad3261c2dd847c47afca0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
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


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


Screenshots:
<https://debbugs.gnu.org/cgi/bugreport.cgi?msg=20;bug=41810;filename=patch2-nospace-1.png;att=13>
<https://debbugs.gnu.org/cgi/bugreport.cgi?msg=20;bug=41810;filename=patch2-nospace-2.png;att=14>


Lars squashed those two patches and applied them to elpa.git.  Before
bumping the version number (and therefore releasing a new version), it'd
be great if people could either

1. review the patches some more: e.g. does the logic in
   adaptive-wrap--prefix-face sound OK, or is it completely bonkers?

2. try the changes and tell us if something broke horribly; if you don't
   have elpa.git cloned, you can get the latest adaptive-wrap.el here:

   https://git.savannah.gnu.org/cgit/emacs/elpa.git/plain/packages/adaptive-wrap/adaptive-wrap.el

   Then run emacs -L /path/to/adaptive-wrap-dir.


Thanks in advance for your help.