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

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71605 <at> debbugs.gnu.org
Subject: Re: bug#71605: 30.0.50;
 [PATCH] Support variable-width text in 'visual-wrap-prefix-mode'
Date: Mon, 17 Jun 2024 14:37:43 +0300

> Date: Sun, 16 Jun 2024 19:56:44 -0700
> From: Jim Porter <jporterbugs <at> 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.

Previous Next

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