Package: emacs;
Reported by: Gautier Ponsinet <gautier <at> gautierponsinet.xyz>
Date: Wed, 2 Oct 2024 15:21:02 UTC
Severity: normal
Found in version 31.0.50
Done: Jim Porter <jporterbugs <at> gmail.com>
Message #32 received at 73600 <at> debbugs.gnu.org (full text, mbox):
From: Jim Porter <jporterbugs <at> gmail.com> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 73600 <at> debbugs.gnu.org, gautier <at> gautierponsinet.xyz Subject: Re: bug#73600: 31.0.50; Visual wrap prefix mode and image display Date: Sat, 5 Oct 2024 11:07:39 -0700
[Message part 1 (text/plain, inline)]
On 10/4/2024 11:41 PM, Eli Zaretskii wrote:
>> Date: Fri, 4 Oct 2024 12:45:10 -0700
>> Cc: 73600 <at> debbugs.gnu.org, gautier <at> gautierponsinet.xyz
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> * Here is some text. [IMG]
>>
>> That should get the wrap prefix.
>
> I think you will find that we always clip images at window edges, so
> the above might be trickier. Did you actually try that? How do you
> get at the width of the image actually displayed in that case?
Yes, see attached. "image.html" is a simple page testing both cases,
which you can try out with something like this:
(progn
(setq ;; Show our image inline.
shr-max-inline-image-size (cons 30 30)
;; Don't hard-wrap text; use 'visual-line-mode' and
;; 'visual-wrap-prefix-mode'.
shr-fill-text nil)
(eww "file:///path/to/image.html"))
I've also attached a screenshot showing this in action.
For this first case, I don't think the width of the image matters to
'visual-wrap-prefix-mode'; the width we want to get here is the width of
the prefix string "* ".
>> The reverse is also true, though it's more complex:
>>
>> [IMG] Here is some text.
>
> What do you expect to happen here, and how does the code handle this,
> especially the visual width of the image?
Assuming the mode sets 'adaptive-fill-function' correctly, I'd expect to
see something like the second block of the attached screenshot, or for a
textual approximation, something like this:
[IMG] Here is some text, and
here is some more.
In the attached HTML page, we get the above behavior automatically
because SHR inserts the string "*" into the buffer to add the image
display spec to. As a result, the fill prefix is "* " (with the "*"
having the display spec). Once 'visual-wrap-prefix-mode' passes that to
'string-pixel-width', we get the actual pixel width we expect.
As you note, this doesn't quite work if Emacs clips the image; in this
example, 'string-pixel-width' just returns the width of the space after
the "*" with the image spec. However, the only "problem" this causes is
that the wrap prefix is smaller than otherwise expected. In practice,
this might even be a *good* thing since it ensures that the wrap prefix
isn't wider than the window width.
However, note that this case only comes up if the image is *also* on the
same logical line as some other text without an image spec. For SHR/EWW,
this shouldn't happen unless a user sets 'shr-max-inline-image-size' to
some large value (it's nil by default) *and* 'shr-max-image-proportion'
to some value >1 (it's 0.9 by default).
For 'image-mode', this doesn't apply at all since the entire buffer text
has the image spec. Are there other test cases I should look at?
>> +(defun visual-wrap--display-property-safe-p (position offset)
>> + "Return non-nil if the display property at POSITION is \"safe\".
>> +A \"safe\" display property is one where all the display specs are
>> +members of `visual-wrap--safe-display-specs', and won't interfere with
>> +the additional text properties that `visual-wrap-prefix-mode' uses.
>
> This should mention the fact that some 'display' properties replace
> the text and some don't, otherwise the above doesn't really explain
> the underlying issue. The text reads as if it says '"Safe" display
> properties are those which are safe and don't interfere with adding
> more properties'. Without more details about the cause this is not
> helpful.
Ok, done. (I moved this explanation to the defvar since that's the place
where people might change the setting.)
>
>> + (when (or (vectorp display) (listp display))
>> + (unless (listp (car display)) (setq display (list display)))
>> + (not (catch 'unsafe
>> + (mapc (lambda (spec)
>> + (unless (memq spec visual-wrap--safe-display-specs)
>> + (throw 'unsafe t)))
>> + display)))))))
>
> I'm not sure I understand this. DISPLAY here could also be a single
> value, not a list or a vector, but in that case you don't test it
> against the "safe" specs? Also, (car display) will signal an error if
> DISPLAY is a vector, but in any case, what is the significance of the
> first element of DISPLAY being a list? I'm probably missing
> something.
In the case where DISPLAY is a single value (e.g. a string), we just
treat it as unsafe. This code is an attempt to adapt the logic from
find_display_property. I've added some comments explaining the logic and
fixed the issue you mentioned.
If this way looks reasonable, I could add some regression tests for it.
Alternately, maybe there should be some function that "normalizes" the
display property so that it's always a list of specs (or something like
that). Then it would be easier to write a function like I did here that
just wants to ask, "Are all the specs on my safe list?"
>> - (when-let ((first-line-prefix (fill-match-adaptive-prefix))
>> + (when-let ((eol (pos-eol))
>> + ;; Don't add wrapping properties if we're in the middle
>> + ;; of an unsafe multi-line display spec. (For example,
>> + ;; this could be an image containing a newline-byte being
>> + ;; displayed in `image-mode'; see bug#73600.)
>> + ((visual-wrap--display-property-safe-p (point) -1))
>> + ((visual-wrap--display-property-safe-p eol 1))
>> + (first-line-prefix (fill-match-adaptive-prefix))
>
> I'm not sure I can reason about this logic. I'd prefer the code to
> completely skip text covered by "unsafe" display properties, because
> reasoning about that text makes no sense: it will not show on display.
> Specifically, for the "text" that is the bytes of the image file, I
> don't know how to begin reasoning about newlines embedded in those
> bytes. Thus, it is hard for me to tell if I agree with the above
> logic or not.
The idea here is this:
1. If we have some display property that spans multiple lines (either
from the previous line onto our current line, or from the current
line onto the next), then we need to be careful and check the specs
of this multi-line display property.
a. If the multi-line display prop is something safe (e.g. 'raise'), we
can proceed with adding our visual-wrap props.
b. Otherwise, we can't safely add visual-wrap props, so we bail out.
2. If no display property spans multiple lines, then any display props
on our line are *only* on our line, so we can add the visual-wrap
props without breaking the 'eq'-ness of any existing display props.
That's there to allow the behavior as shown in "image.html".
For the image-mode case, if we have one newline-byte, then we hit
condition (1): we have the same display property on both the first and
second lines. Then we examine each display spec, and find that there's
one that's not on the safe list (b), so we bail out.
I agree that this logic is fairly complex, and I'm happy to rearrange it
or add comments as needed once we're on the same page. I'm also ok with
just saying "any multi-line display properties are assumed to be unsafe"
for now. That's simpler, and we could add this extra logic later if
someone notices a specific issue.
[0001-Don-t-add-visual-wrap-prefix-properties-to-multi-lin.patch (text/plain, attachment)]
[image.html (text/html, attachment)]
[rendered.png (image/png, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.