bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

[Kévin Le Gouguec]

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

Hello,

I really enjoy the adaptive-wrap package, I find that it usually makes
long lines significantly more legible than plain visual-line-mode.

In some situations though, I think the wrap prefix could be improved.


1. When the fill prefix does not end with a space and extra-indent > 0
======================================================================

When extra indent is requested, the code uses the last character of
(fill-context-prefix beg end) as the padding character.

E.g. in *scratch*, from emacs -Q -rv:

#+begin_src elisp
(progn
  (package-initialize)
  (setq adaptive-wrap-extra-indent 2)
  (visual-line-mode)
  (adaptive-wrap-prefix-mode))
;;Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. 
#+end_src

See first screenshot.


[-- Attachment #2: 1-extra-indent.png --]
[-- Type: image/png, Size: 52042 bytes --]

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


The comment's continuation line starts with ";;;;".  I see two problems
with this:

1. The padding characters are not propertized, so we get two fontified
   semicolons, then two unfontified semicolons.

2. Visually, this looks sort of cluttered.  I have searched Debbugs and
   emacs-devel for a rationale for using (substring fcp -1) instead of
   unconditionally using spaces, but I could not find any.

Should we simplify adaptive-wrap-fill-context-prefix to stop bothering
with (substring fcp -1) and simply use " " for extra-indent?  If not,
should we at least propertize the padding characters, applying whatever
properties we find on (substring fcp -1)?

I can produce patches for either approach; I'd like to know what we'd
prefer first.


2. When wrapping lines that have an :extend t face
==================================================

See second screenshot, taken from emacs -Q -rv, with:

#+begin_src elisp
(progn
  (package-initialize)
  (global-visual-line-mode)
  (setq visual-line-fringe-indicators '(left-curly-arrow right-curly-arrow))
  (add-hook 'visual-line-mode-hook 'adaptive-wrap-prefix-mode)
  (setq-default adaptive-wrap-extra-indent 2))
#+end_src

(Ignore the fact that continuation lines are indented much further for
removed lines than for added lines, that's because adaptive-fill-regexp
matches -'s but not +'s.)


[-- Attachment #4: 2-extend-t-current.png --]
[-- Type: image/png, Size: 102253 bytes --]

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


One of the reasons to use :extend t faces, IMO, is to make it easier for
the eye to delineate horizontal chunks (e.g. "hunk headers", "added
lines", "removed lines" in diff-mode).  The unfontified prefix inserted
by adaptive-wrap makes the overall result visually confusing to me.

In these situations I'd like the wrap prefix to have the same background
color as the extended background: see third screenshot.


[-- Attachment #6: 3-extend-t-patched.png --]
[-- Type: image/png, Size: 102156 bytes --]

[-- Attachment #7: Type: text/plain, Size: 1923 bytes --]


I can't think of a robust way to determine this color though.  In these
diff-mode examples, the string returned by fill-context-prefix has no
properties, so we can't use that as source of truth.

As demonstrated in the screenshot, we could look at the face at EOL and
slap that onto the wrap prefix if it has :extend t, but I wonder if
there could be situations where this heuristic would break down[1].

I'm not suggesting to apply the patch shown in the screenshots as-is[2];
I just cobbled it up to show what I'd like the wrap-prefix to look like.


Let me know if any of what I wrote was unclear or confusing.  I would be
more than happy to work on patches for both issues; I'd just like to
know what resolution people would prefer for the first issue, and what
traps I should watch out for with the second issue.

Thank you for your time.


PS: Congratulations on the Debian inclusion ;)

https://lists.debian.org/debian-devel/2020/06/msg00065.html
https://salsa.debian.org/emacsen-team/adaptive-wrap-el


[1] E.g.

    here is a line that is wrapped,  ⤸
⤹     its continuation lines indented⤸
⤹     with 2 extra-indent spaces.

If only the final chars "spaces.\n" have an :extend t face, and we
naively apply (plist-get (text-properties-at (1- end)) 'face) onto the
wrap prefix, I guess the overall result could look psychedelic:
unardorned first line, then colored wrap-prefix, then unadorned
continuation line, colored wrap-prefix again, unadorned continuation
line again, then the final word, with a background that extends beyond
EOL.

(Maybe the answer is simply that :extend t faces are generally applied
to the whole line, and we shouldn't worry about such hypothetical
situations…  Those sound like famous-last-words material though.)

(I tried to materialize this hypothetical bug, to no avail.)

[2] Though here it is if anyone wants to comment:


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

diff --git a/packages/adaptive-wrap/adaptive-wrap.el b/packages/adaptive-wrap/adaptive-wrap.el
index f8d89ac69..5a23e6d6b 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -81,8 +81,12 @@ extra indent = 2
      ((= 0 adaptive-wrap-extra-indent)
       fcp)
      ((< 0 adaptive-wrap-extra-indent)
-      (concat fcp
-              (make-string adaptive-wrap-extra-indent fill-char)))
+      (let ((face (plist-get (text-properties-at (1- end)) 'face))
+	    (prefix (concat fcp
+			    (make-string adaptive-wrap-extra-indent fill-char))))
+	(if (and face (face-extend-p face))
+	    (propertize prefix 'face face)
+	  prefix)))
      ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
       (substring fcp
                  0

[-- Attachment #9: Type: text/plain, Size: 850 bytes --]



In GNU Emacs 28.0.50 (build 32, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0)
 of 2020-05-30 built on my-little-tumbleweed
Repository revision: 780f674a82a90c4e3e32583059b498bfa57e4a06
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: openSUSE Tumbleweed

Configured using:
 'configure --with-xwidgets --with-cairo'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS XWIDGETS JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  value of $LC_TIME: en_GB.UTF-8
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix

bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

[Stefan Monnier]

> The comment's continuation line starts with ";;;;".  I see two problems
> with this:
>
> 1. The padding characters are not propertized, so we get two fontified
>    semicolons, then two unfontified semicolons.

That looks ugly, indeed.  IIRC the reason is that when we extract the
prefix from the buffer, font-lock hasn't applied its `face` text
property yet.

> 2. Visually, this looks sort of cluttered.  I have searched Debbugs and
>    emacs-devel for a rationale for using (substring fcp -1) instead of
>    unconditionally using spaces, but I could not find any.

I think it just seemed like a good idea.  I suspect it's a matter of taste.
Not sure if it's important enough to justify offering both behaviors.

> See second screenshot, taken from emacs -Q -rv, with:

That's ugly, indeed.  Not sure whether the problem really comes from nor
where it should be fixed, but it's clearly a bug.


        Stefan

bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

[Kévin Le Gouguec]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The comment's continuation line starts with ";;;;".  I see two problems
>> with this:
>>
>> 1. The padding characters are not propertized, so we get two fontified
>>    semicolons, then two unfontified semicolons.
>
> That looks ugly, indeed.  IIRC the reason is that when we extract the
> prefix from the buffer, font-lock hasn't applied its `face` text
> property yet.

Or has it?  The string returned by fill-context-prefix *has* the correct
face, that's why the first two semicolons are fontified IIUC; only the
*extra* padding characters are unfontified, those that we generate with:

#+begin_src
;; Reconstructed from `adaptive-wrap-fill-context-prefix':
(make-string
 adaptive-wrap-extra-indent
 (string-to-char (substring (fill-context-prefix beg end) -1)))
#+end_src

I think font-lock is not to blame here, if we want those extra
characters to be fontified, we'll have to apply the face ourselves…

>> 2. Visually, this looks sort of cluttered.  I have searched Debbugs and
>>    emacs-devel for a rationale for using (substring fcp -1) instead of
>>    unconditionally using spaces, but I could not find any.
>
> I think it just seemed like a good idea.  I suspect it's a matter of taste.
> Not sure if it's important enough to justify offering both behaviors.

Mmm.  Well obviously, I'm biased toward unconditionally using spaces for
the extra-indent characters, so the resolutions I can imagine, in
decreasing order of personal preference, are:

1. Consensus that letting continuation lines breathe is optimal
   ⇒ spaces

2. Agreement that it's a matter of taste
   ⇒ defcustom accepting a char or a symbol (last-fcp-char?), defaulting
     to the latter

3. "There is a very good reason for repeating the last
    fill-context-prefix character extra-indent times: for example,
    consider the case when…"
   ⇒ OK then!

Honestly, as much as I'd like spaces, I'd settle for (substring fcp -1)
as long as we fix the fontification problem.

>> See second screenshot, taken from emacs -Q -rv, with:
>
> That's ugly, indeed.  Not sure whether the problem really comes from nor
> where it should be fixed, but it's clearly a bug.

I think I narrowed it down to this condition in fill-context-prefix:

#+begin_src
		   (if (or (and first-line-regexp
				(string-match first-line-regexp
					      first-line-prefix))
			   (and comment-start-skip
				(string-match comment-start-skip
					      first-line-prefix)))
		       first-line-prefix
		     (make-string (string-width first-line-prefix) ?\s))
#+end_src

In the *scratch* buffer, the condition holds true, so first-line-prefix
is returned, text properties and all: that's why the first two
semicolons are fontified.

In a diff buffer,

1. for removed lines, the condition is false, so we make a new,
   unfontified string, without the diff-removed face,

2. for added lines and headers, there is no prefix at all, so
   fill-context-prefix has nothing to tell us about what faces to apply.

I don't know if the fix belongs in fill-context-prefix, or if it should
be adaptive-wrap-fill-context-prefix's job to fixup faces…

bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

[Stefan Monnier]

> Or has it?  The string returned by fill-context-prefix *has* the correct
> face, that's why the first two semicolons are fontified IIUC; only the
> *extra* padding characters are unfontified, those that we generate with:
>
> #+begin_src
> ;; Reconstructed from `adaptive-wrap-fill-context-prefix':
> (make-string
>  adaptive-wrap-extra-indent
>  (string-to-char (substring (fill-context-prefix beg end) -1)))
> #+end_src

Ah, indeed, it's because we go through `string-to-char` which has no way
to preserve the text properties.
Yes, we should fix that to just concatenate `adaptive-wrap-extra-indent`
times the string returned by (substring (fill-context-prefix beg end) -1).

> I think I narrowed it down to this condition in fill-context-prefix:
>
> #+begin_src
> 		   (if (or (and first-line-regexp
> 				(string-match first-line-regexp
> 					      first-line-prefix))
> 			   (and comment-start-skip
> 				(string-match comment-start-skip
> 					      first-line-prefix)))
> 		       first-line-prefix
> 		     (make-string (string-width first-line-prefix) ?\s))
> #+end_src
>
> In the *scratch* buffer, the condition holds true, so first-line-prefix
> is returned, text properties and all: that's why the first two
> semicolons are fontified.
>
> In a diff buffer,
>
> 1. for removed lines, the condition is false, so we make a new,
>    unfontified string, without the diff-removed face,

We should be able to make this work by trying to preserve
`first-line-prefix`s text properties somehow.

> 2. for added lines and headers, there is no prefix at all, so
>    fill-context-prefix has nothing to tell us about what faces to apply.
>
> I don't know if the fix belongs in fill-context-prefix, or if it should
> be adaptive-wrap-fill-context-prefix's job to fixup faces…

I don't think it could be fixed in `fill-context-prefix`.

I guess we could try and fix it in `adaptive-wrap-fill-context-prefix`
by trying to preserve any face that covers the whole line (including the
final newline).

I'm glad I'm not the one who'll write the code ;-)


        Stefan

bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix

[Kévin Le Gouguec]

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

(Hit reply instead of followup; apologies for the duplicate, Stefan)

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I think I narrowed it down to this condition in fill-context-prefix:
>>
>> <snip>
>>
>> In the *scratch* buffer, the condition holds true, so first-line-prefix
>> is returned, text properties and all: that's why the first two
>> semicolons are fontified.
>>
>> In a diff buffer,
>>
>> 1. for removed lines, the condition is false, so we make a new,
>>    unfontified string, without the diff-removed face,
>
> We should be able to make this work by trying to preserve
> `first-line-prefix`s text properties somehow.

I wonder if we shouldn't go the other direction?  As in, why should
fill-context-prefix bother returning text properties?  From a quick
glance at other users of this function in the Emacs source tree, AFAICT
most of them actually insert (something based on) the return value, so
fontification is updated after insertion; these users do not care about
the returned text properties.

So a conclusion could be that adaptive-wrap-f-c-p should make no
assumptions about what text properties f-c-p returns, and determine the
face… some other way (see below).

> I guess we could try and fix it in `adaptive-wrap-fill-context-prefix`
> by trying to preserve any face that covers the whole line (including the
> final newline).

Mmm… I think that won't DTRT in some cases.  In diff buffers, typically:

- the first character in a removed line has the diff-indicator-removed
  face, which some themes[1] might customize to have no background,

- the rest of the line has the diff-removed face.

> I'm glad I'm not the one who'll write the code ;-)

I certainly wouldn't mind anyone beating me to it ;D

Here's a proof-of-concept patch (which only handles the positive
extra-indent case) and some before/after screenshots.  Again, not
suggesting to apply this patch as-is; this is just to show in which
direction I'm thinking of going:

1. if (substring fcp -1) has text properties, grab that,
2. else grab some text properties from the current line,
3. slap whatever we grabbed on the whole wrap-prefix.

Step 2 is not very well-defined yet, even though the "current
implementation" works well enough for the sales-pitch screenshots.

The rationale behind propertizing the whole prefix in step 3, as
mentioned a few paragraphs above, is to stop relying on
fill-context-prefix returning text properties.


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

diff --git a/packages/adaptive-wrap/adaptive-wrap.el b/packages/adaptive-wrap/adaptive-wrap.el
index f8d89ac69..0d9ed8b94 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -59,6 +59,11 @@ extra indent = 2
   :group 'visual-line)
 (make-variable-buffer-local 'adaptive-wrap-extra-indent)
 
+(defun adaptive-wrap--prefix-properties (fcp beg)
+  (or (and (> (string-width fcp) 0)
+           (text-properties-at 0 (substring fcp -1)))
+      (text-properties-at beg)))
+
 (defun adaptive-wrap-fill-context-prefix (beg end)
   "Like `fill-context-prefix', but with length adjusted by `adaptive-wrap-extra-indent'."
   (let* ((fcp
@@ -81,8 +86,9 @@ extra indent = 2
      ((= 0 adaptive-wrap-extra-indent)
       fcp)
      ((< 0 adaptive-wrap-extra-indent)
-      (concat fcp
-              (make-string adaptive-wrap-extra-indent fill-char)))
+      (apply #'propertize
+             (concat fcp (make-string adaptive-wrap-extra-indent fill-char))
+             (adaptive-wrap--prefix-properties fcp beg)))
      ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
       (substring fcp
                  0

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


The screenshots show improvements for the diff buffer (all leading
whitespace fontified) and for the comments in the scratch buffer (all
semicolons fontified).


[-- Attachment #4: before.png --]
[-- Type: image/png, Size: 84827 bytes --]

[-- Attachment #5: after.png --]
[-- Type: image/png, Size: 84039 bytes --]

[-- Attachment #6: Type: text/plain, Size: 246 bytes --]



Thank you for your time.  I'll try to followup with a more complete
patch Soonish™ (though I'm not sure what heuristic I'll use for step 2
yet).


[1] https://gitlab.com/peniblec/eighters-theme/-/blob/55710346/eighters-theme.el#L53

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

[Kévin Le Gouguec]

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

OK, here is a patch that I think should be good to push, tested against
Emacs 28 and 26.3.


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

From bcb32db22a65d90422aed5255e665356e50f2e49 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] 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 | 64 ++++++++++++++++++-------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/packages/adaptive-wrap/adaptive-wrap.el b/packages/adaptive-wrap/adaptive-wrap.el
index f8d89ac69..ed4fed900 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -59,6 +59,47 @@ 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)
+  (or (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.
+      (when (= (char-before end) ?\n)
+        (let ((eol-face (get-text-property (1- end) 'face)))
+          (when (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))
+                      ?\ ))))
+     ((< 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 +113,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: 231 bytes --]


Some before/after screenshots:

- patch1-diff-1.png: regular diff,
- patch1-diff-2.png: diff with background-less indicator faces,
- patch1-nospace-1.png: when (substring fcp -1) is not a space,
- patch1-nospace-2.png: likewise.


[-- Attachment #4: patch1-diff-1.png --]
[-- Type: image/png, Size: 100448 bytes --]

[-- Attachment #5: patch1-diff-2.png --]
[-- Type: image/png, Size: 100489 bytes --]

[-- Attachment #6: patch1-nospace-1.png --]
[-- Type: image/png, Size: 60481 bytes --]

[-- Attachment #7: patch1-nospace-2.png --]
[-- Type: image/png, Size: 93031 bytes --]

[-- Attachment #8: Type: text/plain, Size: 52 bytes --]


Screenshots generated with the following scripts:


[-- Attachment #9: repro.el --]
[-- Type: text/x-emacs-lisp, Size: 2436 bytes --]

(defun bug41810-setup ()
  (require 'adaptive-wrap)
  (add-hook 'visual-line-mode-hook 'adaptive-wrap-prefix-mode)
  (setq-default adaptive-wrap-extra-indent 2)
  (setq visual-line-fringe-indicators '(left-curly-arrow right-curly-arrow))
  (global-visual-line-mode))

(defun bug41810-teardown (screenshot)
  (text-scale-increase 2)
  ;; AFAICT unless we force redisplay, ImageMagick only captures the
  ;; *scratch* buffer.

  ;; Also, sometimes the scroll bar refuses to be drawn.  I've tried
  ;; various permutations of sit-for, redraw-frame, redraw-display,
  ;; force-window-update…  The following is the most "robust" way I
  ;; found to have the scroll bar show up and smile for the camera.

  ;; tl;dr I have no idea what I'm doing 💻🐾👔🐕
  (redisplay)
  (sleep-for 0.1)
  (redisplay)
  (call-process "magick" nil nil nil
                "import"
                "-window" (frame-parameter (selected-frame) 'window-id)
                "-frame"
                (expand-file-name
                 (concat
                  (buffer-local-value 'default-directory (get-buffer "*scratch*"))
                  screenshot)))
  (kill-emacs))

(defmacro defexample-bug41810 (name &rest body)
  (declare (indent defun))
  (list 'defun (intern (format "bug41810-%s" name)) '(description)
        '(bug41810-setup)
        `(progn
           ,@body)
        `(bug41810-teardown
          (format "%s-%s.png" ,(symbol-name name) description))))

(defexample-bug41810 nospace-1
  (find-library "cl-indent")
  (goto-char (point-max))
  (recenter -1))

(defexample-bug41810 nospace-2
  (switch-to-buffer "*example*")
  (url-insert-file-contents "https://code.orgmode.org/bzg/worg/raw/master/worgmap.org")
  (org-mode))

(defexample-bug41810 diff-1
  (switch-to-buffer "*example*")
  (url-insert-file-contents "https://git.savannah.gnu.org/cgit/emacs.git/patch/?id=be5d0c0f63081b5aee5efe2fbcc5c4ace6ca9a02")
  (diff-mode)
  (search-forward "diff --git")
  (recenter 0))

(defexample-bug41810 diff-2
  (switch-to-buffer "*example*")
  (url-insert-file-contents "https://git.savannah.gnu.org/cgit/emacs.git/patch/?id=be5d0c0f63081b5aee5efe2fbcc5c4ace6ca9a02")
  (diff-mode)
  (set-face-background 'diff-indicator-added (face-background 'default))
  (set-face-background 'diff-indicator-removed (face-background 'default))
  (search-forward "diff --git")
  (recenter 0))

[-- Attachment #10: repro.sh --]
[-- Type: application/x-shellscript, Size: 775 bytes --]

[-- Attachment #11: Type: text/plain, Size: 1029 bytes --]


Open questions:

- Since "check that a face spans the whole line" is neither
  straightforward nor sufficient (cf. diff-mode), I went with a fairly
  naive heuristic.  If anyone wants to describe a more sensible
  algorithm, or point out counter-examples where this logic breaks down,
  I'm all ears!

- The (or … (when … (let … (when (and …))))) chain looks clumsy but I
  don't really know how to improve it off the top of my head.  Maybe a
  when-let or two would help?  That'd mean requiring Emacs 25.1 though.

- (More of a nerd-snipe than an actual question, and definitely not
  related to this bug report, but if any expert on redisplay can look at
  bug41810-teardown in repro.el and tell me what is up with those pesky
  scroll bars, I'd be very grateful.)

Finally, I'd like to suggest this second patch to apply on top of the
first one.  I know there is no consensus that spaces are better than
(substring fcp -1), but I still can't think of a situation were the
latter looks better.


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

From 38202afa73e5612700d33ba4aa985e955f36ac02 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] 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 ed4fed900..ce54fd915 100644
--- a/packages/adaptive-wrap/adaptive-wrap.el
+++ b/packages/adaptive-wrap/adaptive-wrap.el
@@ -87,12 +87,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))
-                      ?\ ))))
+      (concat fcp (make-string adaptive-wrap-extra-indent ?\s)))
      ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
       (substring fcp
                  0
-- 
2.27.0


[-- Attachment #13: Type: text/plain, Size: 15 bytes --]


Screenshots:


[-- Attachment #14: patch2-nospace-1.png --]
[-- Type: image/png, Size: 59614 bytes --]

[-- Attachment #15: patch2-nospace-2.png --]
[-- Type: image/png, Size: 91835 bytes --]

[-- Attachment #16: Type: text/plain, Size: 30 bytes --]


Thank you for your patience.

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

[Basil L. Contovounesios]

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

> OK, here is a patch that I think should be good to push, tested against
> Emacs 28 and 26.3.

Thanks, just some minor comments from me.

> +(defun adaptive-wrap--prefix-face (fcp beg end)
> +  (or (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.
> +      (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.

> +          (when (and eol-face (adaptive-wrap--face-extends eol-face))
> +            eol-face)))))

Nit: Can't the when+and be replaced with a single and?

> +(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))
> +                      ?\ ))))

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

> +     ((< 0 (+ adaptive-wrap-extra-indent fcp-len))
> +      (substring fcp
> +                 0
> +                 (+ adaptive-wrap-extra-indent fcp-len)))
> +     (t
> +      ""))))

> Open questions:
>
> - The (or … (when … (let … (when (and …))))) chain looks clumsy but I
>   don't really know how to improve it off the top of my head.  Maybe a
>   when-let or two would help?  That'd mean requiring Emacs 25.1 though.

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

  (defun adaptive-wrap--prefix-face (fcp beg end)
    (or (get-text-property 0 'face fcp)
        (let ((face (and (= (char-before end) ?\n)
                         (get-text-property (1- end) 'face))))
          (and face (adaptive-wrap--face-extends face) face))))

or

  (defun adaptive-wrap--prefix-face (fcp beg end)
    (cond ((get-text-property 0 'face fcp))
          ((= (char-before end) ?\n)
           (let ((face ...))
             (and face (adaptive-wrap--face-extends face) face)))))

Thanks,

-- 
Basil

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

[Kévin Le Gouguec]

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

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

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


[-- 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 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 #4: Type: text/plain, Size: 27 bytes --]


Thank you for the review!

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

[Lars Ingebrigtsen]

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

> Updated patches:
>
>>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

I've applied these to ELPA, but haven't tested the code.  If somebody
could do that, I'd appreciate it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've applied these to ELPA, but haven't tested the code.  If somebody
> could do that, I'd appreciate it.

Thanks!  I'd love some additional testing as well; I've dogfooded
without issue so far, but that isn't much of a guarantee.

Maybe I should page in emacs-devel or help-gnu-emacs for feedback,
before we bump adaptive-wrap's version?

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

[Lars Ingebrigtsen]

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

> Maybe I should page in emacs-devel or help-gnu-emacs for feedback,
> before we bump adaptive-wrap's version?

Yup, that would be good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no