bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

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

(Note: I plan to merge this only after we cut the Emacs 30 release 
branch, since it seems a bit too substantial a change to sneak in right 
near the end. However, I think the patch is mostly done aside from one 
remaining issue, so any feedback is very welcome.)

'visual-wrap-prefix-mode' has one small issue: since the wrap prefix is 
just a string, the wrapped text may not line up for variable-width 
fonts. This is mainly in cases like so:

   * here is some text that
     got visually wrapped

If the "* " is variable-width, the second line will probably be indented 
wrong by a few pixels.

The attached patch adds a display spec in this case so that the text 
lines up perfectly. There's currently one problem though: I'm not sure 
how to regenerate the wrap prefix automatically if the face changes. 
It's not hard to handle for 'text-scale-adjust', but I don't know how to 
handle 'global-text-scale-adjust' (or other things that could change the 
face[1]).

Does anyone have any ideas for this part?

[1] There's 'after-setting-font-hook', but that doesn't cover everything 
either.

[-- Attachment #2: 0001-Add-support-for-variable-width-text-in-visual-wrap-p.patch --]
[-- Type: text/plain, Size: 9101 bytes --]

From 696a271601457f63dd7127261242e21432713402 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 16 Jun 2024 15:21:52 -0700
Subject: [PATCH] Add support for variable-width text in
 'visual-wrap-prefix-mode'

This uses a display spec to set the width correctly when indenting with
spaces.

* lisp/emacs-lisp/subr-x.el (string-pixel-width): New argument BUFFER.

* lisp/visual-wrap.el (visual-wrap--adjust-display-width)
(visual-wrap--content-prefix): New functions.
(visual-wrap--extra-indent): Rename from 'visual-wrap--prefix' and call
'visual-wrap--adjust-display-width'.
(visual-wrap-fill-context-prefix): Support display width.
(visual-wrap-prefix-function): Allow 'lbp' to be at 'point-min'.
(visual-wrap-prefix-mode): Refontify when changing text scale.

* doc/lispref/display.texi (Size of Displayed Text): Document BUFFER
argument for 'string-pixel-width'.

* etc/NEWS: Announce this change.
---
 doc/lispref/display.texi  |  6 ++--
 etc/NEWS                  |  8 ++++-
 lisp/emacs-lisp/subr-x.el | 11 ++++--
 lisp/visual-wrap.el       | 73 +++++++++++++++++++++++++++------------
 4 files changed, 70 insertions(+), 28 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index d5c96d13e02..52957f2ad07 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -2351,9 +2351,11 @@ Size of Displayed Text
 meaning as with @code{window-text-pixel-size}.
 @end defun
 
-@defun string-pixel-width string
+@defun string-pixel-width string &optional buffer
 This is a convenience function that uses @code{window-text-pixel-size}
-to compute the width of @var{string} (in pixels).
+to compute the width of @var{string} (in pixels).  If @var{buffer} is
+non-@code{nil}, use the face remappings from that buffer when
+determining the width (@pxref{Face Remapping}).
 @end defun
 
 @defun line-pixel-height
diff --git a/etc/NEWS b/etc/NEWS
index b2fdbc4a88f..27a4fd11a87 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -549,7 +549,8 @@ text in any way.  The global minor mode
 buffers.
 
 (This minor mode is the 'adaptive-wrap' ELPA package renamed and
-lightly edited for inclusion in Emacs.)
+enhanced for inclusion in Emacs.  It additionally supports prefixes for
+variable-width text.)
 
 +++
 ** New user option 'gud-highlight-current-line'.
@@ -2789,6 +2790,11 @@ These functions are like 'user-uid' and 'group-gid', respectively, but
 are aware of file name handlers, so they will return the remote UID or
 GID for remote files (or -1 if the connection has no associated user).
 
++++
+** 'string-pixel-width' now accepts a BUFFER argument.
+If BUFFER is non-nil, 'string-pixel-width' will apply BUFFER's face
+remappings when computing the string's width.
+
 +++
 ** 'fset', 'defalias' and 'defvaralias' now signal an error for cyclic aliases.
 Previously, 'fset', 'defalias' and 'defvaralias' could be made to
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 699be767ee7..2cbe1beb9f1 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -333,8 +333,10 @@ named-let
       . ,aargs)))
 
 ;;;###autoload
-(defun string-pixel-width (string)
-  "Return the width of STRING in pixels."
+(defun string-pixel-width (string &optional buffer)
+  "Return the width of STRING in pixels.
+If BUFFER is non-nil, use the face remappings from that buffer when
+determining the width."
   (declare (important-return-value t))
   (if (zerop (length string))
       0
@@ -348,6 +350,11 @@ string-pixel-width
       ;; Disable line-prefix and wrap-prefix, for the same reason.
       (setq line-prefix nil
 	    wrap-prefix nil)
+      (if buffer
+          (setq-local face-remapping-alist
+                      (with-current-buffer buffer
+                        face-remapping-alist))
+        (kill-local-variable 'face-remapping-alist))
       (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))
 
diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index d95cf4bb569..241cd337148 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -97,38 +97,60 @@ visual-wrap--prefix-face
                                  (if (visual-wrap--face-extend-p f) f))
                                eol-face)))))))
 
-(defun visual-wrap--prefix (fcp)
+(defun visual-wrap--adjust-display-width (fcp n)
+  (when-let ((display (get-text-property 0 'display fcp))
+             ((eq (car-safe display) 'space))
+             (width (car (plist-get (cdr display) :width))))
+    (put-text-property 0 (length fcp) 'display
+                       `(space :width (,(+ width n))) fcp))
+  fcp)
+
+(defun visual-wrap--extra-indent (fcp)
   (let ((fcp-len (string-width fcp)))
     (cond
      ((= 0 visual-wrap-extra-indent)
       fcp)
      ((< 0 visual-wrap-extra-indent)
-      (concat fcp (make-string visual-wrap-extra-indent ?\s)))
+      (let* ((extra (make-string visual-wrap-extra-indent ?\s))
+             (result (concat fcp extra)))
+        (visual-wrap--adjust-display-width
+         result (string-pixel-width extra (current-buffer)))))
      ((< 0 (+ visual-wrap-extra-indent fcp-len))
-      (substring fcp
-                 0
-                 (+ visual-wrap-extra-indent fcp-len)))
+      (let* ((idx (+ visual-wrap-extra-indent fcp-len))
+             (trim (substring fcp idx))
+             (result (substring fcp 0 idx)))
+        (remove-text-properties 0 (length trim) '(display) trim)
+        (visual-wrap--adjust-display-width
+         result (- (string-pixel-width trim (current-buffer))))))
      (t
       ""))))
 
+(defun visual-wrap--content-prefix (position)
+  "Get the content prefix for the line starting at POSITION.
+This is like `fill-content-prefix' but doesn't check subsequent lines
+and uses display specs to handle variable-width faces."
+  (save-excursion
+    (goto-char position)
+    (if (eolp) (forward-line 1))
+    ;; Move to the second line unless there is just one.
+    (move-to-left-margin)
+    (let ((prefix (fill-match-adaptive-prefix)))
+      (if (or (and adaptive-fill-first-line-regexp
+		   (string-match adaptive-fill-first-line-regexp prefix))
+	      (and comment-start-skip
+		   (string-match comment-start-skip prefix)))
+	  prefix
+        (propertize
+         (make-string (string-width prefix) ?\s)
+         'display `(space :width (,(string-pixel-width
+                                    prefix (current-buffer)))))))))
+
 (defun visual-wrap-fill-context-prefix (beg end)
   "Compute visual wrap prefix from text between BEG and END.
-This is like `fill-context-prefix', but with prefix length adjusted
-by `visual-wrap-extra-indent'."
-  (let* ((fcp
-          ;; `fill-context-prefix' ignores prefixes that look like
-          ;; paragraph starts, in order to avoid inadvertently
-          ;; creating a new paragraph while filling, but here we're
-          ;; only dealing with single-line "paragraphs" and we don't
-          ;; actually modify the buffer, so this restriction doesn't
-          ;; make much sense (and is positively harmful in
-          ;; taskpaper-mode where paragraph-start matches everything).
-          (or (let ((paragraph-start regexp-unmatchable))
-                    (fill-context-prefix beg end))
-                  ;; Note: fill-context-prefix may return nil; See:
-                  ;; http://article.gmane.org/gmane.emacs.devel/156285
-              ""))
-         (prefix (visual-wrap--prefix fcp))
+This is like `fill-context-prefix', but supporting variable-width faces
+and with the prefix length adjusted by `visual-wrap-extra-indent'."
+  (let* ((fcp (visual-wrap--content-prefix beg))
+         (prefix (visual-wrap--extra-indent fcp))
          (face (visual-wrap--prefix-face fcp beg end)))
     (if face
         (propertize prefix 'face face)
@@ -160,7 +182,8 @@ visual-wrap-prefix-function
 	 (remove-text-properties
 	  0 (length pfx) '(wrap-prefix) pfx)
          (let ((dp (get-text-property 0 'display pfx)))
-           (when (and dp (eq dp (get-text-property (1- lbp) 'display)))
+           (when (and dp (> lbp (point-min))
+                      (eq dp (get-text-property (1- lbp) 'display)))
              ;; There's a `display' property which covers not just the
              ;; prefix but also the previous newline.  So it's not
              ;; just making the prefix more pretty and could interfere
@@ -187,8 +210,12 @@ visual-wrap-prefix-mode
         ;; of the hook (bug#15155).
         (add-hook 'jit-lock-functions
                   #'visual-wrap-prefix-function 'append t)
-        (jit-lock-register #'visual-wrap-prefix-function))
+        (jit-lock-register #'visual-wrap-prefix-function)
+        ;; FIXME: What should we do about `global-text-scale-adjust' or
+        ;; other things that can change the text size?
+        (add-hook 'text-scale-mode-hook #'jit-lock-refontify nil t))
     (jit-lock-unregister #'visual-wrap-prefix-function)
+    (remove-hook 'text-scale-mode-hook #'jit-lock-refontify)
     (with-silent-modifications
       (save-restriction
         (widen)
-- 
2.25.1

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Sun, 16 Jun 2024 19:56:44 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> (Note: I plan to merge this only after we cut the Emacs 30 release 
> branch, since it seems a bit too substantial a change to sneak in right 
> near the end. However, I think the patch is mostly done aside from one 
> remaining issue, so any feedback is very welcome.)

OK.

> 'visual-wrap-prefix-mode' has one small issue: since the wrap prefix is 
> just a string, the wrapped text may not line up for variable-width 
> fonts. This is mainly in cases like so:
> 
>    * here is some text that
>      got visually wrapped
> 
> If the "* " is variable-width, the second line will probably be indented 
> wrong by a few pixels.

It also means the line after "*", the one that begins with "here is",
will also move horizontally.  Isn't that a misfeature?  Perhaps this
mode should align the text to some fixed pixel-coordinate, in which
case changes in font should not matter?  Or am I missing something?

> The attached patch adds a display spec in this case so that the text 
> lines up perfectly.

Can you explain the idea of the patch?  I don't think I understand why
you use '(space :width)' rather than '(space :align-to)'.

> There's currently one problem though: I'm not sure 
> how to regenerate the wrap prefix automatically if the face changes. 
> It's not hard to handle for 'text-scale-adjust', but I don't know how to 
> handle 'global-text-scale-adjust' (or other things that could change the 
> face[1]).
> 
> Does anyone have any ideas for this part?

Perhaps we could provide a function "face-change (&optional frame)"
which would access the frame's face_change flag and the global
face_change flag.  Then you could test those in a post-command-hook or
somesuch.  (However, using :align-to, if feasible, sounds like a
better solution to me.)

> -@defun string-pixel-width string
> +@defun string-pixel-width string &optional buffer
>  This is a convenience function that uses @code{window-text-pixel-size}
> -to compute the width of @var{string} (in pixels).
> +to compute the width of @var{string} (in pixels).  If @var{buffer} is
> +non-@code{nil}, use the face remappings from that buffer when
> +determining the width (@pxref{Face Remapping}).

An alternative would be to provide a face to use.

In any case, using BUFFER only for face-remapping-alist is only a
small part of what a buffer can do to a string: there's the major mode
with its fontifications and whatnot.

> +(defun string-pixel-width (string &optional buffer)
> +  "Return the width of STRING in pixels.
> +If BUFFER is non-nil, use the face remappings from that buffer when
> +determining the width."
>    (declare (important-return-value t))
>    (if (zerop (length string))
>        0
> @@ -348,6 +350,11 @@ string-pixel-width
>        ;; Disable line-prefix and wrap-prefix, for the same reason.
>        (setq line-prefix nil
>  	    wrap-prefix nil)
> +      (if buffer
          ^^^^^^^^^
This should test buffer-live-p, I think, not just buffer non-nil.

> +(defun visual-wrap--adjust-display-width (fcp n)
> +  (when-let ((display (get-text-property 0 'display fcp))
> +             ((eq (car-safe display) 'space))

Doesn't this only work with very simple 'display' specs?  The 'space'
part could be in some place deep in the spec, not just the second
symbol.

>  (defun visual-wrap-fill-context-prefix (beg end)
>    "Compute visual wrap prefix from text between BEG and END.
> -This is like `fill-context-prefix', but with prefix length adjusted
> -by `visual-wrap-extra-indent'."
> -  (let* ((fcp
> -          ;; `fill-context-prefix' ignores prefixes that look like
> -          ;; paragraph starts, in order to avoid inadvertently
> -          ;; creating a new paragraph while filling, but here we're
> -          ;; only dealing with single-line "paragraphs" and we don't
> -          ;; actually modify the buffer, so this restriction doesn't
> -          ;; make much sense (and is positively harmful in
> -          ;; taskpaper-mode where paragraph-start matches everything).
> -          (or (let ((paragraph-start regexp-unmatchable))
> -                    (fill-context-prefix beg end))
> -                  ;; Note: fill-context-prefix may return nil; See:
> -                  ;; http://article.gmane.org/gmane.emacs.devel/156285
> -              ""))

The comment above and the URL it included are deleted: is that because
they are no longer relevant?  If not, maybe move them with the code,
so that the information is not lost.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

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

On 6/17/2024 4:37 AM, Eli Zaretskii wrote:
>> Date: Sun, 16 Jun 2024 19:56:44 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>> The attached patch adds a display spec in this case so that the text
>> lines up perfectly.
> 
> Can you explain the idea of the patch?  I don't think I understand why
> you use '(space :width)' rather than '(space :align-to)'.

I tried using :align-to, and it doesn't seem to take effect for the 
'wrap-prefix' text property. I haven't looked closely at why that 
doesn't work, but even if it did, I think it might make things more 
complex than they already are.

I'll try to describe the current process:

1. 'visual-wrap-prefix-mode' goes a line at a time, finding the 
first-line prefix (for a bulleted item, this is something like "* ").
2. Then it constructs the wrap-prefix string (for a bulleted item, 
something like "  "; for other items it might be the same as the 
first-line prefix).
3. Finally, it applies the the wrap-prefix to the entire line it's 
examining.

The problem comes up for variable-pitch fonts, where "* " and "  " have 
different pixel widths. Before my patch, this results in the second line 
not lining up correctly. See the attached image for an example.

My patch just sets a display-spec on the "  " to make it have the same 
pixel-width as "* ". Then it all lines up.

If I understand your :align-to suggestion, setting :align-to on 
everything after the "* " bullet could work in theory, but I don't know 
what value you could set there to make everything correct. If it's a 
fixed number of pixels, then scaling up the text could mean the "* " 
becomes too wide for the space we reserved for it, and then things would 
probably look wrong. If it's based on the canonical character width, 
that might work so long as that updates when needed, but it might still 
look off depending on how the canonical width and the pixel width 
compare. (Ideally, we'd align to the exact pixel-width of "* " or 
whatever the first-line prefix is.) I couldn't get :align-to to work in 
the first place though so this is all hypothetical...

>> There's currently one problem though: I'm not sure
>> how to regenerate the wrap prefix automatically if the face changes.
>> It's not hard to handle for 'text-scale-adjust', but I don't know how to
>> handle 'global-text-scale-adjust' (or other things that could change the
>> face[1]).
>>
>> Does anyone have any ideas for this part?
> 
> Perhaps we could provide a function "face-change (&optional frame)"
> which would access the frame's face_change flag and the global
> face_change flag.  Then you could test those in a post-command-hook or
> somesuch.  (However, using :align-to, if feasible, sounds like a
> better solution to me.)

The 'face-change' idea could work, or here's another possibility: what 
about using :relative-width? If I set that correctly, then the 
pixel-size should adjust as the text scales. It wouldn't handle the case 
where the actual font changes though. It would also have some loss of 
precision, but I tested out a hacky patch using :relative-width and it 
looks good in practice.

>> -@defun string-pixel-width string
>> +@defun string-pixel-width string &optional buffer
>>   This is a convenience function that uses @code{window-text-pixel-size}
>> -to compute the width of @var{string} (in pixels).
>> +to compute the width of @var{string} (in pixels).  If @var{buffer} is
>> +non-@code{nil}, use the face remappings from that buffer when
>> +determining the width (@pxref{Face Remapping}).
> 
> An alternative would be to provide a face to use.
> 
> In any case, using BUFFER only for face-remapping-alist is only a
> small part of what a buffer can do to a string: there's the major mode
> with its fontifications and whatnot.

Yeah, I'm not entirely happy with this BUFFER argument either. I don't 
think we need to worry about fontification in this case though, since 
you can pass in a fontified string.

Maybe this should take the face-remapping-alist directly? Or maybe we 
should pass in a window? The latter might be better for handling things 
like frame-specific font settings. (Although as Po Lu points out, 
frame-specific fonts are challenging to handle correctly here.)

>> +(defun visual-wrap--adjust-display-width (fcp n)
>> +  (when-let ((display (get-text-property 0 'display fcp))
>> +             ((eq (car-safe display) 'space))
> 
> Doesn't this only work with very simple 'display' specs?  The 'space'
> part could be in some place deep in the spec, not just the second
> symbol.

Yeah, though the FCP argument is always the prefix we constructed, so we 
know what the display-spec looks like if it's present. The extra checks 
are just my natural paranoia. I've added a comment here explaining though.

>>   (defun visual-wrap-fill-context-prefix (beg end)
>>     "Compute visual wrap prefix from text between BEG and END.
>> -This is like `fill-context-prefix', but with prefix length adjusted
>> -by `visual-wrap-extra-indent'."
>> -  (let* ((fcp
>> -          ;; `fill-context-prefix' ignores prefixes that look like
>> -          ;; paragraph starts, in order to avoid inadvertently
>> -          ;; creating a new paragraph while filling, but here we're
>> -          ;; only dealing with single-line "paragraphs" and we don't
>> -          ;; actually modify the buffer, so this restriction doesn't
>> -          ;; make much sense (and is positively harmful in
>> -          ;; taskpaper-mode where paragraph-start matches everything).
>> -          (or (let ((paragraph-start regexp-unmatchable))
>> -                    (fill-context-prefix beg end))
>> -                  ;; Note: fill-context-prefix may return nil; See:
>> -                  ;; http://article.gmane.org/gmane.emacs.devel/156285
>> -              ""))
> 
> The comment above and the URL it included are deleted: is that because
> they are no longer relevant?  If not, maybe move them with the code,
> so that the information is not lost.

Correct, they're no longer relevant. I extracted the logic that we need 
out of 'fill-content-prefix' and into 'visual-wrap--content-prefix'. The 
former didn't behave quite the way we wanted (hence all the comments), 
and it made handling the display-spec parts of my patch even harder, so 
I just took the relevant logic out and made a function that does exactly 
what we want. I've added more detail to the commit message explaining 
the change.

[-- Attachment #2: misaligned.png --]
[-- Type: image/png, Size: 24816 bytes --]

[-- Attachment #3: 0001-Add-support-for-variable-width-text-in-visual-wrap-p.patch --]
[-- Type: text/plain, Size: 9981 bytes --]

From 120748358b6a717c740e8d4f139ce62a30be7606 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 16 Jun 2024 15:21:52 -0700
Subject: [PATCH 1/2] Add support for variable-width text in
 'visual-wrap-prefix-mode'

This uses a display spec to set the width correctly when indenting with
spaces.

* lisp/emacs-lisp/subr-x.el (string-pixel-width): New argument BUFFER.

* lisp/visual-wrap.el (visual-wrap--content-prefix)
(visual-wrap--adjust-display-width): New functions.
(visual-wrap--extra-indent): Rename from 'visual-wrap--prefix' and call
'visual-wrap--adjust-display-width'.
(visual-wrap-fill-context-prefix): Support display width.  Use
'visual-wrap--content-prefix' instead of 'fill-content-prefix', which
lets us remove the old workarounds.
(visual-wrap-prefix-function): Allow 'lbp' to be at 'point-min'.
(visual-wrap-prefix-mode): Refontify when changing text scale.

* doc/lispref/display.texi (Size of Displayed Text): Document BUFFER
argument for 'string-pixel-width'.

* etc/NEWS: Announce this change.
---
 doc/lispref/display.texi  |  6 ++-
 etc/NEWS                  |  8 +++-
 lisp/emacs-lisp/subr-x.el | 11 ++++-
 lisp/visual-wrap.el       | 86 ++++++++++++++++++++++++++++-----------
 4 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index d5c96d13e02..52957f2ad07 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -2351,9 +2351,11 @@ Size of Displayed Text
 meaning as with @code{window-text-pixel-size}.
 @end defun
 
-@defun string-pixel-width string
+@defun string-pixel-width string &optional buffer
 This is a convenience function that uses @code{window-text-pixel-size}
-to compute the width of @var{string} (in pixels).
+to compute the width of @var{string} (in pixels).  If @var{buffer} is
+non-@code{nil}, use the face remappings from that buffer when
+determining the width (@pxref{Face Remapping}).
 @end defun
 
 @defun line-pixel-height
diff --git a/etc/NEWS b/etc/NEWS
index b2fdbc4a88f..27a4fd11a87 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -549,7 +549,8 @@ text in any way.  The global minor mode
 buffers.
 
 (This minor mode is the 'adaptive-wrap' ELPA package renamed and
-lightly edited for inclusion in Emacs.)
+enhanced for inclusion in Emacs.  It additionally supports prefixes for
+variable-width text.)
 
 +++
 ** New user option 'gud-highlight-current-line'.
@@ -2789,6 +2790,11 @@ These functions are like 'user-uid' and 'group-gid', respectively, but
 are aware of file name handlers, so they will return the remote UID or
 GID for remote files (or -1 if the connection has no associated user).
 
++++
+** 'string-pixel-width' now accepts a BUFFER argument.
+If BUFFER is non-nil, 'string-pixel-width' will apply BUFFER's face
+remappings when computing the string's width.
+
 +++
 ** 'fset', 'defalias' and 'defvaralias' now signal an error for cyclic aliases.
 Previously, 'fset', 'defalias' and 'defvaralias' could be made to
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 699be767ee7..2cbe1beb9f1 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -333,8 +333,10 @@ named-let
       . ,aargs)))
 
 ;;;###autoload
-(defun string-pixel-width (string)
-  "Return the width of STRING in pixels."
+(defun string-pixel-width (string &optional buffer)
+  "Return the width of STRING in pixels.
+If BUFFER is non-nil, use the face remappings from that buffer when
+determining the width."
   (declare (important-return-value t))
   (if (zerop (length string))
       0
@@ -348,6 +350,11 @@ string-pixel-width
       ;; Disable line-prefix and wrap-prefix, for the same reason.
       (setq line-prefix nil
 	    wrap-prefix nil)
+      (if buffer
+          (setq-local face-remapping-alist
+                      (with-current-buffer buffer
+                        face-remapping-alist))
+        (kill-local-variable 'face-remapping-alist))
       (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))
 
diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index d95cf4bb569..886219cff54 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -97,38 +97,73 @@ visual-wrap--prefix-face
                                  (if (visual-wrap--face-extend-p f) f))
                                eol-face)))))))
 
-(defun visual-wrap--prefix (fcp)
+(defun visual-wrap--content-prefix (position)
+  "Get the content prefix for the line starting at POSITION.
+This is like `fill-content-prefix' but doesn't check subsequent lines
+and uses display specs to handle variable-pitch faces."
+  (save-excursion
+    (goto-char position)
+    (if (eolp) (forward-line 1))
+    ;; Move to the second line unless there is just one.
+    (move-to-left-margin)
+    (let ((prefix (fill-match-adaptive-prefix)))
+      ;; Check whether we should use our first-line content prefix.
+      (if (or (and adaptive-fill-first-line-regexp
+		   (string-match adaptive-fill-first-line-regexp prefix))
+	      (and comment-start-skip
+		   (string-match comment-start-skip prefix)))
+	  prefix
+        ;; We want the prefix to be whitespace of the same width as the
+        ;; first-line prefix.
+        (let ((spaces (make-string (string-width prefix) ?\s)))
+          ;; If the font for our first-line prefix is variable-pitch,
+          ;; use a display spec to line the subsequent lines up
+          ;; correctly.
+          (when-let ((font (font-at position))
+                     ((memq (font-get font :spacing) '(nil 0))))
+            (put-text-property 0 (length spaces) 'display
+                               `(space :width (,(string-pixel-width
+                                                 prefix (current-buffer))))
+                               spaces))
+          spaces)))))
+
+(defun visual-wrap--adjust-display-width (fcp n)
+  (when-let ((display (get-text-property 0 'display fcp))
+             ;; If we have a display spec here, it should be what we
+             ;; specified in `visual-wrap--content-prefix', but
+             ;; double-check just to be safe.
+             ((eq (car-safe display) 'space))
+             (width (car (plist-get (cdr display) :width))))
+    (put-text-property 0 (length fcp) 'display
+                       `(space :width (,(+ width n))) fcp))
+  fcp)
+
+(defun visual-wrap--extra-indent (fcp)
   (let ((fcp-len (string-width fcp)))
     (cond
      ((= 0 visual-wrap-extra-indent)
       fcp)
      ((< 0 visual-wrap-extra-indent)
-      (concat fcp (make-string visual-wrap-extra-indent ?\s)))
+      (let* ((extra (make-string visual-wrap-extra-indent ?\s))
+             (result (concat fcp extra)))
+        (visual-wrap--adjust-display-width
+         result (string-pixel-width extra (current-buffer)))))
      ((< 0 (+ visual-wrap-extra-indent fcp-len))
-      (substring fcp
-                 0
-                 (+ visual-wrap-extra-indent fcp-len)))
+      (let* ((idx (+ visual-wrap-extra-indent fcp-len))
+             (trim (substring fcp idx))
+             (result (substring fcp 0 idx)))
+        (remove-text-properties 0 (length trim) '(display) trim)
+        (visual-wrap--adjust-display-width
+         result (- (string-pixel-width trim (current-buffer))))))
      (t
       ""))))
 
 (defun visual-wrap-fill-context-prefix (beg end)
   "Compute visual wrap prefix from text between BEG and END.
-This is like `fill-context-prefix', but with prefix length adjusted
-by `visual-wrap-extra-indent'."
-  (let* ((fcp
-          ;; `fill-context-prefix' ignores prefixes that look like
-          ;; paragraph starts, in order to avoid inadvertently
-          ;; creating a new paragraph while filling, but here we're
-          ;; only dealing with single-line "paragraphs" and we don't
-          ;; actually modify the buffer, so this restriction doesn't
-          ;; make much sense (and is positively harmful in
-          ;; taskpaper-mode where paragraph-start matches everything).
-          (or (let ((paragraph-start regexp-unmatchable))
-                    (fill-context-prefix beg end))
-                  ;; Note: fill-context-prefix may return nil; See:
-                  ;; http://article.gmane.org/gmane.emacs.devel/156285
-              ""))
-         (prefix (visual-wrap--prefix fcp))
+This is like `fill-context-prefix', but supporting variable-width faces
+and with the prefix length adjusted by `visual-wrap-extra-indent'."
+  (let* ((fcp (visual-wrap--content-prefix beg))
+         (prefix (visual-wrap--extra-indent fcp))
          (face (visual-wrap--prefix-face fcp beg end)))
     (if face
         (propertize prefix 'face face)
@@ -160,7 +195,8 @@ visual-wrap-prefix-function
 	 (remove-text-properties
 	  0 (length pfx) '(wrap-prefix) pfx)
          (let ((dp (get-text-property 0 'display pfx)))
-           (when (and dp (eq dp (get-text-property (1- lbp) 'display)))
+           (when (and dp (> lbp (point-min))
+                      (eq dp (get-text-property (1- lbp) 'display)))
              ;; There's a `display' property which covers not just the
              ;; prefix but also the previous newline.  So it's not
              ;; just making the prefix more pretty and could interfere
@@ -187,8 +223,12 @@ visual-wrap-prefix-mode
         ;; of the hook (bug#15155).
         (add-hook 'jit-lock-functions
                   #'visual-wrap-prefix-function 'append t)
-        (jit-lock-register #'visual-wrap-prefix-function))
+        (jit-lock-register #'visual-wrap-prefix-function)
+        ;; FIXME: What should we do about `global-text-scale-adjust' or
+        ;; other things that can change the text size?
+        (add-hook 'text-scale-mode-hook #'jit-lock-refontify nil t))
     (jit-lock-unregister #'visual-wrap-prefix-function)
+    (remove-hook 'text-scale-mode-hook #'jit-lock-refontify)
     (with-silent-modifications
       (save-restriction
         (widen)
-- 
2.25.1

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Mon, 17 Jun 2024 10:42:56 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/17/2024 4:37 AM, Eli Zaretskii wrote:
> >> Date: Sun, 16 Jun 2024 19:56:44 -0700
> >> From: Jim Porter <jporterbugs@gmail.com>
> >> The attached patch adds a display spec in this case so that the text
> >> lines up perfectly.
> > 
> > Can you explain the idea of the patch?  I don't think I understand why
> > you use '(space :width)' rather than '(space :align-to)'.
> 
> I tried using :align-to, and it doesn't seem to take effect for the 
> 'wrap-prefix' text property. I haven't looked closely at why that 
> doesn't work, but even if it did, I think it might make things more 
> complex than they already are.

What exactly did you try?  I might be misremembering, but I'm not
aware of any limitations wrt use of :align-to in wrap-prefix.  In
fact, the ELisp reference manual explicitly mentions :align-to in its
description of wrap-prefix.

> If I understand your :align-to suggestion, setting :align-to on 
> everything after the "* " bullet could work in theory, but I don't know 
> what value you could set there to make everything correct.

Some multiple of the width of the default font's representative
character, or of its average-width property?

> The 'face-change' idea could work, or here's another possibility: what 
> about using :relative-width?

:relative-width could work, but you'd need to make sure it takes the
width from a fixed character, otherwise different paragraphs won't
align the same.  The ELisp manual says:

  ‘:relative-width FACTOR’
       Specifies that the width of the stretch should be computed from the
       first character in the group of consecutive characters that have
       the same ‘display’ property.  The space width is the pixel width of
       that character, multiplied by FACTOR.


> If I set that correctly, then the pixel-size should adjust as the
> text scales. It wouldn't handle the case where the actual font
> changes though.

Why not?

> >> -@defun string-pixel-width string
> >> +@defun string-pixel-width string &optional buffer
> >>   This is a convenience function that uses @code{window-text-pixel-size}
> >> -to compute the width of @var{string} (in pixels).
> >> +to compute the width of @var{string} (in pixels).  If @var{buffer} is
> >> +non-@code{nil}, use the face remappings from that buffer when
> >> +determining the width (@pxref{Face Remapping}).
> > 
> > An alternative would be to provide a face to use.
> > 
> > In any case, using BUFFER only for face-remapping-alist is only a
> > small part of what a buffer can do to a string: there's the major mode
> > with its fontifications and whatnot.
> 
> Yeah, I'm not entirely happy with this BUFFER argument either. I don't 
> think we need to worry about fontification in this case though, since 
> you can pass in a fontified string.
> 
> Maybe this should take the face-remapping-alist directly? Or maybe we 
> should pass in a window?

If you can pass a window, you can use window-text-pixel-size instead.

> >> +(defun visual-wrap--adjust-display-width (fcp n)
> >> +  (when-let ((display (get-text-property 0 'display fcp))
> >> +             ((eq (car-safe display) 'space))
> > 
> > Doesn't this only work with very simple 'display' specs?  The 'space'
> > part could be in some place deep in the spec, not just the second
> > symbol.
> 
> Yeah, though the FCP argument is always the prefix we constructed, so we 
> know what the display-spec looks like if it's present.

Sure, but that means the code is fragile, and you need to comment
prominently that if the form of the display spec changes in the
future, this code will need to be adapted.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

Replying to your individual points below, but in the other subthread, I 
had the idea that maybe a better way to do this would be for the wrap 
prefix to always be the first-line prefix but to make it transparent 
when desired. So for the "* some text" example, the wrap-prefix would be 
"* " but fontified(?) such that you can't see it.

A face transparency attribute might do the trick, and be useful for 
other things too: 
<https://lists.gnu.org/archive/html/emacs-devel/2024-01/msg00657.html>. 
Or maybe :align-to could take a string value, which would mean "use the 
pixel-width of this string as the value".

On 6/17/2024 11:20 AM, Eli Zaretskii wrote:
>> Date: Mon, 17 Jun 2024 10:42:56 -0700
>> Cc: 71605@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> I tried using :align-to, and it doesn't seem to take effect for the
>> 'wrap-prefix' text property. I haven't looked closely at why that
>> doesn't work, but even if it did, I think it might make things more
>> complex than they already are.
> 
> What exactly did you try?  I might be misremembering, but I'm not
> aware of any limitations wrt use of :align-to in wrap-prefix.  In
> fact, the ELisp reference manual explicitly mentions :align-to in its
> description of wrap-prefix.

My minimal test case is to open a buffer, put some random text in (long 
enough that it would wrap), and then call:

   (put-text-property (point-min) (point-max) 'wrap-prefix '(space 
:align-to 4))

Nothing changes for me. If I replace :align-to with :width, I see the 
continuation lines get indented by 4 characters.

>> The 'face-change' idea could work, or here's another possibility: what
>> about using :relative-width?
> 
> :relative-width could work, but you'd need to make sure it takes the
> width from a fixed character, otherwise different paragraphs won't
> align the same.

In this case, the first character is always a space so that's ok.

>> If I set that correctly, then the pixel-size should adjust as the
>> text scales. It wouldn't handle the case where the actual font
>> changes though.
> 
> Why not?

I was planning to set :relative-width to <first_line_prefix_width> / 
<width_of_one_space>. If the font changes, the result of that 
calculation can change.

>>>> -@defun string-pixel-width string
>>>> +@defun string-pixel-width string &optional buffer
[snip]
>> Maybe this should take the face-remapping-alist directly? Or maybe we
>> should pass in a window?
> 
> If you can pass a window, you can use window-text-pixel-size instead.

I think 'window-text-pixel-size' would compute the size of the text 
already in the buffer, and I was looking for a function that told the 
width of some text if I were to later display it in that window. In any 
case, I might not need to use this function at all depending on how I do 
things...

>> Yeah, though the FCP argument is always the prefix we constructed, so we
>> know what the display-spec looks like if it's present.
> 
> Sure, but that means the code is fragile, and you need to comment
> prominently that if the form of the display spec changes in the
> future, this code will need to be adapted.

Assuming I keep going down this route, I'll be sure to comment this 
extensively.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Mon, 17 Jun 2024 11:44:47 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Replying to your individual points below, but in the other subthread, I 
> had the idea that maybe a better way to do this would be for the wrap 
> prefix to always be the first-line prefix but to make it transparent 
> when desired. So for the "* some text" example, the wrap-prefix would be 
> "* " but fontified(?) such that you can't see it.

I don't think I understand how would this do the job.  Surely, the
indentation space should be visible?

> A face transparency attribute might do the trick, and be useful for 
> other things too: 

It isn't universally supported, AFAIK.

> Or maybe :align-to could take a string value, which would mean "use the 
> pixel-width of this string as the value".

How is that different from using a column (as opposed to pixel) value
for :align-to?

> >> If I set that correctly, then the pixel-size should adjust as the
> >> text scales. It wouldn't handle the case where the actual font
> >> changes though.
> > 
> > Why not?
> 
> I was planning to set :relative-width to <first_line_prefix_width> / 
> <width_of_one_space>. If the font changes, the result of that 
> calculation can change.

The idea is to set it to the multiple of the character's width, which
will then scale with the font.

> > If you can pass a window, you can use window-text-pixel-size instead.
> 
> I think 'window-text-pixel-size' would compute the size of the text 
> already in the buffer

Yes, and we have with-current-buffer...

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/18/2024 4:37 AM, Eli Zaretskii wrote:
> I don't think I understand how would this do the job.  Surely, the
> indentation space should be visible?

I mean the second-line "*  " prefix would be visible but transparent.

>> A face transparency attribute might do the trick, and be useful for
>> other things too:
> 
> It isn't universally supported, AFAIK.

I think it would be feasible to support an opacity level of 1.0 and 0.0. 
opacity=0.0 could just allocate the space for the text but not actually 
draw the glyphs. (Whether we want to go this route is another question.)

>> Or maybe :align-to could take a string value, which would mean "use the
>> pixel-width of this string as the value".
> 
> How is that different from using a column (as opposed to pixel) value
> for :align-to?

A column wouldn't work, since for a variable-pitch font, N columns is 
just N * <canonical character width>. If the actual characters you're 
trying to align to are narrower than the canonical width, they won't 
line up correctly.

Po Lu also raised the issue that in some cases, different frames can be 
displaying the same buffer using different fonts. Conceptually, I'm 
really trying to tell the display engine, "Put a space here exactly as 
wide as <some text> using whatever font you end up using." At the buffer 
level, I can't provide a numeric width here that works everywhere, since 
it might really be multiple numbers, one for each frame.

Providing a number in pixels is also challenging because then I need to 
be able to determine when to recompute that number.

>>>> If I set that correctly, then the pixel-size should adjust as the
>>>> text scales. It wouldn't handle the case where the actual font
>>>> changes though.
>>>
>>> Why not?
>>
>> I was planning to set :relative-width to <first_line_prefix_width> /
>> <width_of_one_space>. If the font changes, the result of that
>> calculation can change.
> 
> The idea is to set it to the multiple of the character's width, which
> will then scale with the font.

Imagine two fonts A and B, where the only difference is that the space 
character in B is twice as wide. So:

     <asterisk_width> = 15
     <space_width_A> = 10
     <space_width_B> = 20
     <first_line_prefix_width_A> = 15 + 10 = 25
     <first_line_prefix_width_B> = 15 + 20 = 35

If I compute :relative-width for font A, the result is 25/10 = 2.5. Then,

     2.5 * <space_width_A> = 25  = <first_line_prefix_width_A>  (good)
     2.5 * <space_width_B> = 50 != <first_line_prefix_width_B>  (bad!)

So we'd need a way of keeping the width in-sync with any font changes.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Tue, 18 Jun 2024 15:17:29 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/18/2024 4:37 AM, Eli Zaretskii wrote:
> >> Or maybe :align-to could take a string value, which would mean "use the
> >> pixel-width of this string as the value".
> > 
> > How is that different from using a column (as opposed to pixel) value
> > for :align-to?
> 
> A column wouldn't work, since for a variable-pitch font, N columns is 
> just N * <canonical character width>. If the actual characters you're 
> trying to align to are narrower than the canonical width, they won't 
> line up correctly.

If both the first line of the paragraph and the rest are aligned on
that width, they will all line up.  IOW, the idea is to make the text
of all the lines :align-to to the same column number, like this:

  * some text
    some other text

Then you don't care about the actual pixel width of "* ".

> Po Lu also raised the issue that in some cases, different frames can be 
> displaying the same buffer using different fonts. Conceptually, I'm 
> really trying to tell the display engine, "Put a space here exactly as 
> wide as <some text> using whatever font you end up using." At the buffer 
> level, I can't provide a numeric width here that works everywhere, since 
> it might really be multiple numbers, one for each frame.

IMNSHO, this is over-engineering, as we already have the means to do
the same with existing features.  Asking the display engine to measure
and record somewhere the pixel width of a string is a significant
complication to how the display code works, because the display
routines can be called to start their job at any arbitrary place in
the buffer.

> Providing a number in pixels is also challenging because then I need to 
> be able to determine when to recompute that number.

Which is why I didn't suggest that.  I suggested columns because they
scale with text-scale-adjust.

> >>>> If I set that correctly, then the pixel-size should adjust as the
> >>>> text scales. It wouldn't handle the case where the actual font
> >>>> changes though.
> >>>
> >>> Why not?
> >>
> >> I was planning to set :relative-width to <first_line_prefix_width> /
> >> <width_of_one_space>. If the font changes, the result of that
> >> calculation can change.
> > 
> > The idea is to set it to the multiple of the character's width, which
> > will then scale with the font.
> 
> Imagine two fonts A and B, where the only difference is that the space 
> character in B is twice as wide. So:
> 
>      <asterisk_width> = 15
>      <space_width_A> = 10
>      <space_width_B> = 20
>      <first_line_prefix_width_A> = 15 + 10 = 25
>      <first_line_prefix_width_B> = 15 + 20 = 35
> 
> If I compute :relative-width for font A, the result is 25/10 = 2.5. Then,
> 
>      2.5 * <space_width_A> = 25  = <first_line_prefix_width_A>  (good)
>      2.5 * <space_width_B> = 50 != <first_line_prefix_width_B>  (bad!)
> 
> So we'd need a way of keeping the width in-sync with any font changes.

I don't understand the example.  If these are two different
paragraphs, then their indentation cannot be guaranteed to be the same
anyway, because no one can assure you all the paragraphs will use the
same font.  So whatever you mean by <width_of_one_space>, it will
always be different for different fonts, and I don't see how this can
be solved for an arbitrary combination of fonts.

So my suggestion is to go back to the simpler idea of using :align-to
with an absolute value, either in pixels or in columns (which AFAIR is
interpreted in units of the frame's default font's width), and if
needed augment that by recalculation when necessary.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/19/2024 4:45 AM, Eli Zaretskii wrote:
> If both the first line of the paragraph and the rest are aligned on
> that width, they will all line up.  IOW, the idea is to make the text
> of all the lines :align-to to the same column number, like this:
> 
>    * some text
>      some other text
> 
> Then you don't care about the actual pixel width of "* ".

Just so I'm sure I understand this: the idea is that the buffer's 
contents are "* some text some other text" and then we set the :align-to 
property on "some text some other text" so that the result is displayed 
like this?

      +---- Here is the align-to column
      v
   * _some text
   ___some other text

The underscores represent the space added by :align-to. In practice, the 
first line might not have any extra space from :align-to, but the 
pixel-width of "* " could be less than 2 * <canonical character width> 
for variable-pitch fonts.

I think that would work, provided :align-to were fixed so that it could 
handle wrapping again. (And so long as we're ok with there being a bit 
of extra space after the first-line prefix when using some fonts, but I 
don't see why that would be an issue.)

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Wed, 19 Jun 2024 12:53:33 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/19/2024 4:45 AM, Eli Zaretskii wrote:
> > If both the first line of the paragraph and the rest are aligned on
> > that width, they will all line up.  IOW, the idea is to make the text
> > of all the lines :align-to to the same column number, like this:
> > 
> >    * some text
> >      some other text
> > 
> > Then you don't care about the actual pixel width of "* ".
> 
> Just so I'm sure I understand this: the idea is that the buffer's 
> contents are "* some text some other text" and then we set the :align-to 
> property on "some text some other text" so that the result is displayed 
> like this?
> 
>       +---- Here is the align-to column
>       v
>    * _some text
>    ___some other text

Yes.

> The underscores represent the space added by :align-to. In practice, the 
> first line might not have any extra space from :align-to, but the 
> pixel-width of "* " could be less than 2 * <canonical character width> 
> for variable-pitch fonts.

The value calculated for :align-to needs to make sure that never
happens.

> I think that would work, provided :align-to were fixed so that it could 
> handle wrapping again.

Stay tuned.

> (And so long as we're ok with there being a bit of extra space after
> the first-line prefix when using some fonts, but I don't see why
> that would be an issue.)

We _should_ be ok with some extra white space, because that's what I
see in word processors all around me.  Their indentation of the
itemized lists always indents by more than just one SPC after the item
symbol (bullet or number or letter).

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/19/2024 9:58 PM, Eli Zaretskii wrote:
>> I think that would work, provided :align-to were fixed so that it could
>> handle wrapping again.
> 
> Stay tuned.

Thanks. No rush on this, since Emacs 30 takes priority over this small 
nice-to-have enhancement. (I do have a patch for EWW that would benefit 
from fixing this bug, but that too can wait.)

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Wed, 19 Jun 2024 22:37:33 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/19/2024 9:58 PM, Eli Zaretskii wrote:
> >> I think that would work, provided :align-to were fixed so that it could
> >> handle wrapping again.
> > 
> > Stay tuned.
> 
> Thanks. No rush on this, since Emacs 30 takes priority over this small 
> nice-to-have enhancement. (I do have a patch for EWW that would benefit 
> from fixing this bug, but that too can wait.)

Should be fixed now on the master branch.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/20/2024 2:58 AM, Eli Zaretskii wrote:
> Should be fixed now on the master branch.

Thanks, it all seems to work now.

I think I have a way forward to implement this that should avoid issues, 
though it's a bit more complex than I'd initially thought. I think there 
are three cases:


"* here is some text":
   Text                    Properties
   "* "                    (display (min-width (2)))
   "here is some text"     (wrap-prefix (space :align-to 2))

"  here is some text":
   Text                    Properties
   "  "                    (display "")
   "  here is some text"   (line-prefix (space :align-to 2)
                            wrap-prefix (space :align-to 2))

"// here is some text" (in c-mode):
   Text                    Properties
   "// here is some text"  (wrap-prefix "// ")


(The last case is just doing what visual-wrap-prefix-mode currently 
does.) I'm pretty sure all these should be unproblematic in the 
scenarios Po Lu mentioned. The only case I haven't considered is how 
this interacts with 'visual-wrap-extra-indent'. I'll have to think about 
that some more.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Thu, 20 Jun 2024 10:36:23 -0700
> Cc: 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/20/2024 2:58 AM, Eli Zaretskii wrote:
> > Should be fixed now on the master branch.
> 
> Thanks, it all seems to work now.

Does this mean we can close this bug, or is there anything else to do
here?

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/20/2024 11:08 AM, Eli Zaretskii wrote:
> Does this mean we can close this bug, or is there anything else to do
> here?

I need to implement a new version of my patch that uses :align-to and 
such. So there are still things to do in this bug, but the :align-to 
issue that was blocking progress on this bug is now resolved.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter <jporterbugs@gmail.com> writes:

> (Note: I plan to merge this only after we cut the Emacs 30 release
> branch, since it seems a bit too substantial a change to sneak in
> right near the end. However, I think the patch is mostly done aside
> from one remaining issue, so any feedback is very welcome.)
>
> 'visual-wrap-prefix-mode' has one small issue: since the wrap prefix
> is just a string, the wrapped text may not line up for variable-width
> fonts. This is mainly in cases like so:
>
>   * here is some text that
>     got visually wrapped
>
> If the "* " is variable-width, the second line will probably be
> indented wrong by a few pixels.
>
> The attached patch adds a display spec in this case so that the text
> lines up perfectly. There's currently one problem though: I'm not sure
> how to regenerate the wrap prefix automatically if the face
> changes. It's not hard to handle for 'text-scale-adjust', but I don't

Actually, there's more than just this one problem.  To start with, the
existing format of the generated line prefix properties enables the
extracted fill prefix to be displayed consistently on any frame,
whatever the metrics of its default font/face.  This I consider a far
more critical capability than perfect alignment of wrapped text in the
presence of a variable-pitch default font, as the latter is virtually
unknown among programmers, and also unimplementable within reasonable
standards of performance with pixelwise spacers.  If this is to be
installed, please condition it behind a user option and restore the
existing logic that you have effaced as the default.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/17/2024 7:23 AM, Po Lu via Bug reports for GNU Emacs, the Swiss 
army knife of text editors wrote:
> Actually, there's more than just this one problem.  To start with, the
> existing format of the generated line prefix properties enables the
> extracted fill prefix to be displayed consistently on any frame,
> whatever the metrics of its default font/face.  This I consider a far
> more critical capability than perfect alignment of wrapped text in the
> presence of a variable-pitch default font, as the latter is virtually
> unknown among programmers, and also unimplementable within reasonable
> standards of performance with pixelwise spacers.

That's a good point. In practice, we should only do this if the 
first-line prefix uses a variable-pitch font, since otherwise we might 
as well just use the requisite number of space characters as before, 
sans display-spec.

> If this is to be
> installed, please condition it behind a user option and restore the
> existing logic that you have effaced as the default.

I'll add a check for variable-pitch fonts and a user option. With the 
variable-pitch check, I think it should be ok for the user option to 
default to enabling this new behavior? For fixed-pitch fonts, the new 
code would behave the same as before regardless of the option.

Note that I haven't removed any of the old logic though; it's all there 
as before, since the space characters are useful for terminals (where 
the pixelwise :width display-spec doesn't do anything for us, if I 
understand correctly). I did extract parts of 'fill-content-prefix' into 
'visual-wrap--content-prefix', but the only parts I removed were the 
ones for handling multiple lines; those don't apply here, since 
'visual-wrap-prefix-function' gets the prefixes one line at a time.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/17/2024 9:13 AM, Jim Porter wrote:
> Note that I haven't removed any of the old logic though; it's all there 
> as before, since the space characters are useful for terminals (where 
> the pixelwise :width display-spec doesn't do anything for us, if I 
> understand correctly).

Evidently I wasn't understanding this correctly, since further testing 
and consulting the code shows that the :width display-spec works just 
fine on terminals. It does make it harder to get this patch working well 
though...

Maybe :align-to could be made to work. It doesn't seem to work at all 
for me, even in a minimal test case like calling this on a long line of 
text:

   (put-text-property (point-min) (point-max) 'wrap-prefix '(space 
:align-to 4))

Replacing :align-to with :width works just fine though. It's entirely 
possible I'm just doing something wrong, but maybe this is a bug?

(Now what would be great is if :align-to supported a *string* value that 
meant "use the pixel-width of this string". Which actually gives me an 
idea... what if the wrap-prefix is just the original text like "* " and 
we fontify it to be invisible? It's not part of the buffer and you can't 
select it anyway, so it doesn't really matter what the text is so long 
as it's not visible.)

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Cc: 71605@debbugs.gnu.org
> Date: Mon, 17 Jun 2024 11:17:45 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Maybe :align-to could be made to work. It doesn't seem to work at all 
> for me, even in a minimal test case like calling this on a long line of 
> text:
> 
>    (put-text-property (point-min) (point-max) 'wrap-prefix '(space 
> :align-to 4))
> 
> Replacing :align-to with :width works just fine though. It's entirely 
> possible I'm just doing something wrong, but maybe this is a bug?

It's a bug in Emacs 29 and Emacs 30.  It works in Emacs 27.

> (Now what would be great is if :align-to supported a *string* value that 
> meant "use the pixel-width of this string".

It's already possible: just have a variable that holds the width, and
use it in the :align-to expression.

> Which actually gives me an 
> idea... what if the wrap-prefix is just the original text like "* " and 
> we fontify it to be invisible? It's not part of the buffer and you can't 
> select it anyway, so it doesn't really matter what the text is so long 
> as it's not visible.)

I don't think I follow: how is this relevant to the issue at hand?

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/17/2024 12:55 PM, Eli Zaretskii wrote:
>> Cc: 71605@debbugs.gnu.org
>> Date: Mon, 17 Jun 2024 11:17:45 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Replacing :align-to with :width works just fine though. It's entirely
>> possible I'm just doing something wrong, but maybe this is a bug?
> 
> It's a bug in Emacs 29 and Emacs 30.  It works in Emacs 27.

Ah, that explains it.

>> (Now what would be great is if :align-to supported a *string* value that
>> meant "use the pixel-width of this string".
> 
> It's already possible: just have a variable that holds the width, and
> use it in the :align-to expression.

That would make the code simpler, but I'd still need to figure out when 
to reevaluate the variable to update things.

>> Which actually gives me an
>> idea... what if the wrap-prefix is just the original text like "* " and
>> we fontify it to be invisible? It's not part of the buffer and you can't
>> select it anyway, so it doesn't really matter what the text is so long
>> as it's not visible.)
> 
> I don't think I follow: how is this relevant to the issue at hand?

I was thinking you could wrap "* this is some text" like so:

   * this is
   * some text

The second "* " would come from the wrap-prefix, but we'd make the 
foreground transparent (or the same color as the background) so it 
wouldn't be visible. Then it would always take up the same width as the 
first "* " because it's the same string with the same font and everything.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/17/2024 1:08 PM, Jim Porter wrote:
> On 6/17/2024 12:55 PM, Eli Zaretskii wrote:
>>> Cc: 71605@debbugs.gnu.org
>>> Date: Mon, 17 Jun 2024 11:17:45 -0700
>>> From: Jim Porter <jporterbugs@gmail.com>
>>>
>>> Replacing :align-to with :width works just fine though. It's entirely
>>> possible I'm just doing something wrong, but maybe this is a bug?
>>
>> It's a bug in Emacs 29 and Emacs 30.  It works in Emacs 27.
> 
> Ah, that explains it.

Just so others are aware I'm going to try and bisect this. I'll report 
back when it's finished.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Jim Porter]

On 6/17/2024 8:02 PM, Jim Porter wrote:
> Just so others are aware I'm going to try and bisect this. I'll report 
> back when it's finished.

Looks like this is the commit that regressed this:


commit fb3d582e7ba595b7680e2c2adf22c7ab699e5792
Author: Eli Zaretskii <eliz@gnu.org>
Date:   Wed Jun 29 20:15:36 2022 +0300

     Fix hscrolling of :align-to when display-line-numbers is in effect

     * src/dispextern.h (struct it): Rename 'tab_offset' member to
     'stretch_adjust'.
     * src/xdisp.c (gui_produce_glyphs, produce_stretch_glyph)
     (display_line): All users of 'tab_offset' changed.
     (produce_stretch_glyph): Fix calculation of ':align-to' when
     line numbers are displayed and the window is hscrolled.
     (calc_pixel_width_or_height): Fix calculation of width of 'space'
     display property when 'display-line-numbers' is turned on, but the
     line number was not yet produced for the current glyph row.
     (Bug#56176)


I'll take a look in more detail tomorrow, but if anyone else wants to 
fix this, go ahead. I also have a bit of Elisp that checks whether this 
feature works, and I should be able to make it into a proper regression 
test with a bit of work.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Mon, 17 Jun 2024 23:27:49 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: luangruo@yahoo.com, 71605@debbugs.gnu.org
> 
> On 6/17/2024 8:02 PM, Jim Porter wrote:
> > Just so others are aware I'm going to try and bisect this. I'll report 
> > back when it's finished.
> 
> Looks like this is the commit that regressed this:
> 
> 
> commit fb3d582e7ba595b7680e2c2adf22c7ab699e5792
> Author: Eli Zaretskii <eliz@gnu.org>
> Date:   Wed Jun 29 20:15:36 2022 +0300
> 
>      Fix hscrolling of :align-to when display-line-numbers is in effect
> 
>      * src/dispextern.h (struct it): Rename 'tab_offset' member to
>      'stretch_adjust'.
>      * src/xdisp.c (gui_produce_glyphs, produce_stretch_glyph)
>      (display_line): All users of 'tab_offset' changed.
>      (produce_stretch_glyph): Fix calculation of ':align-to' when
>      line numbers are displayed and the window is hscrolled.
>      (calc_pixel_width_or_height): Fix calculation of width of 'space'
>      display property when 'display-line-numbers' is turned on, but the
>      line number was not yet produced for the current glyph row.
>      (Bug#56176)

Thanks.

Unfortunately, it means the current behavior is not a bug, but rather
an unintended consequence of another bugfix.  The above changeset
changed :align-to to count from the beginning of the physical line,
not the screen line.  So, for example, if you have an 80-column line,
the continuation line begins from column 80, not 0.  Thus, something
like ':align-to 4' will never work on continuation lines, because we
got past column 4 long ago...

So I need to think whether and how to restore the capability of using
:align-to in wrap-prefix.

bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'

[Eli Zaretskii]

> Date: Mon, 17 Jun 2024 13:08:47 -0700
> Cc: luangruo@yahoo.com, 71605@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >> Which actually gives me an
> >> idea... what if the wrap-prefix is just the original text like "* " and
> >> we fontify it to be invisible? It's not part of the buffer and you can't
> >> select it anyway, so it doesn't really matter what the text is so long
> >> as it's not visible.)
> > 
> > I don't think I follow: how is this relevant to the issue at hand?
> 
> I was thinking you could wrap "* this is some text" like so:
> 
>    * this is
>    * some text
> 
> The second "* " would come from the wrap-prefix, but we'd make the 
> foreground transparent (or the same color as the background) so it 
> wouldn't be visible. Then it would always take up the same width as the 
> first "* " because it's the same string with the same font and everything.

If it's transparent, some change in the faces could reveal it, which I
think will be perceived as a bug.  And invisible text cannot take up
screen estate, so I'm not sure how would that help...