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

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 41810 in the body.
You can then email your comments to 41810 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Thu, 11 Jun 2020 18:16:40 +0200

[Message part 1 (text/plain, inline)]

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.

[1-extra-indent.png (image/png, attachment)]

[Message part 3 (text/plain, inline)]

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.)

[2-extend-t-current.png (image/png, attachment)]

[Message part 5 (text/plain, inline)]

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.

[3-extend-t-patched.png (image/png, attachment)]

[Message part 7 (text/plain, inline)]

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:

[extend.patch (text/x-patch, inline)]

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

[Message part 9 (text/plain, inline)]

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

Message #8 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 41810 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Thu, 11 Jun 2020 18:42:26 -0400

> 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

Message #11 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 41810 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Fri, 12 Jun 2020 10:50:06 +0200

Stefan Monnier <monnier <at> 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…

Message #14 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 41810 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Fri, 12 Jun 2020 11:33:25 -0400

> 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

Message #17 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 41810 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: 28.0.50; [ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Sat, 13 Jun 2020 00:48:09 +0200

[Message part 1 (text/plain, inline)]

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

Stefan Monnier <monnier <at> 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.

[poc.patch (text/x-patch, inline)]

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

[Message part 3 (text/plain, inline)]

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

[before.png (image/png, attachment)]

[after.png (image/png, attachment)]

[Message part 6 (text/plain, inline)]

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

Message #20 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: 41810 <at> debbugs.gnu.org
Cc: Stephen Berman <stephen.berman <at> gmx.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Sun, 21 Jun 2020 17:34:35 +0200

[Message part 1 (text/plain, inline)]

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

[patch1.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

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.

[patch1-diff-1.png (image/png, attachment)]

[patch1-diff-2.png (image/png, attachment)]

[patch1-nospace-1.png (image/png, attachment)]

[patch1-nospace-2.png (image/png, attachment)]

[Message part 8 (text/plain, inline)]

Screenshots generated with the following scripts:

[repro.el (text/x-emacs-lisp, attachment)]

[repro.sh (application/x-shellscript, attachment)]

[Message part 11 (text/plain, inline)]

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.

[patch2.patch (text/x-patch, attachment)]

[Message part 13 (text/plain, inline)]

Screenshots:

[patch2-nospace-1.png (image/png, attachment)]

[patch2-nospace-2.png (image/png, attachment)]

[Message part 16 (text/plain, inline)]

Thank you for your patience.

Message #23 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 41810 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Sun, 21 Jun 2020 19:32:17 +0100

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

Message #26 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 41810 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Mon, 22 Jun 2020 00:01:20 +0200

[Message part 1 (text/plain, inline)]

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

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

[patch1.patch (text/x-patch, attachment)]

[patch2.patch (text/x-patch, attachment)]

[Message part 4 (text/plain, inline)]

Thank you for the review!

Message #31 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 41810 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Fri, 14 Aug 2020 19:15:46 +0200

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

Message #38 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 41810 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Fri, 14 Aug 2020 19:58:21 +0200

Lars Ingebrigtsen <larsi <at> 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?

Message #41 received at 41810 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 41810 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#41810: [PATCH][ELPA] adaptive-wrap: Fontify wrap-prefix
Date: Fri, 14 Aug 2020 19:59:48 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.