* 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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 0 siblings, 0 replies; 17+ 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] 17+ messages in thread
end of thread, other threads:[~2024-10-13 20:50 UTC | newest] Thread overview: 17+ 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
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).
![[-- Attachment #4: rendered.png --] [-- Type: image/png, Size: 26584 bytes --]](../../c5a392f2-c19e-77a7-39f6-72b68b07ab1e@gmail.com/4-rendered.png){kind=link}