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