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

all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
@ 2024-10-02 13:04 Gautier Ponsinet
  2024-10-02 15:42 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Gautier Ponsinet @ 2024-10-02 13:04 UTC (permalink / raw)
  To: 73600

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hello everyone,

The mode visual-wrap-prefix-mode sometimes causes images to be displayed
incorrectly in emacs (in a mosaic-like way).

To this mail are attached screenshots of the problem and the image file
used in the screenshots which is from wikipedia:
- https://fr.wikipedia.org/wiki/Olcades
- https://upload.wikimedia.org/wikipedia/commons/thumb/3/3a/Iberia_300BC-fr.svg/langfr-873px-Iberia_300BC-fr.svg.png

Note that the error does not occur with all images, but I am not able to
identify which characteristic of an image makes the problem appear.
Still, I have encountered the problem with multiple image files.

In emacs -Q:
- enable global-visual-wrap-prefix-mode,
- open the attached image.

All the best,
Gautier.


[-- Attachment #2: 873px-Iberia_300BC-fr.svg.png --]
[-- Type: image/png, Size: 340812 bytes --]

[-- Attachment #3: visual-wrap-prefix-mode-disabled.jpg --]
[-- Type: image/jpeg, Size: 162405 bytes --]

[-- Attachment #4: visual-wrap-prefix-mode-enabled.jpg --]
[-- Type: image/jpeg, Size: 276199 bytes --]

[-- Attachment #5: Type: text/plain, Size: 12204 bytes --]


========================================================================
In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.18.2, Xaw3d scroll bars) of 2024-10-02 built on
 gautier-laptop
Repository revision: 4bb62af3263057312021e076dc7e0c8ff195e38f
Repository branch: makepkg
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
 --with-modules --without-m17n-flt --without-gconf
 --with-native-compilation=yes --with-xinput2 --with-x-toolkit=lucid
 --without-xft --with-xaw3d --with-sound=no --with-tree-sitter
 --without-gpm --without-compress-install
 '--program-transform-name=s/\([ec]tags\)/\1.emacs/'
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions
 -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security
 -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer
 -mno-omit-leaf-frame-pointer -g
 -ffile-prefix-map=/home/gautier/Documents/Sources/AUR/emacs-git/src=/usr/src/debug/emacs-git
 -flto=auto' 'LDFLAGS=-Wl,-O1 -Wl,--sort-common -Wl,--as-needed
 -Wl,-z,relro -Wl,-z,now -Wl,-z,pack-relative-relocs -flto=auto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LCMS2 LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP X11 XAW3D XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: fr_FR.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Splash Screen

Minor modes in effect:
  visual-fill-column-mode: t
  pdf-occur-global-minor-mode: t
  TeX-PDF-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  server-mode: t
  recentf-mode: t
  marginalia-mode: t
  global-corfu-mode: t
  corfu-mode: t
  vertico-mouse-mode: t
  vertico-mode: t
  minibuffer-depth-indicate-mode: t
  delete-selection-mode: t
  global-goto-address-mode: t
  goto-address-mode: t
  pulsar-global-mode: t
  pulsar-mode: t
  lin-global-mode: t
  override-global-mode: t
  winner-mode: t
  repeat-mode: t
  global-visual-wrap-prefix-mode: t
  visual-wrap-prefix-mode: t
  global-word-wrap-whitespace-mode: t
  word-wrap-whitespace-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  window-divider-mode: t
  minibuffer-regexp-mode: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/gautier/.local/share/emacs/packages/ef-themes-1.8.0/theme-loaddefs hides /home/gautier/.local/share/emacs/packages/modus-themes-20240921.616/theme-loaddefs
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-tag hides /usr/share/emacs/site-lisp/notmuch-tag
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch hides /usr/share/emacs/site-lisp/notmuch
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-wash hides /usr/share/emacs/site-lisp/notmuch-wash
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-maildir-fcc hides /usr/share/emacs/site-lisp/notmuch-maildir-fcc
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-hello hides /usr/share/emacs/site-lisp/notmuch-hello
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-jump hides /usr/share/emacs/site-lisp/notmuch-jump
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-mua hides /usr/share/emacs/site-lisp/notmuch-mua
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-query hides /usr/share/emacs/site-lisp/notmuch-query
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-print hides /usr/share/emacs/site-lisp/notmuch-print
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-compat hides /usr/share/emacs/site-lisp/notmuch-compat
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-company hides /usr/share/emacs/site-lisp/notmuch-company
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-message hides /usr/share/emacs/site-lisp/notmuch-message
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-parser hides /usr/share/emacs/site-lisp/notmuch-parser
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-crypto hides /usr/share/emacs/site-lisp/notmuch-crypto
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-address hides /usr/share/emacs/site-lisp/notmuch-address
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-tree hides /usr/share/emacs/site-lisp/notmuch-tree
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-show hides /usr/share/emacs/site-lisp/notmuch-show
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/coolj hides /usr/share/emacs/site-lisp/coolj
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-draft hides /usr/share/emacs/site-lisp/notmuch-draft
/home/gautier/.local/share/emacs/packages/notmuch-20240816.2039/notmuch-lib hides /usr/share/emacs/site-lisp/notmuch-lib
/home/gautier/.local/share/emacs/packages/transient-20241001.1031/transient hides /usr/share/emacs/31.0.50/lisp/transient
/home/gautier/.local/share/emacs/packages/ef-themes-1.8.0/theme-loaddefs hides /usr/share/emacs/31.0.50/lisp/theme-loaddefs

Features:
(shadow sort mail-extr emacsbug char-fold mule-util visual-fill-column
nov esxml-query pdf-sync pdf-occur ibuf-ext ibuffer ibuffer-loaddefs
tablist advice tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet pdf-isearch
let-alist pdf-misc pdf-tools pdf-view pdf-cache pdf-info tq pdf-util
pdf-macs tex texmathp elfeed-show elfeed-search elfeed-csv elfeed
elfeed-curl elfeed-log elfeed-db elfeed-lib xml-query ol-notmuch notmuch
notmuch-tree notmuch-jump notmuch-hello notmuch-show notmuch-print
notmuch-crypto notmuch-mua notmuch-message notmuch-draft
notmuch-maildir-fcc notmuch-address notmuch-company notmuch-parser
notmuch-wash coolj icalendar notmuch-tag notmuch-lib notmuch-version
notmuch-compat org-duration vc-git vc-dispatcher org-contacts
org-capture quail oc-basic disp-table org-habit ol-eww eww url-queue
mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu
mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill
kinsoku url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus
gnus-cloud nnimap nnmail mail-source utf7 nnoo parse-time gnus-spec
gnus-int gnus-range gnus-win gnus nnheader range ol-docview doc-view
jka-compr image-mode exif ol-bibtex bibtex iso8601 ol-bbdb ol-w3m ol-doi
org-link-doi org-agenda embark-org org-element org-persist org-id
org-refile org-element-ast inline avl-tree generator org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-src sh-script smie treesit
executable ob-comint org-pcomplete org-list org-footnote org-faces
org-entities noutline outline org-version ob-emacs-lisp ob-core ob-eval
org-cycle org-table ol org-fold org-fold-core org-keys oc org-loaddefs
find-func org-compat org-macs notifications dbus compile xml appt
diary-lib diary-loaddefs cal-menu calendar cal-loaddefs project
magit-extras magit-bookmark magit-submodule magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
which-func imenu magit-diff smerge-mode diff diff-mode track-changes
git-commit log-edit message sendmail yank-media puny rfc822 mml mml-sec
epa derived epg rfc6068 epg-config gnus-util time-date mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor comp comp-cstr
warnings comp-run comp-common rx shell pcomplete comint ansi-osc server
ansi-color magit-mode transient benchmark magit-git magit-base
magit-section format-spec cursor-sensor crm dash recentf tree-widget
gnus-dired dired dired-loaddefs embark-consult embark ffap consult
orderless marginalia cape corfu vertico-mouse vertico-directory vertico
mb-depth jinx tempel compat delsel fontaine goto-addr
modus-operandi-theme pulsar pulse color cus-edit cus-load wid-edit lin
hl-line face-remap modus-themes ef-themes edmacro kmacro
use-package-bind-key bind-key easy-mmode use-package-ensure
use-package-core burly frameset thingatpt bookmark text-property-search
pp winner ace-window avy ring repeat visual-wrap word-wrap-mode
ace-window-autoloads auctex-autoloads tex-site avy-autoloads
bbdb-autoloads beframe-autoloads bufferlo-autoloads burly-autoloads
cape-autoloads citar-embark-autoloads citar-autoloads citeproc-autoloads
corfu-autoloads cursory-autoloads debbugs-autoloads
dired-preview-autoloads eat-autoloads ebdb-autoloads cl-extra help-mode
ef-themes-autoloads elfeed-autoloads embark-consult-autoloads
consult-autoloads embark-autoloads expand-region-autoloads f-autoloads
fontaine-autoloads forge-autoloads closql-autoloads emacsql-autoloads
fountain-mode-autoloads ghub-autoloads gnuplot-autoloads
htmlize-autoloads hyperbole-autoloads kotl-autoloads hact set hhist
indent-bars-autoloads jinx-autoloads ledger-mode-autoloads lin-autoloads
magit-autoloads pcase magit-section-autoloads dash-autoloads
marginalia-autoloads markdown-mode-autoloads modus-themes-autoloads
nov-autoloads esxml-autoloads kv-autoloads ol-notmuch-autoloads
notmuch-autoloads orderless-autoloads org-contacts-autoloads
org-modern-autoloads osm-autoloads parsebib-autoloads
pdf-tools-autoloads popper-autoloads pulsar-autoloads queue-autoloads
rainbow-mode-autoloads s-autoloads spacious-padding-autoloads
string-inflection-autoloads tablist-autoloads tempel-autoloads
tmr-autoloads transient-autoloads trashed-autoloads treepy-autoloads
vertico-autoloads visual-fill-column-autoloads wgrep-autoloads info
with-editor-autoloads yaml-autoloads xdg package browse-url url
url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs icons password-cache json
subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib
rmc iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd touch-screen tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty move-toolbar
make-network-process native-compile emacs)

Memory information:
((conses 16 641073 63157) (symbols 48 42867 0) (strings 32 167248 6724)
 (string-bytes 1 5185552) (vectors 16 237023)
 (vector-slots 8 3126477 384380) (floats 8 770 1)
 (intervals 56 1489 224) (buffers 992 15))

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-02 13:04 bug#73600: 31.0.50; Visual wrap prefix mode and image display Gautier Ponsinet
@ 2024-10-02 15:42 ` Eli Zaretskii
  2024-10-02 15:51   ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-02 15:42 UTC (permalink / raw)
  To: Gautier Ponsinet; +Cc: 73600

> From: Gautier Ponsinet <gautier@gautierponsinet.xyz>
> Date: Wed, 02 Oct 2024 16:04:02 +0300
> 
> The mode visual-wrap-prefix-mode sometimes causes images to be displayed
> incorrectly in emacs (in a mosaic-like way).
> 
> To this mail are attached screenshots of the problem and the image file
> used in the screenshots which is from wikipedia:
> - https://fr.wikipedia.org/wiki/Olcades
> - https://upload.wikimedia.org/wikipedia/commons/thumb/3/3a/Iberia_300BC-fr.svg/langfr-873px-Iberia_300BC-fr.svg.png
> 
> Note that the error does not occur with all images, but I am not able to
> identify which characteristic of an image makes the problem appear.
> Still, I have encountered the problem with multiple image files.
> 
> In emacs -Q:
> - enable global-visual-wrap-prefix-mode,
> - open the attached image.

In a build with --enable-checking, this recipe causes an assertion
violation:

  dispnew.c:4495: Emacs fatal error: assertion failed: entry || verify_row_hash (row)

  Thread 1 hit Breakpoint 1, terminate_due_to_signal (sig=22,
      backtrace_limit=2147483647) at emacs.c:432
  432       signal (sig, SIG_DFL);
  (gdb) bt
  #0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at emacs.c:432
  #1  0x0053d2d9 in die (msg=0xc11c78 "entry || verify_row_hash (row)",
      file=0xc112a7 "dispnew.c", line=4495) at alloc.c:8057
  #2  0x0032d713 in add_row_entry (row=0xb9ed5b0) at dispnew.c:4495
  #3  0x0032df1d in scrolling_window (w=0xb941fb8, tab_line_p=0)
      at dispnew.c:4735
  #4  0x0032b8eb in update_window (w=0xb941fb8, force_p=true) at dispnew.c:3735
  #5  0x0032ae0d in update_window_tree (w=0xb941fb8, force_p=true)
      at dispnew.c:3491
  #6  0x0032a6b4 in update_frame (f=0xb941d98, force_p=true,
      inhibit_hairy_id_p=false) at dispnew.c:3326
  #7  0x0038ac25 in redisplay_internal () at xdisp.c:17561
  #8  0x003883e7 in redisplay () at xdisp.c:16656
  #9  0x0048a5f0 in read_char (commandflag=1, map=XIL(0xc000000005788d00),
      prev_event=XIL(0), used_mouse_menu=0x61ff31f, end_time=0x0)
      at keyboard.c:2673
  #10 0x004a2c09 in read_key_sequence (keybuf=0x61ff5f8, prompt=XIL(0),
      dont_downcase_last=false, can_return_switch_frame=true,
      fix_current_buffer=true, prevent_redisplay=false,
      disable_text_conversion_p=false) at keyboard.c:10747
  #11 0x004861cb in command_loop_1 () at keyboard.c:1424
  #12 0x0057b9ad in internal_condition_case (bfun=0x485b69 <command_loop_1>,
      handlers=XIL(0x90), hfun=0x484bc2 <cmd_error>) at eval.c:1598
  #13 0x004855ce in command_loop_2 (handlers=XIL(0x90)) at keyboard.c:1163
  #14 0x0057ab2a in internal_catch (tag=XIL(0x12750),
      func=0x485597 <command_loop_2>, arg=XIL(0x90)) at eval.c:1277
  #15 0x00485539 in command_loop () at keyboard.c:1141
  #16 0x00484622 in recursive_edit_1 () at keyboard.c:749
  #17 0x004848c0 in Frecursive_edit () at keyboard.c:832
  #18 0x0047f9e1 in main (argc=2, argv=0x6322570) at emacs.c:2624





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-02 15:42 ` Eli Zaretskii
@ 2024-10-02 15:51   ` Eli Zaretskii
  2024-10-02 16:37     ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-02 15:51 UTC (permalink / raw)
  To: gautier, Jim Porter; +Cc: 73600

> Cc: 73600@debbugs.gnu.org
> Date: Wed, 02 Oct 2024 18:42:09 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > In emacs -Q:
> > - enable global-visual-wrap-prefix-mode,
> > - open the attached image.
> 
> In a build with --enable-checking, this recipe causes an assertion
> violation:

And in Emacs 30 the problem doesn't happen, so this is some regression
in Emacs 31.  Probably due to my changes in commit 71505b723c9f.

Jim, any ideas or suggestions?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-02 15:51   ` Eli Zaretskii
@ 2024-10-02 16:37     ` Jim Porter
  2024-10-03 10:59       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-02 16:37 UTC (permalink / raw)
  To: Eli Zaretskii, gautier; +Cc: 73600

On 10/2/2024 8:51 AM, Eli Zaretskii wrote:
>> Cc: 73600@debbugs.gnu.org
>> Date: Wed, 02 Oct 2024 18:42:09 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>>
>>> In emacs -Q:
>>> - enable global-visual-wrap-prefix-mode,
>>> - open the attached image.
>>
>> In a build with --enable-checking, this recipe causes an assertion
>> violation:
> 
> And in Emacs 30 the problem doesn't happen, so this is some regression
> in Emacs 31.  Probably due to my changes in commit 71505b723c9f.
> 
> Jim, any ideas or suggestions?

Strangely, this doesn't seem to happen with every image. "splash.png" in 
the Emacs repository seems fine, for example. I also can't reproduce 
this in EWW, even by opening the bad image reported in this bug.

I haven't looked into the internals of 'image-mode' very much, but I 
seem to recall that it has some special code for scrolling large images. 
Maybe that's interacting in some bad way with 'visual-wrap-prefix-mode'?

One option might simply be to disable 'visual-wrap-prefix-mode' when in 
'image-mode'. I don't think wrapping is useful in that context.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-02 16:37     ` Jim Porter
@ 2024-10-03 10:59       ` Eli Zaretskii
  2024-10-04  0:28         ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-03 10:59 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Wed, 2 Oct 2024 09:37:48 -0700
> Cc: 73600@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 10/2/2024 8:51 AM, Eli Zaretskii wrote:
> >> Cc: 73600@debbugs.gnu.org
> >> Date: Wed, 02 Oct 2024 18:42:09 +0300
> >> From: Eli Zaretskii <eliz@gnu.org>
> >>
> >>> In emacs -Q:
> >>> - enable global-visual-wrap-prefix-mode,
> >>> - open the attached image.
> >>
> >> In a build with --enable-checking, this recipe causes an assertion
> >> violation:
> > 
> > And in Emacs 30 the problem doesn't happen, so this is some regression
> > in Emacs 31.  Probably due to my changes in commit 71505b723c9f.
> > 
> > Jim, any ideas or suggestions?
> 
> Strangely, this doesn't seem to happen with every image. "splash.png" in 
> the Emacs repository seems fine, for example. I also can't reproduce 
> this in EWW, even by opening the bad image reported in this bug.

It depends on the width of the image and the contents of the image
file.  As soon as you show an image whose width is as wide or wider
than the window, and/or the image file has newline bytes, the problem
happens.

Note that displaying splash.svg, the problem happens even through the
image is not wider than window.  See below why this happens.

I've installed a fix which avoids the assertion violations in a build
with --enable-checking.  The fix does not prevent displaying multiple
images where only one should be shown -- because that is AFAICT a bug
in visual-wrap-prefix-mode, and should be fixed there.

What happens is that displaying an image in a buffer places a single
'display' property on the entire buffer portion which should be
displayed as an image, and the value of the property is the image
spec.  When visiting an image file, that property spans the entire
contents of the image file.  In this situation, the analysis of the
buffer text performed by visual-wrap-prefix-mode, which looks at
newlines and tries to calculate the suitable prefix width, is
completely wrong, because the buffer text (which are just the raw
bytes of the image file) is replaced on display by the image, and the
calculations based on character width are all wrong.  But
visual-wrap-prefix-mode does still make these calculations, and as
result could decide that a min-width or align-to display property
should be placed on some of these raw bytes.  Since
visual-wrap-prefix-mode uses add-display-text-property, which attempts
to keep existing display properties when it adds additional ones, the
result is that the single display property that specified the image is
broken into several identical display properties (some with min-width
added, some without).  And since these properties are not 'eq' to one
another, Emacs shows N identical images instead of just one.

The solution, I think, is for visual-wrap-prefix-mode to ignore text
that is covered by replacing display properties, at least those which
specify images, xwidgets, fringes, etc.  It's possible that we want to
pay attention to display properties whose value is a string, but not
to any others.

> One option might simply be to disable 'visual-wrap-prefix-mode' when in 
> 'image-mode'. I don't think wrapping is useful in that context.

I think image-mode is just the tip of a very large iceberg, see above:
visual-wrap-prefix-mode currently doesn't correctly handle text
covered by display properties.  I think this bug was uncovered when we
removed on master a few artificial restrictions on min-width, such as
non-support for display and overlay strings.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-03 10:59       ` Eli Zaretskii
@ 2024-10-04  0:28         ` Jim Porter
  2024-10-04  6:28           ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-04  0:28 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On 10/3/2024 3:59 AM, Eli Zaretskii wrote:
> I think image-mode is just the tip of a very large iceberg, see above:
> visual-wrap-prefix-mode currently doesn't correctly handle text
> covered by display properties.  I think this bug was uncovered when we
> removed on master a few artificial restrictions on min-width, such as
> non-support for display and overlay strings.

How about something like this? With this patch, if the start of a line 
is in the middle of a multi-line display spec, 'visual-wrap-prefix-mode' 
won't add any new properties. That should prevent any unwanted interference.

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-multiline.patch --]
[-- Type: text/plain, Size: 1756 bytes --]

From 432ac5b4fd4a5f15b0ae59d739ed2dd2b2e38a05 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to multiline 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--apply-to-line): Bail out if we're in
the middle of a multi-line display spec
---
 lisp/visual-wrap.el | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..af0b4fc8b0f 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -117,7 +117,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 (;; Don't add wrapping properties if we're in the middle
+               ;; of a multi-line display spec; it could be doing
+               ;; absolutely anything, and we don't want to interfere.
+               ;; (For example, this could be an image containing a
+               ;; newline-byte being displayed in `image-mode'; see
+               ;; bug#73600.)
+               ((or (= position (point-min))
+                    (not (eq (get-char-property position 'display)
+                             (get-char-property (1- position) 'display)))))
+               (first-line-prefix (fill-match-adaptive-prefix))
                (next-line-prefix (visual-wrap--content-prefix
                                   first-line-prefix position)))
       (when (numberp next-line-prefix)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-04  0:28         ` Jim Porter
@ 2024-10-04  6:28           ` Eli Zaretskii
  2024-10-04 19:45             ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-04  6:28 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Thu, 3 Oct 2024 17:28:14 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 10/3/2024 3:59 AM, Eli Zaretskii wrote:
> > I think image-mode is just the tip of a very large iceberg, see above:
> > visual-wrap-prefix-mode currently doesn't correctly handle text
> > covered by display properties.  I think this bug was uncovered when we
> > removed on master a few artificial restrictions on min-width, such as
> > non-support for display and overlay strings.
> 
> How about something like this? With this patch, if the start of a line 
> is in the middle of a multi-line display spec, 'visual-wrap-prefix-mode' 
> won't add any new properties. That should prevent any unwanted interference.

Something like that, yes.  But unfortunately, it's more complicated,
see below.

> --- a/lisp/visual-wrap.el
> +++ b/lisp/visual-wrap.el
> @@ -117,7 +117,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 (;; Don't add wrapping properties if we're in the middle
> +               ;; of a multi-line display spec; it could be doing
> +               ;; absolutely anything, and we don't want to interfere.
> +               ;; (For example, this could be an image containing a
> +               ;; newline-byte being displayed in `image-mode'; see
> +               ;; bug#73600.)
> +               ((or (= position (point-min))
                       ^^^^^^^^^^^^^^^^^^^^^^^^
A.k.a. (bobp), right?  Since point is already at position.

> +                    (not (eq (get-char-property position 'display)
> +                             (get-char-property (1- position) 'display)))))

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.

We could also start with the above and document that lines that begin
with any 'display' property will not be visually aligned/wrapped --
maybe this is not a severe restriction in practice.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-04  6:28           ` Eli Zaretskii
@ 2024-10-04 19:45             ` Jim Porter
  2024-10-05  6:41               ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-04 19:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On 10/3/2024 11:28 PM, Eli Zaretskii wrote:
> A.k.a. (bobp), right?  Since point is already at position.

Fixed. Since point is at position, I've modified everything in the 
function to use (point). That way, there's no risk of confusion if 
someone looks at this years later and wonders why some parts use "point" 
while others use "position".

> 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. The reverse is also true, though it's 
more complex:

   [IMG] Here is some text.

A mode that does something like that should set 'adaptive-fill-function' 
to handle this case and return an appropriate fill-prefix. Then 
'visual-wrap-prefix-mode' would do the right thing. For example, maybe 
EWW could do this to support images for bullet points (I'm not sure this 
is something we *actually* want to do in EWW, but hopefully it serves as 
an example.)

To be on the safe side, I'm only marking 'height' and 'raise' as safe 
for now, but adding new safe specs should just be a matter of putting 
them in the list.

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-multi-lin.patch --]
[-- Type: text/plain, Size: 4414 bytes --]

From d4d97008452893c0043d585a2730300142f0b32d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-04 19:45             ` Jim Porter
@ 2024-10-05  6:41               ` Eli Zaretskii
  2024-10-05 18:07                 ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-05  6:41 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Fri, 4 Oct 2024 12:45:10 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > 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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-05  6:41               ` Eli Zaretskii
@ 2024-10-05 18:07                 ` Jim Porter
  2024-10-06  0:28                   ` Jim Porter
  2024-10-12 12:00                   ` Eli Zaretskii
  0 siblings, 2 replies; 18+ messages in thread
From: Jim Porter @ 2024-10-05 18:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 7335 bytes --]

On 10/4/2024 11:41 PM, Eli Zaretskii wrote:
>> Date: Fri, 4 Oct 2024 12:45:10 -0700
>> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
>> From: Jim Porter <jporterbugs@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.

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-multi-lin.patch --]
[-- Type: text/plain, Size: 4414 bytes --]

From d4d97008452893c0043d585a2730300142f0b32d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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


[-- Attachment #3: image.html --]
[-- Type: text/html, Size: 3166 bytes --]

[-- Attachment #4: rendered.png --]
[-- Type: image/png, Size: 26584 bytes --]

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-05 18:07                 ` Jim Porter
@ 2024-10-06  0:28                   ` Jim Porter
  2024-10-12 12:00                   ` Eli Zaretskii
  1 sibling, 0 replies; 18+ messages in thread
From: Jim Porter @ 2024-10-06  0:28 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 135 bytes --]

On 10/5/2024 11:07 AM, Jim Porter wrote:
> Yes, see attached.

It would probably help if I attached the current version of my patch...

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-multi-lin.patch --]
[-- Type: text/plain, Size: 5056 bytes --]

From aa679b0945ffb298a8556a60e163727acb7fe68f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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 | 57 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..d798302d091 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,46 @@ 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.
+A \"safe\" display spec is one that won't interfere with the additional
+text properties that `visual-wrap-prefix-mode' uses.
+
+Specs that replace the text are unsafe, since they generally determine
+the range of text to replace via `eq'.  If `visual-wrap-prefix-mode'
+were to add text properties to some subset of this range, it would
+violate this assumption.")
+
+(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' (which see).
+
+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))
+        ;; Iterate over all the display specs to check if they're safe.
+        ;; Assume any display property other than a vector or list
+        ;; (e.g. a string) is unsafe.
+        (when (or (vectorp display) (listp display))
+          ;; The display property could be a single display spec; if so,
+          ;; wrap it in a list to iterate over.
+          (unless (consp (car-safe display)) (setq display (list display)))
+          (not (catch 'unsafe
+                 (mapc (lambda (spec)
+                         (unless (memq (car-safe 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 +157,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 +174,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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-05 18:07                 ` Jim Porter
  2024-10-06  0:28                   ` Jim Porter
@ 2024-10-12 12:00                   ` Eli Zaretskii
  2024-10-13  1:36                     ` Jim Porter
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-12 12:00 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Sat, 5 Oct 2024 11:07:39 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >> -    (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.

I'd prefer if we had code that, when it detects an unsafe display
property, would completely skip all of it.  With the above code, I
fond it difficult to reason whether it sometimes considers newlines
inside unsafe properties.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-12 12:00                   ` Eli Zaretskii
@ 2024-10-13  1:36                     ` Jim Porter
  2024-10-13  5:36                       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-13  1:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On 10/12/2024 5:00 AM, Eli Zaretskii wrote:
> I'd prefer if we had code that, when it detects an unsafe display
> property, would completely skip all of it.  With the above code, I
> fond it difficult to reason whether it sometimes considers newlines
> inside unsafe properties.

How about this implementation instead? I think it should do the same 
thing as my previous implementation, at least in all the cases we care 
about. I've also added some regression tests to verify that the code 
works the way we expect.

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-unsafe-mu.patch --]
[-- Type: text/plain, Size: 8220 bytes --]

From 48d57c0f8e70e31bb9ed2c6b485d8d5cf2950d99 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to unsafe 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): Use 'pos-eol'; we don't want to respect
field boundaries here.
(visual-wrap-prefix-function): Check for unsafe display properties at
the end of the line and skip past them if present.

* test/lisp/visual-wrap-tests.el: New test file.
---
 lisp/visual-wrap.el            | 48 +++++++++++++++++--
 test/lisp/visual-wrap-tests.el | 87 ++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 4 deletions(-)
 create mode 100644 test/lisp/visual-wrap-tests.el

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..26333089189 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,36 @@ 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.
+A \"safe\" display spec is one that won't interfere with the additional
+text properties that `visual-wrap-prefix-mode' uses.
+
+Specs that replace the text are unsafe, since they generally determine
+the range of text to replace via `eq'.  If `visual-wrap-prefix-mode'
+were to add text properties to some subset of this range, it would
+violate this assumption.")
+
+(defun visual-wrap--display-property-safe-p (display)
+  "Return non-nil if the display property DISPLAY is \"safe\".
+A \"safe\" display property is one where all the display specs are
+members of `visual-wrap--safe-display-specs' (which see)."
+  ;; The display property could be a single display spec; if so, wrap it
+  ;; in a list so we can iterate over it in our loop below.
+  (when (and (consp display) (not (consp (car display))))
+    (setq display (list display)))
+  ;; Loop over all the display specs to check if they're safe.  Assume
+  ;; any display property other than a vector or list (e.g. a string) is
+  ;; unsafe.
+  (when (or (vectorp display) (listp display))
+    (not (catch 'unsafe
+           (mapc (lambda (spec)
+                   (unless (memq (car-safe 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))
@@ -128,11 +158,11 @@ visual-wrap--apply-to-line
         ;; the buffer.)
         (add-display-text-property
          position (min (+ position (length first-line-prefix))
-                       (line-end-position))
+                       (pos-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
+       position (pos-eol) 'wrap-prefix
        (if (numberp next-line-prefix)
            `(space :align-to (,next-line-prefix . width))
          next-line-prefix)))))
@@ -206,8 +236,18 @@ visual-wrap-prefix-function
   (forward-line 0)
   (setq beg (point))
   (while (< (point) end)
-    (visual-wrap--apply-to-line (point))
-    (forward-line))
+    ;; Check if the display property at the end of this line is "safe".
+    (if (visual-wrap--display-property-safe-p
+         (get-char-property (pos-eol) 'display))
+        ;; If so, we can apply our visual wrapping properties to this
+        ;; line and continue to the next line.
+        (progn
+          (visual-wrap--apply-to-line (point))
+          (forward-line))
+      ;; Otherwise, skip ahead to the beginning of the first line after
+      ;; the unsafe display property changes.
+      (goto-char (next-single-char-property-change (pos-eol) 'display))
+      (unless (bolp) (forward-line 1))))
   `(jit-lock-bounds ,beg . ,end))
 
 ;;;###autoload
diff --git a/test/lisp/visual-wrap-tests.el b/test/lisp/visual-wrap-tests.el
new file mode 100644
index 00000000000..5cc4a8ac8a7
--- /dev/null
+++ b/test/lisp/visual-wrap-tests.el
@@ -0,0 +1,87 @@
+;;; visual-wrap-tests.el --- Tests for `visual-wrap-prefix-mode'  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tets for `visual-wrap-prefix-mode'.
+
+;;; Code:
+
+(require 'visual-wrap)
+(require 'ert)
+
+;;; Tests:
+
+(ert-deftest visual-wrap-tests/simple ()
+  "Test adding wrapping properties to text without display properties."
+  (with-temp-buffer
+    (insert "greetings\n* hello\n* hi")
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("greetings\n* hello\n* hi"
+               10 12 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               12 17 ( wrap-prefix (space :align-to (2 . width)))
+               18 20 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               20 22 ( wrap-prefix (space :align-to (2 . width))))))))
+
+(ert-deftest visual-wrap-tests/safe-display ()
+  "Test adding wrapping properties to text with safe display properties."
+  (with-temp-buffer
+    (insert #("* hello" 2 7 (display (raise 1))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (message "%S" (buffer-string))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* hello"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (raise 1)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/within-line ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties don't extend across multiple lines,
+`visual-wrap-prefix-mode' can still add wrapping properties."
+  (with-temp-buffer
+    (insert #("* [img]" 2 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (message "%S" (buffer-string))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* [img]"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/spanning-lines ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties do extend across multiple lines,
+`visual-wrap-prefix-mode' must avoid adding wrapping properties."
+  (with-temp-buffer
+    (insert #("* a\n* b" 0 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b" 0 7 (display (image :type bmp)))))))
+
+;; visual-wrap-tests.el ends here
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-13  1:36                     ` Jim Porter
@ 2024-10-13  5:36                       ` Eli Zaretskii
  2024-10-13 17:38                         ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-13  5:36 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Sat, 12 Oct 2024 18:36:27 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> +    ;; Check if the display property at the end of this line is "safe".
> +    (if (visual-wrap--display-property-safe-p
> +         (get-char-property (pos-eol) 'display))
> +        ;; If so, we can apply our visual wrapping properties to this
> +        ;; line and continue to the next line.
> +        (progn
> +          (visual-wrap--apply-to-line (point))
> +          (forward-line))
> +      ;; Otherwise, skip ahead to the beginning of the first line after
> +      ;; the unsafe display property changes.
> +      (goto-char (next-single-char-property-change (pos-eol) 'display))

Yes, this is what I had in mind, thanks.  But does
next-single-char-property-change really fit the bill?  What if you
have several overlapping 'display' properties, and in particular one
property "enclosed" in another?  Did you test such a scenario?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-13  5:36                       ` Eli Zaretskii
@ 2024-10-13 17:38                         ` Jim Porter
  2024-10-13 18:51                           ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-13 17:38 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

On 10/12/2024 10:36 PM, Eli Zaretskii wrote:
> Yes, this is what I had in mind, thanks.  But does
> next-single-char-property-change really fit the bill?  What if you
> have several overlapping 'display' properties, and in particular one
> property "enclosed" in another?  Did you test such a scenario?

I had tested those manually and they worked (according to my 
understanding of display properties, anyway). I've added some test cases 
to that effect in the latest patch.

Just to make sure we're on the same page: my (admittedly limited) 
understanding of xdisp.c is that, for replacing-display-properties, 
Emacs determines the range of buffer text to replace by using 'eq' on 
the 'display' property. So if you have an 'image' spec from positions 1 
to 10, but add a 'height' spec from positions 3 to 4, Emacs will now 
render the image three times: one each for the ranges [1, 3), [3, 4), 
and [4, 10).

This means that when 'visual-wrap-prefix-mode' sees the start of a 
replacing-display-property[1], we know we shouldn't interfere with the 
'eq'-ness of that run of text, so we move past it to the first position 
that's not 'eq' to the start. If you like, we treat the 
text-to-be-replaced as atomic, since (I think) that's roughly what 
xdisp.c does too.

[1] Technically, the visual-wrap code avoids adding properties to some 
non-replacing properties too, but since "do nothing" should always be 
safe, that shouldn't be a problem. (It could result in some text not 
having a wrap-prefix when we'd like it too, but we can always add new 
"safe" display specs if someone notices an issue; I wanted to err on the 
side of caution with what we consider safe.)

[-- Attachment #2: 0001-Don-t-add-visual-wrap-prefix-properties-to-unsafe-mu.patch --]
[-- Type: text/plain, Size: 10033 bytes --]

From 33ddaae3aacd5b5eb1c823f3461019b53ca02c2e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to unsafe 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): Use 'pos-eol'; we don't want to respect
field boundaries here.
(visual-wrap-prefix-function): Check for unsafe display properties at
the end of the line and skip past them if present.

* test/lisp/visual-wrap-tests.el: New test file.
---
 lisp/visual-wrap.el            |  48 +++++++++++--
 test/lisp/visual-wrap-tests.el | 124 +++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 4 deletions(-)
 create mode 100644 test/lisp/visual-wrap-tests.el

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..26333089189 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,36 @@ 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.
+A \"safe\" display spec is one that won't interfere with the additional
+text properties that `visual-wrap-prefix-mode' uses.
+
+Specs that replace the text are unsafe, since they generally determine
+the range of text to replace via `eq'.  If `visual-wrap-prefix-mode'
+were to add text properties to some subset of this range, it would
+violate this assumption.")
+
+(defun visual-wrap--display-property-safe-p (display)
+  "Return non-nil if the display property DISPLAY is \"safe\".
+A \"safe\" display property is one where all the display specs are
+members of `visual-wrap--safe-display-specs' (which see)."
+  ;; The display property could be a single display spec; if so, wrap it
+  ;; in a list so we can iterate over it in our loop below.
+  (when (and (consp display) (not (consp (car display))))
+    (setq display (list display)))
+  ;; Loop over all the display specs to check if they're safe.  Assume
+  ;; any display property other than a vector or list (e.g. a string) is
+  ;; unsafe.
+  (when (or (vectorp display) (listp display))
+    (not (catch 'unsafe
+           (mapc (lambda (spec)
+                   (unless (memq (car-safe 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))
@@ -128,11 +158,11 @@ visual-wrap--apply-to-line
         ;; the buffer.)
         (add-display-text-property
          position (min (+ position (length first-line-prefix))
-                       (line-end-position))
+                       (pos-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
+       position (pos-eol) 'wrap-prefix
        (if (numberp next-line-prefix)
            `(space :align-to (,next-line-prefix . width))
          next-line-prefix)))))
@@ -206,8 +236,18 @@ visual-wrap-prefix-function
   (forward-line 0)
   (setq beg (point))
   (while (< (point) end)
-    (visual-wrap--apply-to-line (point))
-    (forward-line))
+    ;; Check if the display property at the end of this line is "safe".
+    (if (visual-wrap--display-property-safe-p
+         (get-char-property (pos-eol) 'display))
+        ;; If so, we can apply our visual wrapping properties to this
+        ;; line and continue to the next line.
+        (progn
+          (visual-wrap--apply-to-line (point))
+          (forward-line))
+      ;; Otherwise, skip ahead to the beginning of the first line after
+      ;; the unsafe display property changes.
+      (goto-char (next-single-char-property-change (pos-eol) 'display))
+      (unless (bolp) (forward-line 1))))
   `(jit-lock-bounds ,beg . ,end))
 
 ;;;###autoload
diff --git a/test/lisp/visual-wrap-tests.el b/test/lisp/visual-wrap-tests.el
new file mode 100644
index 00000000000..7cae3761b6a
--- /dev/null
+++ b/test/lisp/visual-wrap-tests.el
@@ -0,0 +1,124 @@
+;;; visual-wrap-tests.el --- Tests for `visual-wrap-prefix-mode'  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tets for `visual-wrap-prefix-mode'.
+
+;;; Code:
+
+(require 'visual-wrap)
+(require 'ert)
+
+;;; Tests:
+
+(ert-deftest visual-wrap-tests/simple ()
+  "Test adding wrapping properties to text without display properties."
+  (with-temp-buffer
+    (insert "greetings\n* hello\n* hi")
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("greetings\n* hello\n* hi"
+               10 12 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               12 17 ( wrap-prefix (space :align-to (2 . width)))
+               18 20 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               20 22 ( wrap-prefix (space :align-to (2 . width))))))))
+
+(ert-deftest visual-wrap-tests/safe-display ()
+  "Test adding wrapping properties to text with safe display properties."
+  (with-temp-buffer
+    (insert #("* hello" 2 7 (display (raise 1))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (message "%S" (buffer-string))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* hello"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (raise 1)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/within-line ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties don't extend across multiple lines,
+`visual-wrap-prefix-mode' can still add wrapping properties."
+  (with-temp-buffer
+    (insert #("* [img]" 2 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (message "%S" (buffer-string))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* [img]"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/spanning-lines ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties do extend across multiple lines,
+`visual-wrap-prefix-mode' must avoid adding wrapping properties."
+  (with-temp-buffer
+    (insert #("* a\n* b" 0 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b" 0 7 (display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-1 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by a
+single-line unsafe display prop.  `visual-wrap-prefix-mode' should *not*
+add wrapping properties to the first block, but do add them to the
+second."
+  (with-temp-buffer
+    (insert #("* a\n* b"
+              0 4 (display ((image :type bmp)))
+              4 7 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b"
+               0 4 ( display ((image :type bmp)))
+               4 6 ( wrap-prefix (space :align-to (2 . width))
+                     display ((min-width ((2 . width)))
+                              (image :type bmp) (height 1.5)))
+               6 7 ( wrap-prefix (space :align-to (2 . width))
+                     display ((image :type bmp) (height 1.5))))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-2 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by
+another multi-line unsafe display prop.  `visual-wrap-prefix-mode'
+should *not* add wrapping properties to either block."
+  (with-temp-buffer
+    (insert #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5))))))))
+
+;; visual-wrap-tests.el ends here
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-13 17:38                         ` Jim Porter
@ 2024-10-13 18:51                           ` Eli Zaretskii
  2024-10-13 20:50                             ` Jim Porter
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-13 18:51 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Sun, 13 Oct 2024 10:38:09 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 10/12/2024 10:36 PM, Eli Zaretskii wrote:
> > Yes, this is what I had in mind, thanks.  But does
> > next-single-char-property-change really fit the bill?  What if you
> > have several overlapping 'display' properties, and in particular one
> > property "enclosed" in another?  Did you test such a scenario?
> 
> I had tested those manually and they worked (according to my 
> understanding of display properties, anyway). I've added some test cases 
> to that effect in the latest patch.
> 
> Just to make sure we're on the same page: my (admittedly limited) 
> understanding of xdisp.c is that, for replacing-display-properties, 
> Emacs determines the range of buffer text to replace by using 'eq' on 
> the 'display' property. So if you have an 'image' spec from positions 1 
> to 10, but add a 'height' spec from positions 3 to 4, Emacs will now 
> render the image three times: one each for the ranges [1, 3), [3, 4), 
> and [4, 10).

That is probably correct, at least in some cases, but why should we
rely on that for this purpose?  Isn't it more reliable and
future-proof to skip the entire span of the outermost "replacing"
display property, and never look at properties inside that text?  Does
it really matter for visual-wrap-prefix-mode to look inside?

IOW, I prefer simplicity of the logic to sophisticated design based on
what we actually see Emacs do in tricky cases, because time and again
I learned the hard way that my mental models of what actually happens
can be erroneous in the fine details.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-13 18:51                           ` Eli Zaretskii
@ 2024-10-13 20:50                             ` Jim Porter
  2024-10-19  8:23                               ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jim Porter @ 2024-10-13 20:50 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 73600, gautier

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

On 10/13/2024 11:51 AM, Eli Zaretskii wrote:
> That is probably correct, at least in some cases, but why should we
> rely on that for this purpose?  Isn't it more reliable and
> future-proof to skip the entire span of the outermost "replacing"
> display property, and never look at properties inside that text?  Does
> it really matter for visual-wrap-prefix-mode to look inside?

As I understand this bug, we just need to make sure that visual-wrap.el 
doesn't change the display property of any single/atomic span of text 
that xdisp.c will replace. Otherwise, xdisp.c will stop at every change 
to 'display' in turn, leading to duplicated images (or whatever we're 
displaying).

I think the most reliable/future-proof way to fix this would be work as 
much like xdisp.c as we can. From what I can tell, xdisp.c is using 
'next-single-char-property-change' for this: for 
replacing-display-props, 'handle_single_display_spec' calls 
'display_prop_end', which calls 'Fnext_single_char_property_change'.

That's what my patch is doing, though it's in Lisp, and it's erring on 
the side of caution with what display specs could be "unsafe".

> IOW, I prefer simplicity of the logic to sophisticated design based on
> what we actually see Emacs do in tricky cases, because time and again
> I learned the hard way that my mental models of what actually happens
> can be erroneous in the fine details.

I've attached a patch that isn't *exactly* what you suggested, but 
should add an extra layer of safety at the cost of possibly failing to 
add wrapping properties in when we should/could. In this version 
("be-extra-careful.patch"), whenever we find an unsafe display property 
at the end of a line, we just skip ahead until we find the first 
known-safe display property.

This is the best I could come up with without adding a lot of extra 
complexity to visual-wrap.el, which I think would have just made it more 
prone to mistakes. If you have any other ideas though, let me know.

I also attached the patch I prefer ("match-xdisp.patch"), which is just 
what I posted previously with some additional details in the comments. 
Would you feel better about this version if I could add regression tests 
that ensure xdisp.c does what we expect? In previous work on 
visual-wrap, I looked a bit at doing this, and I think it would probably 
work if we ran the tests in a GUI frame. That probably requires some 
additional scaffolding in the Makefiles, but if it helps improve our 
confidence in the display engine, I think it's worth my time to attempt it.

[-- Attachment #2: be-extra-careful.patch --]
[-- Type: text/plain, Size: 10516 bytes --]

From 41d4b59dc4e6d075b8cce244ecf65ae5018637da Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to unsafe 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): Use 'pos-eol'; we don't want to respect
field boundaries here.
(visual-wrap-prefix-function): Check for unsafe display properties at
the end of the line and skip past them if present.

* test/lisp/visual-wrap-tests.el: New test file.
---
 lisp/visual-wrap.el            |  57 ++++++++++++++--
 test/lisp/visual-wrap-tests.el | 120 +++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+), 4 deletions(-)
 create mode 100644 test/lisp/visual-wrap-tests.el

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..c3e28e6dd06 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,36 @@ 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.
+A \"safe\" display spec is one that won't interfere with the additional
+text properties that `visual-wrap-prefix-mode' uses.
+
+Specs that replace the text are unsafe, since they generally determine
+the range of text to replace via `eq'.  If `visual-wrap-prefix-mode'
+were to add text properties to some subset of this range, it would
+violate this assumption.")
+
+(defun visual-wrap--display-property-safe-p (display)
+  "Return non-nil if the display property DISPLAY is \"safe\".
+A \"safe\" display property is one where all the display specs are
+members of `visual-wrap--safe-display-specs' (which see)."
+  ;; The display property could be a single display spec; if so, wrap it
+  ;; in a list so we can iterate over it in our loop below.
+  (when (and (consp display) (not (consp (car display))))
+    (setq display (list display)))
+  ;; Loop over all the display specs to check if they're safe.  Assume
+  ;; any display property other than a vector or list (e.g. a string) is
+  ;; unsafe.
+  (when (or (vectorp display) (listp display))
+    (not (catch 'unsafe
+           (mapc (lambda (spec)
+                   (unless (memq (car-safe 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))
@@ -128,11 +158,11 @@ visual-wrap--apply-to-line
         ;; the buffer.)
         (add-display-text-property
          position (min (+ position (length first-line-prefix))
-                       (line-end-position))
+                       (pos-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
+       position (pos-eol) 'wrap-prefix
        (if (numberp next-line-prefix)
            `(space :align-to (,next-line-prefix . width))
          next-line-prefix)))))
@@ -206,8 +236,27 @@ visual-wrap-prefix-function
   (forward-line 0)
   (setq beg (point))
   (while (< (point) end)
-    (visual-wrap--apply-to-line (point))
-    (forward-line))
+    ;; Check if the display property at the end of this line is "safe".
+    (if (visual-wrap--display-property-safe-p
+         (get-char-property (pos-eol) 'display))
+        ;; If so, we can apply our visual wrapping properties to this
+        ;; line and continue to the next line.
+        (progn
+          (visual-wrap--apply-to-line (point))
+          (forward-line))
+      ;; Otherwise, skip ahead until the end of any unsafe display
+      ;; properties.  NOTE: We do this out of an abundance of caution to
+      ;; be as certain as possible that we're not interfering with the
+      ;; display engine.  If this results in cases where we fail to add
+      ;; wrapping properties when we should, then we should remove the
+      ;; `while' loop below.  Without that loop, this should be the same
+      ;; logic `handle_single_display_spec' in xdisp.c uses for
+      ;; determining what text to replace.  See bug#73600.
+      (goto-char (next-single-char-property-change (pos-eol) 'display))
+      (while (not (visual-wrap--display-property-safe-p
+                   (get-char-property (point) 'display)))
+        (goto-char (next-single-char-property-change (point) 'display)))
+      (unless (bolp) (forward-line 1))))
   `(jit-lock-bounds ,beg . ,end))
 
 ;;;###autoload
diff --git a/test/lisp/visual-wrap-tests.el b/test/lisp/visual-wrap-tests.el
new file mode 100644
index 00000000000..04977afe207
--- /dev/null
+++ b/test/lisp/visual-wrap-tests.el
@@ -0,0 +1,120 @@
+;;; visual-wrap-tests.el --- Tests for `visual-wrap-prefix-mode'  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tets for `visual-wrap-prefix-mode'.
+
+;;; Code:
+
+(require 'visual-wrap)
+(require 'ert)
+
+;;; Tests:
+
+(ert-deftest visual-wrap-tests/simple ()
+  "Test adding wrapping properties to text without display properties."
+  (with-temp-buffer
+    (insert "greetings\n* hello\n* hi")
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("greetings\n* hello\n* hi"
+               10 12 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               12 17 ( wrap-prefix (space :align-to (2 . width)))
+               18 20 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               20 22 ( wrap-prefix (space :align-to (2 . width))))))))
+
+(ert-deftest visual-wrap-tests/safe-display ()
+  "Test adding wrapping properties to text with safe display properties."
+  (with-temp-buffer
+    (insert #("* hello" 2 7 (display (raise 1))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* hello"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (raise 1)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/within-line ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties don't extend across multiple lines,
+`visual-wrap-prefix-mode' can still add wrapping properties."
+  (with-temp-buffer
+    (insert #("* [img]" 2 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* [img]"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/spanning-lines ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties do extend across multiple lines,
+`visual-wrap-prefix-mode' must avoid adding wrapping properties."
+  (with-temp-buffer
+    (insert #("* a\n* b" 0 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b" 0 7 (display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-1 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by a
+single-line unsafe display prop.  `visual-wrap-prefix-mode' should *not*
+add wrapping properties to either block."
+  (with-temp-buffer
+    (insert #("* a\n* b"
+              0 4 (display ((image :type bmp)))
+              4 7 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             ;; NOTE: See the note in `visual-wrap-prefix-function'.  If
+             ;; applying the change mentioned there, then this case
+             ;; should add wrapping properties to the second block.
+             #("* a\n* b"
+              0 4 (display ((image :type bmp)))
+              4 7 (display ((image :type bmp) (height 1.5))))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-2 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by
+another multi-line unsafe display prop.  `visual-wrap-prefix-mode'
+should *not* add wrapping properties to either block."
+  (with-temp-buffer
+    (insert #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5))))))))
+
+;; visual-wrap-tests.el ends here
-- 
2.25.1


[-- Attachment #3: match-xdisp.patch --]
[-- Type: text/plain, Size: 10085 bytes --]

From 6d4597e27c99548105317d483121325d2c98bfda Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Oct 2024 17:24:18 -0700
Subject: [PATCH] Don't add visual-wrap-prefix properties to unsafe 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): Use 'pos-eol'; we don't want to respect
field boundaries here.
(visual-wrap-prefix-function): Check for unsafe display properties at
the end of the line and skip past them if present.

* test/lisp/visual-wrap-tests.el: New test file.
---
 lisp/visual-wrap.el            |  50 ++++++++++++--
 test/lisp/visual-wrap-tests.el | 122 +++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 4 deletions(-)
 create mode 100644 test/lisp/visual-wrap-tests.el

diff --git a/lisp/visual-wrap.el b/lisp/visual-wrap.el
index 76276c0f474..87dd0bccacd 100644
--- a/lisp/visual-wrap.el
+++ b/lisp/visual-wrap.el
@@ -73,6 +73,36 @@ 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.
+A \"safe\" display spec is one that won't interfere with the additional
+text properties that `visual-wrap-prefix-mode' uses.
+
+Specs that replace the text are unsafe, since they generally determine
+the range of text to replace via `eq'.  If `visual-wrap-prefix-mode'
+were to add text properties to some subset of this range, it would
+violate this assumption.")
+
+(defun visual-wrap--display-property-safe-p (display)
+  "Return non-nil if the display property DISPLAY is \"safe\".
+A \"safe\" display property is one where all the display specs are
+members of `visual-wrap--safe-display-specs' (which see)."
+  ;; The display property could be a single display spec; if so, wrap it
+  ;; in a list so we can iterate over it in our loop below.
+  (when (and (consp display) (not (consp (car display))))
+    (setq display (list display)))
+  ;; Loop over all the display specs to check if they're safe.  Assume
+  ;; any display property other than a vector or list (e.g. a string) is
+  ;; unsafe.
+  (when (or (vectorp display) (listp display))
+    (not (catch 'unsafe
+           (mapc (lambda (spec)
+                   (unless (memq (car-safe 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))
@@ -128,11 +158,11 @@ visual-wrap--apply-to-line
         ;; the buffer.)
         (add-display-text-property
          position (min (+ position (length first-line-prefix))
-                       (line-end-position))
+                       (pos-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
+       position (pos-eol) 'wrap-prefix
        (if (numberp next-line-prefix)
            `(space :align-to (,next-line-prefix . width))
          next-line-prefix)))))
@@ -206,8 +236,20 @@ visual-wrap-prefix-function
   (forward-line 0)
   (setq beg (point))
   (while (< (point) end)
-    (visual-wrap--apply-to-line (point))
-    (forward-line))
+    ;; Check if the display property at the end of this line is "safe".
+    (if (visual-wrap--display-property-safe-p
+         (get-char-property (pos-eol) 'display))
+        ;; If so, we can apply our visual wrapping properties to this
+        ;; line and continue to the next line.
+        (progn
+          (visual-wrap--apply-to-line (point))
+          (forward-line))
+      ;; Otherwise, skip ahead to the beginning of the first line after
+      ;; the end of the display property.  This is the same logic
+      ;; `handle_single_display_spec' in xdisp.c uses for determining
+      ;; what text to replace.
+      (goto-char (next-single-char-property-change (pos-eol) 'display))
+      (unless (bolp) (forward-line 1))))
   `(jit-lock-bounds ,beg . ,end))
 
 ;;;###autoload
diff --git a/test/lisp/visual-wrap-tests.el b/test/lisp/visual-wrap-tests.el
new file mode 100644
index 00000000000..b62c1e5a05a
--- /dev/null
+++ b/test/lisp/visual-wrap-tests.el
@@ -0,0 +1,122 @@
+;;; visual-wrap-tests.el --- Tests for `visual-wrap-prefix-mode'  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tets for `visual-wrap-prefix-mode'.
+
+;;; Code:
+
+(require 'visual-wrap)
+(require 'ert)
+
+;;; Tests:
+
+(ert-deftest visual-wrap-tests/simple ()
+  "Test adding wrapping properties to text without display properties."
+  (with-temp-buffer
+    (insert "greetings\n* hello\n* hi")
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("greetings\n* hello\n* hi"
+               10 12 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               12 17 ( wrap-prefix (space :align-to (2 . width)))
+               18 20 ( wrap-prefix (space :align-to (2 . width))
+                       display (min-width ((2 . width))))
+               20 22 ( wrap-prefix (space :align-to (2 . width))))))))
+
+(ert-deftest visual-wrap-tests/safe-display ()
+  "Test adding wrapping properties to text with safe display properties."
+  (with-temp-buffer
+    (insert #("* hello" 2 7 (display (raise 1))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* hello"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (raise 1)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/within-line ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties don't extend across multiple lines,
+`visual-wrap-prefix-mode' can still add wrapping properties."
+  (with-temp-buffer
+    (insert #("* [img]" 2 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* [img]"
+               0 2 ( wrap-prefix (space :align-to (2 . width))
+                     display (min-width ((2 . width))))
+               2 7 ( wrap-prefix (space :align-to (2 . width))
+                     display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/spanning-lines ()
+  "Test adding wrapping properties to text with unsafe display properties.
+When these properties do extend across multiple lines,
+`visual-wrap-prefix-mode' must avoid adding wrapping properties."
+  (with-temp-buffer
+    (insert #("* a\n* b" 0 7 (display (image :type bmp))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b" 0 7 (display (image :type bmp)))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-1 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by a
+single-line unsafe display prop.  `visual-wrap-prefix-mode' should *not*
+add wrapping properties to the first block, but do add them to the
+second."
+  (with-temp-buffer
+    (insert #("* a\n* b"
+              0 4 (display ((image :type bmp)))
+              4 7 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b"
+               0 4 ( display ((image :type bmp)))
+               4 6 ( wrap-prefix (space :align-to (2 . width))
+                     display ((min-width ((2 . width)))
+                              (image :type bmp) (height 1.5)))
+               6 7 ( wrap-prefix (space :align-to (2 . width))
+                     display ((image :type bmp) (height 1.5))))))))
+
+(ert-deftest visual-wrap-tests/unsafe-display/multiple-2 ()
+  "Test adding wrapping properties to text with unsafe display properties.
+This tests a multi-line unsafe display prop immediately followed by
+another multi-line unsafe display prop.  `visual-wrap-prefix-mode'
+should *not* add wrapping properties to either block."
+  (with-temp-buffer
+    (insert #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5)))))
+    (visual-wrap-prefix-function (point-min) (point-max))
+    (should (equal-including-properties
+             (buffer-string)
+             #("* a\n* b\n"
+              0 4 (display ((image :type bmp)))
+              4 8 (display ((image :type bmp) (height 1.5))))))))
+
+;; visual-wrap-tests.el ends here
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#73600: 31.0.50; Visual wrap prefix mode and image display
  2024-10-13 20:50                             ` Jim Porter
@ 2024-10-19  8:23                               ` Eli Zaretskii
  0 siblings, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2024-10-19  8:23 UTC (permalink / raw)
  To: Jim Porter; +Cc: 73600, gautier

> Date: Sun, 13 Oct 2024 13:50:25 -0700
> Cc: 73600@debbugs.gnu.org, gautier@gautierponsinet.xyz
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 10/13/2024 11:51 AM, Eli Zaretskii wrote:
> > That is probably correct, at least in some cases, but why should we
> > rely on that for this purpose?  Isn't it more reliable and
> > future-proof to skip the entire span of the outermost "replacing"
> > display property, and never look at properties inside that text?  Does
> > it really matter for visual-wrap-prefix-mode to look inside?
> 
> As I understand this bug, we just need to make sure that visual-wrap.el 
> doesn't change the display property of any single/atomic span of text 
> that xdisp.c will replace. Otherwise, xdisp.c will stop at every change 
> to 'display' in turn, leading to duplicated images (or whatever we're 
> displaying).
> 
> I think the most reliable/future-proof way to fix this would be work as 
> much like xdisp.c as we can. From what I can tell, xdisp.c is using 
> 'next-single-char-property-change' for this: for 
> replacing-display-props, 'handle_single_display_spec' calls 
> 'display_prop_end', which calls 'Fnext_single_char_property_change'.

That's what xdisp.c does to detect the beginning of a property.  But
is that what it does to find the end of the property which is being
processed?

> That's what my patch is doing, though it's in Lisp, and it's erring on 
> the side of caution with what display specs could be "unsafe".
> 
> > IOW, I prefer simplicity of the logic to sophisticated design based on
> > what we actually see Emacs do in tricky cases, because time and again
> > I learned the hard way that my mental models of what actually happens
> > can be erroneous in the fine details.
> 
> I've attached a patch that isn't *exactly* what you suggested, but 
> should add an extra layer of safety at the cost of possibly failing to 
> add wrapping properties in when we should/could. In this version 
> ("be-extra-careful.patch"), whenever we find an unsafe display property 
> at the end of a line, we just skip ahead until we find the first 
> known-safe display property.
> 
> This is the best I could come up with without adding a lot of extra 
> complexity to visual-wrap.el, which I think would have just made it more 
> prone to mistakes. If you have any other ideas though, let me know.
> 
> I also attached the patch I prefer ("match-xdisp.patch"), which is just 
> what I posted previously with some additional details in the comments. 
> Would you feel better about this version if I could add regression tests 
> that ensure xdisp.c does what we expect? In previous work on 
> visual-wrap, I looked a bit at doing this, and I think it would probably 
> work if we ran the tests in a GUI frame. That probably requires some 
> additional scaffolding in the Makefiles, but if it helps improve our 
> confidence in the display engine, I think it's worth my time to attempt it.

I think at this stage it is basically up to you which version to
install.  Adding tests is always welcome, of course, regardless of
which version is installed.

Thanks.





^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-10-19  8:23 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-02 13:04 bug#73600: 31.0.50; Visual wrap prefix mode and image display Gautier Ponsinet
2024-10-02 15:42 ` Eli Zaretskii
2024-10-02 15:51   ` Eli Zaretskii
2024-10-02 16:37     ` Jim Porter
2024-10-03 10:59       ` Eli Zaretskii
2024-10-04  0:28         ` Jim Porter
2024-10-04  6:28           ` Eli Zaretskii
2024-10-04 19:45             ` Jim Porter
2024-10-05  6:41               ` Eli Zaretskii
2024-10-05 18:07                 ` Jim Porter
2024-10-06  0:28                   ` Jim Porter
2024-10-12 12:00                   ` Eli Zaretskii
2024-10-13  1:36                     ` Jim Porter
2024-10-13  5:36                       ` Eli Zaretskii
2024-10-13 17:38                         ` Jim Porter
2024-10-13 18:51                           ` Eli Zaretskii
2024-10-13 20:50                             ` Jim Porter
2024-10-19  8:23                               ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.