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

Previous Next

Full log

View this message in rfc822 format

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

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

On 6/17/2024 4:37 AM, Eli Zaretskii wrote:
>> Date: Sun, 16 Jun 2024 19:56:44 -0700
>> From: Jim Porter <jporterbugs <at> 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.

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

[0001-Add-support-for-variable-width-text-in-visual-wrap-p.patch (text/plain, attachment)]

Previous Next

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