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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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



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



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.



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:


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



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

1-extra-indent.png (69K) Download Attachment
2-extend-t-current.png (136K) Download Attachment
3-extend-t-patched.png (136K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kévin Le Gouguec
(Hit reply instead of followup; apologies for the duplicate, Stefan)

Stefan Monnier <[hidden email]> 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.


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


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




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


before.png (113K) Download Attachment
after.png (112K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

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



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.



Screenshots generated with the following scripts:



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.



Screenshots:



Thank you for your patience.

patch1.patch (4K) Download Attachment
patch1-diff-1.png (134K) Download Attachment
patch1-diff-2.png (134K) Download Attachment
patch1-nospace-1.png (80K) Download Attachment
patch1-nospace-2.png (124K) Download Attachment
repro.el (2K) Download Attachment
repro.sh (1K) Download Attachment
patch2.patch (1K) Download Attachment
patch2-nospace-1.png (79K) Download Attachment
patch2-nospace-2.png (122K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kévin Le Gouguec
"Basil L. Contovounesios" <[hidden email]> writes:

> Kévin Le Gouguec <[hidden email]> 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:



Thank you for the review!

patch1.patch (4K) Download Attachment
patch2.patch (1K) Download Attachment