Subject: Re: bug#73600: 31.0.50; Visual wrap prefix mode and image display

From: Jim Porter <[email-masked]>
Date: Sat, 5 Oct 2024 11:07:39 -0700

On 10/4/2024 11:41 PM, Eli Zaretskii wrote:
>> Date: Fri, 4 Oct 2024 12:45:10 -0700
>> Cc: [email-masked], [email-masked]
>> From: Jim Porter <[email-masked]>
>>
>>     * 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.

From d4d97008452893c0043d585a2730300142f0b32d Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to multi-line display
 specs

This makes sure we don't interfere with other display specs, e.g. for
images displayed in 'image-mode' (bug#73600).

* lisp/visual-wrap.el (visual-wrap--safe-display-specs): New variable.
(visual-wrap--display-property-safe-p): New function.
(visual-wrap--apply-to-line): Bail out if we're in the middle of an
unsafe multi-line display spec.
---
 lisp/visual-wrap.el | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..63d6510f2e7 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,34 @@ visual-wrap--face-extend-p
         (face-extend-p face nil t)
       (face-background face nil t)))))
 
+(defvar visual-wrap--safe-display-specs
+  '(height raise)
+  "A list of display specs that don't interfere with wrap prefixes.")
+
+(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.
+
+OFFSET should be 1 or -1 and tells which direction to look to see if the
+display property has changed.  If it has changed, then the display
+property is always considered safe: since POSITION is at the
+beginning (or end) of that property, our additional text properties
+don't cause problems."
+  (let ((pos-limit (if (= offset 1) (point-max) (point-min)))
+        (display (get-char-property position 'display)))
+    (or (= position pos-limit)
+        (not display)
+        (not (eq (get-char-property (+ position offset) 'display) display))
+        (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)))))))
+
 (defun visual-wrap--prefix-face (fcp _beg end)
   ;; If the fill-context-prefix already specifies a face, just use that.
   (cond ((get-text-property 0 'face fcp))
@@ -117,9 +145,16 @@ visual-wrap--apply-to-line
   "Apply visual-wrapping properties to the logical line starting at POSITION."
   (save-excursion
     (goto-char position)
-    (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))
                (next-line-prefix (visual-wrap--content-prefix
-                                  first-line-prefix position)))
+                                  first-line-prefix (point))))
       (when (numberp next-line-prefix)
         ;; Set a minimum width for the prefix so it lines up correctly
         ;; with subsequent lines.  Make sure not to do this past the end
@@ -127,12 +162,12 @@ visual-wrap--apply-to-line
         ;; potentially return a prefix longer than the current line in
         ;; the buffer.)
         (add-display-text-property
-         position (min (+ position (length first-line-prefix))
-                       (line-end-position))
+         (point) (min (+ (point) (length first-line-prefix))
+                      eol)
          'min-width `((,next-line-prefix . width))))
       (setq next-line-prefix (visual-wrap--adjust-prefix next-line-prefix))
       (put-text-property
-       position (line-end-position) 'wrap-prefix
+       (point) eol 'wrap-prefix
        (if (numberp next-line-prefix)
            `(space :align-to (,next-line-prefix . width))
          next-line-prefix)))))
-- 
2.25.1