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.