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

From: Eli Zaretskii <[email-masked]>
Date: Sat, 05 Oct 2024 09:41:05 +0300

> Date: Fri, 4 Oct 2024 12:45:10 -0700
> Cc: [email-masked], [email-masked]
> From: Jim Porter <[email-masked]>
> 
> > I think this is too radical.  There are 'display' properties that do
> > not replace the text: (space PROPS), (height HEIGHT), (raise FACT),
> > and a few others.  These should not prevent adding the wrapping
> > properties, I think.  The challenge is to detect the presence of
> > "allowed" properties and the absence of "forbidden" ones, since a
> > 'display' property's spec could be a list or a vector of several
> > separate values.  So the code will need to look inside the list/vector
> > and examine each element.
> 
> I think in addition to that, it's allowed if the display property has 
> changed at the beginning (or end) of the line. For example, if I were to 
> use the 'image' spec to render an image in-line with some other text, 
> then 'visual-wrap-prefix-mode' can set the wrap-prefix as usual with no 
> issues. The simpler case is when the image is at the end:
> 
>    * 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?

> 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?

> +(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.

> +        (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.

> -    (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.

Thanks.