bug#69920: 29.2; gnus: article-mode: Toggling MIME inline attachment previews adds superfluous newlines

bug#69920: 29.2; gnus: article-mode: Toggling MIME inline attachment previews adds superfluous newlines

[Alcor via Bug reports for GNU Emacs, the Swiss army knife of text editors]

1. Receive an E-Mail message with at least two image attachments (I
tried with two image/png files, sent via SMTP and read via IMAP).

2. Go into the gnus summary buffer and view the received message/article.

3. In the "Attachments: ..." line, click on the attachment to toggle the
inline preview.

4. Click on the attachment button again to hide the inline preview.

5. Repeat steps 3-4 a number of times (e.g. 10-20 times).

6. Observe as newlines keep getting inserted and accumulate under the
preview.

Additionally, the following can be observed:
- Overlays are added to the attachment button, but are never cleaned up
(can be checked with `describe-char' with point on the button).

- If there is more than 1 inline-previewable attachment, clicking on the
button for attachment #1, then #2, then #1, and #2 in that order breaks
the preview state (the inline previews can no longer be hidden). This
may be unrelated, however (please advise if a separate bug should be
opened for this).


In GNU Emacs 29.2 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.20,
 cairo version 1.16.0) of 2024-03-13 built on lcy02-amd64-051
Repository revision: 900dc1b4bf3011e685a1ec7d7ce4dcd0262ec880
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12201001
System Description: Ubuntu 22.04.4 LTS

Configured using:
 'configure --prefix=/snap/emacs/current/usr --with-x-toolkit=gtk3
 --without-xaw3d --with-modules --with-cairo
 --with-native-compilation=aot --without-pgtk --with-xinput2
 --with-tree-sitter --with-json
 'CFLAGS=-isystem/build/emacs/parts/emacs/install/usr/include
 -isystem/build/emacs/parts/emacs/install/usr/include/x86_64-linux-gnu
 -isystem/build/emacs/stage/usr/include -O2'
 'CPPFLAGS=-isystem/build/emacs/parts/emacs/install/usr/include
 -isystem/build/emacs/parts/emacs/install/usr/include/x86_64-linux-gnu
 -isystem/build/emacs/stage/usr/include'
 'LDFLAGS=-L/build/emacs/parts/emacs/install/lib
 -L/build/emacs/parts/emacs/install/usr/lib
 -L/build/emacs/parts/emacs/install/lib/x86_64-linux-gnu
 -L/build/emacs/parts/emacs/install/usr/lib/x86_64-linux-gnu
 -L/build/emacs/stage/usr/lib''

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

Important settings:
  value of $LANG: de_DE.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Article

Minor modes in effect:
  shell-dirtrack-mode: t
  erc-ring-mode: t
  erc-notifications-mode: t
  erc-netsplit-mode: t
  erc-menu-mode: t
  erc-list-mode: t
  erc-irccontrols-mode: t
  erc-keep-place-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  erc-scrolltobottom-mode: t
  erc-imenu-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-bufbar-mode: t
  erc-track-mode: t
  erc-match-mode: t
  erc-autojoin-mode: t
  erc-autoaway-mode: t
  recentf-mode: t
  pixel-scroll-precision-mode: t
  minibuffer-depth-indicate-mode: t
  global-whitespace-mode: t
  global-goto-address-mode: t
  goto-address-mode: t
  global-auto-revert-mode: t
  fido-vertical-mode: t
  icomplete-vertical-mode: t
  icomplete-mode: t
  fido-mode: t
  erc-networks-mode: t
  desktop-save-mode: t
  windmove-mode: t
  tooltip-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
  blink-cursor-mode: t
  buffer-read-only: 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

Features:
(shadow emacsbug misearch multi-isearch qp descr-text elec-pair
mh-buffers mh-loaddefs gnus-registry registry eieio-base rmailsum rmail
gnus-salt gnus-topic gnus-uu yenc gnus-vm shortdoc help-fns radix-tree
mailalias smtpmail smerge-mode diff diff-mode sort smiley gnus-cite
mm-archive mail-extr textsec uni-scripts idna-mapping ucs-normalize
uni-confusable textsec-check gnus-async gnus-bcklg gnus-ml nndraft nnmh
nnfolder utf-7 rfc2104 network-stream nsm gnus-agent gnus-srvr
gnus-score score-mode nnvirtual gnus-msg nntp gnus-cache
display-line-numbers org-element org-persist org-id org-refile avl-tree
generator oc-basic 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 gnus-spec gnus-int gnus-range message yank-media puny rfc822 mml
mml-sec epa epg rfc6068 epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader gnus-win ol-docview
doc-view jka-compr image-mode exif dired dired-loaddefs ol-bibtex bibtex
ol-bbdb ol-w3m ol-doi org-link-doi org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete org-list
org-footnote org-faces org-entities noutline outline ob-emacs-lisp
ob-core ob-eval org-cycle org-table ol org-fold org-fold-core org-keys
oc org-loaddefs find-func cal-menu calendar cal-loaddefs org-version
org-compat org-macs tramp-cache time-stamp tramp-sh tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat shell parse-time iso8601
disp-table erc-ring erc-desktop-notifications notifications dbus xml
erc-nicks color erc-netsplit erc-menu erc-list erc-goodies erc-imenu
imenu erc-pcomplete pcomplete comint ansi-osc ansi-color erc-button
erc-fill erc-stamp erc-status-sidebar erc-track erc-match erc-join
erc-autoaway leuven-dark-theme recentf tree-widget pixel-scroll cua-base
ring mb-depth whitespace goto-addr thingatpt autorevert filenotify
icomplete erc derived format-spec erc-backend erc-networks easy-mmode
erc-common inline erc-compat pcase compat erc-loaddefs desktop frameset
sendmail rfc2047 rfc2045 ietf-drums gnus nnheader gnus-util
text-property-search time-date mail-utils range mm-util mail-prsvr
cus-edit pp cus-load wid-edit windmove xdg site-start comp comp-cstr
warnings icons rx cl-extra help-mode erc-autoloads info compat-autoloads
markdown-mode-autoloads 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 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 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
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 735396 84127)
 (symbols 48 46838 24)
 (strings 32 190158 4181)
 (string-bytes 1 5259546)
 (vectors 16 82583)
 (vector-slots 8 2226749 106897)
 (floats 8 679 1292)
 (intervals 56 3126 636)
 (buffers 984 33))

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Alcor via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Emacs maintainers,

After some investigation, I noticed that `mm-inline-image' adds a
newline via (insert "\n") directly after `insert-image' for unclear
reasons.

Removing the (insert "\n") form seems to resolve the issue (along with
the other issues mentioned under "Additionally, the following can be
observed") in the initial bug report message.

A patch is included illustrating the change made, but I'm unfamiliar
with the gnus codebase. It would be great if somebody more experienced
took a look.

Cheers,
-A.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-add-a-superfluous-newline-after-insert-image.patch --]
[-- Type: text/x-diff, Size: 818 bytes --]

From f04178b25010f1a91094bea6b4a0fcbc590b14b0 Mon Sep 17 00:00:00 2001
From: "F. Moukayed" <smfadi+emacs@gmail.com>
Date: Fri, 22 Mar 2024 20:46:21 +0000
Subject: [PATCH] Do not add a superfluous newline after `insert-image'.

* lisp/gnus/mm-view.el (mm-inline-image): Remove unnecessary (insert "\n") to resolve issues with toggling inline attachment previews (bug#69920).
---
 lisp/gnus/mm-view.el | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index 3a71920..50636de 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -99,7 +99,6 @@ This is only used if `mm-inline-large-images' is set to
 				 (- (nth 3 edges) (nth 1 edges)))))))
          image))
      "x")
-    (insert "\n")
     (mm-handle-set-undisplayer
      handle
      (lambda ()
-- 
2.34.1

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Alcor via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tags: patch

Add patch available mark to bug (forgot to do that in the previous
message).

git blame'ing the line in question on yields a decade-old
commit. Apparently, this used to be (insert "\n\n") … (delete-region b
(+ b 2)) about ~10 years ago, and the last relevant change turned that
into (insert "\n") … (delete-region b (1+ b)), but it seems lots of
other things on that codepath changed in the meantime.

FWIW, (insert-image … "x") (insert "\n") inserts two characters,
i.e. the image itself "x" and "\n", so it should either be (insert "\n")
… (delete-region b (+ 2 b)) or, alternatively just (delete-region b (1+
b)) (without any `insert' call).

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eli Zaretskii]

> Date: Fri, 22 Mar 2024 22:01:09 +0100
> From:  Alcor via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> After some investigation, I noticed that `mm-inline-image' adds a
> newline via (insert "\n") directly after `insert-image' for unclear
> reasons.

I'm guessing this is to make sure text following the image starts on a
new line, rather than after the image.

Why is the current code a problem?  IOW, when does the extra newline
cause any harm?  You talk about "the issue", but I didn't see any
explanation what that issue is.

Eric, any comments?

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eli Zaretskii]

> Date: Fri, 22 Mar 2024 23:25:20 +0100
> From:  Alcor via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> git blame'ing the line in question on yields a decade-old
> commit. Apparently, this used to be (insert "\n\n") … (delete-region b
> (+ b 2)) about ~10 years ago, and the last relevant change turned that
> into (insert "\n") … (delete-region b (1+ b)), but it seems lots of
> other things on that codepath changed in the meantime.

That commit explains the rationale:

  commit 14ff920dc885636a763d6ab7f256cc9981c24781
  Author:     Katsumi Yamaoka <yamaoka@jpl.org>
  AuthorDate: Fri May 2 09:44:34 2014 +0000
  Commit:     Katsumi Yamaoka <yamaoka@jpl.org>
  CommitDate: Fri May 2 09:44:34 2014 +0000

      Gnus: Make gnus-mime-inline-part and gnus-mime-inline-part work similarly

> FWIW, (insert-image … "x") (insert "\n") inserts two characters,
> i.e. the image itself "x" and "\n"

The code inserts the character "x" with a 'display' property on it
(which causes the image to be shown instead of "x"), followed by the
newline.

> so it should either be (insert "\n") … (delete-region b (+ 2 b)) or,
> alternatively just (delete-region b (1+ b)) (without any `insert'
> call).

Are you saying that the problem is with the function that
"un-displays" the inline image, in that it fails to remove the
inserted newline?  (AFAIU, the code before the above commit also had
the same issue.)  That wasn't clear from the description of the
problem, and the Subject is ambiguous wrt what newlines are deemed
"superfluous".  So please clarify what is the problem you are flagging
here.

Thanks.

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Alcor via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Are you saying that the problem is with the function that
> "un-displays" the inline image, in that it fails to remove the
> inserted newline?  (AFAIU, the code before the above commit also had
> the same issue.)  That wasn't clear from the description of the
> problem, and the Subject is ambiguous wrt what newlines are deemed
> "superfluous".  So please clarify what is the problem you are flagging
> here.

The problem is that the undisplayer does not correctly undo what the
displayer does.

The undisplayer (as it is right now) just removes the image. That can be
confirmed by stepping through the code with edebug or by inspecting the
"b" variable.

I think we can agree that (delete-region b (1+ b)) will always delete
_exactly one_ character, and in that case that would be the propertized
"x" with the image. That would leave a dangling newline.

There are two ways to solve this:

1. Do not add the extra newline (this is what the patch does).
2. Remove the extra newline via (delete-region b (+ b 2)) – note that I
have not tried this, but it would make sense to me.

I happen to prefer option #1 as the extra newline does not seem to have
any meaningful function. But this is just my own preference (Emacs/gnus
maintainers may wish to retain the extra newline if it serves a valid purpose).

If it helps clarify things, I'm okay with renaming the bug report to
something like "MIME inline image preview undisplayer does not clean up
displayed image correctly" or something along these lines. When I filed
the report, I wasn't sure about the cause so I described the user-facing
issue instead of the actual technical problem.

Cheers,
-A.

PS: I'm not sure the original code from
before 14ff920dc885636a763d6ab7f256cc9981c24781 was correct either. It
used to insert "x\n\n" (3 characters) on display (x being the
propertized image) and removed via (delete-region b (+ b 2)) exactly 2
characters. The new code after that revision inserted "x\n" (2
characters, x being the propertized image) and removed via
(delete-region b (1+ b)) exactly 1 character. So it might be possible
that this off-by-one error in `mm-inline-image' has always existed.

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eli Zaretskii]

> From: Alcor <alcor@tilde.club>
> Cc: 69920@debbugs.gnu.org
> Date: Sat, 23 Mar 2024 10:53:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Are you saying that the problem is with the function that
> > "un-displays" the inline image, in that it fails to remove the
> > inserted newline?  (AFAIU, the code before the above commit also had
> > the same issue.)  That wasn't clear from the description of the
> > problem, and the Subject is ambiguous wrt what newlines are deemed
> > "superfluous".  So please clarify what is the problem you are flagging
> > here.
> 
> The problem is that the undisplayer does not correctly undo what the
> displayer does.
> 
> The undisplayer (as it is right now) just removes the image. That can be
> confirmed by stepping through the code with edebug or by inspecting the
> "b" variable.
> 
> I think we can agree that (delete-region b (1+ b)) will always delete
> _exactly one_ character, and in that case that would be the propertized
> "x" with the image. That would leave a dangling newline.
> 
> There are two ways to solve this:
> 
> 1. Do not add the extra newline (this is what the patch does).
> 2. Remove the extra newline via (delete-region b (+ b 2)) – note that I
> have not tried this, but it would make sense to me.
> 
> I happen to prefer option #1 as the extra newline does not seem to have
> any meaningful function. But this is just my own preference (Emacs/gnus
> maintainers may wish to retain the extra newline if it serves a valid purpose).

I prefer #2.  Eric, WDYT?

> PS: I'm not sure the original code from
> before 14ff920dc885636a763d6ab7f256cc9981c24781 was correct either. It
> used to insert "x\n\n" (3 characters) on display (x being the
> propertized image) and removed via (delete-region b (+ b 2)) exactly 2
> characters. The new code after that revision inserted "x\n" (2
> characters, x being the propertized image) and removed via
> (delete-region b (1+ b)) exactly 1 character. So it might be possible
> that this off-by-one error in `mm-inline-image' has always existed.

Yes, I think so.

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eli Zaretskii]

Ping! Eric, can you please chime in?

> Cc: 69920@debbugs.gnu.org
> Date: Sat, 23 Mar 2024 12:20:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Alcor <alcor@tilde.club>
> > Cc: 69920@debbugs.gnu.org
> > Date: Sat, 23 Mar 2024 10:53:22 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Are you saying that the problem is with the function that
> > > "un-displays" the inline image, in that it fails to remove the
> > > inserted newline?  (AFAIU, the code before the above commit also had
> > > the same issue.)  That wasn't clear from the description of the
> > > problem, and the Subject is ambiguous wrt what newlines are deemed
> > > "superfluous".  So please clarify what is the problem you are flagging
> > > here.
> > 
> > The problem is that the undisplayer does not correctly undo what the
> > displayer does.
> > 
> > The undisplayer (as it is right now) just removes the image. That can be
> > confirmed by stepping through the code with edebug or by inspecting the
> > "b" variable.
> > 
> > I think we can agree that (delete-region b (1+ b)) will always delete
> > _exactly one_ character, and in that case that would be the propertized
> > "x" with the image. That would leave a dangling newline.
> > 
> > There are two ways to solve this:
> > 
> > 1. Do not add the extra newline (this is what the patch does).
> > 2. Remove the extra newline via (delete-region b (+ b 2)) – note that I
> > have not tried this, but it would make sense to me.
> > 
> > I happen to prefer option #1 as the extra newline does not seem to have
> > any meaningful function. But this is just my own preference (Emacs/gnus
> > maintainers may wish to retain the extra newline if it serves a valid purpose).
> 
> I prefer #2.  Eric, WDYT?
> 
> > PS: I'm not sure the original code from
> > before 14ff920dc885636a763d6ab7f256cc9981c24781 was correct either. It
> > used to insert "x\n\n" (3 characters) on display (x being the
> > propertized image) and removed via (delete-region b (+ b 2)) exactly 2
> > characters. The new code after that revision inserted "x\n" (2
> > characters, x being the propertized image) and removed via
> > (delete-region b (1+ b)) exactly 1 character. So it might be possible
> > that this off-by-one error in `mm-inline-image' has always existed.
> 
> Yes, I think so.
> 
> 
> 
> 

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eli Zaretskii]

Ping! Ping! Eric, please chime in.

> Cc: 69920@debbugs.gnu.org, alcor@tilde.club
> Date: Sat, 06 Apr 2024 11:59:33 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping! Eric, can you please chime in?
> 
> > Cc: 69920@debbugs.gnu.org
> > Date: Sat, 23 Mar 2024 12:20:16 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > > From: Alcor <alcor@tilde.club>
> > > Cc: 69920@debbugs.gnu.org
> > > Date: Sat, 23 Mar 2024 10:53:22 +0100
> > > 
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > > 
> > > > Are you saying that the problem is with the function that
> > > > "un-displays" the inline image, in that it fails to remove the
> > > > inserted newline?  (AFAIU, the code before the above commit also had
> > > > the same issue.)  That wasn't clear from the description of the
> > > > problem, and the Subject is ambiguous wrt what newlines are deemed
> > > > "superfluous".  So please clarify what is the problem you are flagging
> > > > here.
> > > 
> > > The problem is that the undisplayer does not correctly undo what the
> > > displayer does.
> > > 
> > > The undisplayer (as it is right now) just removes the image. That can be
> > > confirmed by stepping through the code with edebug or by inspecting the
> > > "b" variable.
> > > 
> > > I think we can agree that (delete-region b (1+ b)) will always delete
> > > _exactly one_ character, and in that case that would be the propertized
> > > "x" with the image. That would leave a dangling newline.
> > > 
> > > There are two ways to solve this:
> > > 
> > > 1. Do not add the extra newline (this is what the patch does).
> > > 2. Remove the extra newline via (delete-region b (+ b 2)) – note that I
> > > have not tried this, but it would make sense to me.
> > > 
> > > I happen to prefer option #1 as the extra newline does not seem to have
> > > any meaningful function. But this is just my own preference (Emacs/gnus
> > > maintainers may wish to retain the extra newline if it serves a valid purpose).
> > 
> > I prefer #2.  Eric, WDYT?
> > 
> > > PS: I'm not sure the original code from
> > > before 14ff920dc885636a763d6ab7f256cc9981c24781 was correct either. It
> > > used to insert "x\n\n" (3 characters) on display (x being the
> > > propertized image) and removed via (delete-region b (+ b 2)) exactly 2
> > > characters. The new code after that revision inserted "x\n" (2
> > > characters, x being the propertized image) and removed via
> > > (delete-region b (1+ b)) exactly 1 character. So it might be possible
> > > that this off-by-one error in `mm-inline-image' has always existed.
> > 
> > Yes, I think so.
> > 
> > 
> > 
> > 
> 
> 
> 
> 

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Eric Abrahamsen]

On 04/18/24 12:00 PM, Eli Zaretskii wrote:
> Ping! Ping! Eric, please chime in.

Sorry this has taken so long!

>> Cc: 69920@debbugs.gnu.org, alcor@tilde.club
>> Date: Sat, 06 Apr 2024 11:59:33 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> Ping! Eric, can you please chime in?
>> 
>> > Cc: 69920@debbugs.gnu.org
>> > Date: Sat, 23 Mar 2024 12:20:16 +0200
>> > From: Eli Zaretskii <eliz@gnu.org>
>> > 
>> > > From: Alcor <alcor@tilde.club>
>> > > Cc: 69920@debbugs.gnu.org
>> > > Date: Sat, 23 Mar 2024 10:53:22 +0100
>> > > 
>> > > Eli Zaretskii <eliz@gnu.org> writes:
>> > > 
>> > > > Are you saying that the problem is with the function that
>> > > > "un-displays" the inline image, in that it fails to remove the
>> > > > inserted newline?  (AFAIU, the code before the above commit also had
>> > > > the same issue.)  That wasn't clear from the description of the
>> > > > problem, and the Subject is ambiguous wrt what newlines are deemed
>> > > > "superfluous".  So please clarify what is the problem you are flagging
>> > > > here.
>> > > 
>> > > The problem is that the undisplayer does not correctly undo what the
>> > > displayer does.
>> > > 
>> > > The undisplayer (as it is right now) just removes the image. That can be
>> > > confirmed by stepping through the code with edebug or by inspecting the
>> > > "b" variable.
>> > > 
>> > > I think we can agree that (delete-region b (1+ b)) will always delete
>> > > _exactly one_ character, and in that case that would be the propertized
>> > > "x" with the image. That would leave a dangling newline.
>> > > 
>> > > There are two ways to solve this:
>> > > 
>> > > 1. Do not add the extra newline (this is what the patch does).
>> > > 2. Remove the extra newline via (delete-region b (+ b 2)) – note that I
>> > > have not tried this, but it would make sense to me.
>> > > 
>> > > I happen to prefer option #1 as the extra newline does not seem to have
>> > > any meaningful function. But this is just my own preference (Emacs/gnus
>> > > maintainers may wish to retain the extra newline if it serves a valid purpose).
>> > 
>> > I prefer #2.  Eric, WDYT?

I also prefer #2, mostly because the other `mm-inline-*' functions here
all ensure that inlined content ends with a newline, and even if there's
no immediate consequence to not having it, I'd prefer the consistency.
Alcor, would you be willing to update your patch?

Thanks,
Eric

bug#69920: Proposed fix - Toggling MIME inline attachment previews adds superfluous newlines

[Alcor via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I also prefer #2, mostly because the other `mm-inline-*' functions here
> all ensure that inlined content ends with a newline, and even if there's
> no immediate consequence to not having it, I'd prefer the consistency.
> Alcor, would you be willing to update your patch?

Hello Eric, Eli –

Attached is an updated patch adjusting the `delete-region' call in the
undisplayer to account for the additional newline, as discussed earlier.

I have tested this change on Emacs 29.3 (Linux/GTK) with multiple
image/png attachments and can confirm it works.

Cheers,
-A.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-that-inline-preview-undisplayer-removes-added.patch --]
[-- Type: text/x-diff, Size: 960 bytes --]

From 31578f2c8d538eb4b944c073d4804f355f88f035 Mon Sep 17 00:00:00 2001
From: "F. Moukayed" <smfadi+emacs@gmail.com>
Date: Mon, 22 Apr 2024 12:33:08 +0000
Subject: [PATCH] Ensure that inline preview undisplayer removes added newline

* lisp/gnus/mm-view.el (mm-inline-image): Remove the regioninterval b..b+2 (aka "x\n") instead of removing b..b+1 ("x") and leaving behind a superfluous newline (bug#69920).
---
 lisp/gnus/mm-view.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index 109b6c17c2..223da19a16 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -105,7 +105,7 @@ This is only used if `mm-inline-large-images' is set to
      (lambda ()
        (let ((inhibit-read-only t))
 	 (remove-images b b)
-	 (delete-region b (1+ b)))))))
+	 (delete-region b (+ b 2)))))))
 
 (defvar mm-w3m-setup nil
   "Whether gnus-article-mode has been setup to use emacs-w3m.")
-- 
2.34.1

bug#69920: 29.2; gnus: article-mode: Toggling MIME inline attachment previews adds superfluous newlines

[Eric Abrahamsen]

Alcor <alcor@tilde.club> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I also prefer #2, mostly because the other `mm-inline-*' functions here
>> all ensure that inlined content ends with a newline, and even if there's
>> no immediate consequence to not having it, I'd prefer the consistency.
>> Alcor, would you be willing to update your patch?
>
> Hello Eric, Eli –
>
> Attached is an updated patch adjusting the `delete-region' call in the
> undisplayer to account for the additional newline, as discussed earlier.
>
> I have tested this change on Emacs 29.3 (Linux/GTK) with multiple
> image/png attachments and can confirm it works.

In it goes -- thanks a lot.

Eric