[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-
From: |
Jim Porter |
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 |
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.
misaligned.png
Description: PNG image
0001-Add-support-for-variable-width-text-in-visual-wrap-p.patch
Description: Text document
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/16
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/17
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode',
Jim Porter <=
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/17
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/17
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/18
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/18
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/19
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/19
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/20
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/20
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Eli Zaretskii, 2024/06/20
- bug#71605: 30.0.50; [PATCH] Support variable-width text in 'visual-wrap-prefix-mode', Jim Porter, 2024/06/20