bug#65734: 29.1.50; kill-whole-line and visibility of Org subtrees

bug#65734: 29.1.50; kill-whole-line and visibility of Org subtrees

[Sebastian Miele]

In an emacs -Q, create an Org buffer with the following contents:

<-----cut-here----->
* AB
** C
<-----cut-here----->

Fold the subtree under the heading AB, so that only a single line is
diplayed (ending in "...").  With point between A and B, hit
C-S-<backspace> (kill-whole-line).

Expected: The whole _visible_ line, i.e., the entire contents of the
buffer is erased.  Actual behavior: The line with heading C remains.

Contrast this with the same experiment, except that the point is at the
beginning of the line containing AB when hitting C-S-<backspace>.  Then
the expected behavior happens.  And according to the source of
kill-whole-line, the intended effect indeed is to kill a whole _visible_
line.

The following patch fixes the issue:

diff --git a/lisp/simple.el b/lisp/simple.el
index abd587245fe..44221f3fc24 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6649,9 +6649,7 @@ kill-whole-line
 			     (unless (bobp) (backward-char))
 			     (point))))
 	(t
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point)
+         (kill-region (save-excursion (forward-visible-line 0) (point))
 		      (progn (forward-visible-line arg) (point))))))
 
 (defun forward-visible-line (arg)

The reason for the issue probably is: Without the patch, the killing
happens in two stages.  The first kill-region kills from the beginning
of the line until after the A.  That kills the leading *.  That probably
somehow triggers Org visibility changes.  With the patch applied the
whole killing happens in one stage, probably without causing an
intermediate change of visibility.


In GNU Emacs 29.1.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-09-04 built on huette
Repository revision: 5cbe96d17f67e58091de1653f409d87bcc2b3e99
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Arch Linux

Configured using:
 'configure --with-x-toolkit=gtk --with-native-compilation
 --with-tree-sitter --with-json --with-mailutils --with-imagemagick'

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

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-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:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils comp comp-cstr
warnings icons subr-x rx cl-seq cl-macs gv cl-extra help-mode
cl-loaddefs cl-lib bytecomp byte-compile 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 66653 8786)
 (symbols 48 7137 0)
 (strings 32 20076 3360)
 (string-bytes 1 606691)
 (vectors 16 16454)
 (vector-slots 8 296368 15462)
 (floats 8 29 22)
 (intervals 56 240 0)
 (buffers 984 12))

bug#65734: 29.1.50; kill-whole-line and visibility of Org subtrees

[Eli Zaretskii]

> From: Sebastian Miele <iota@whxvd.name>
> Date: Mon, 04 Sep 2023 16:44:19 +0200
> 
> In an emacs -Q, create an Org buffer with the following contents:
> 
> <-----cut-here----->
> * AB
> ** C
> <-----cut-here----->
> 
> Fold the subtree under the heading AB, so that only a single line is
> diplayed (ending in "...").  With point between A and B, hit
> C-S-<backspace> (kill-whole-line).
> 
> Expected: The whole _visible_ line, i.e., the entire contents of the
> buffer is erased.  Actual behavior: The line with heading C remains.
> 
> Contrast this with the same experiment, except that the point is at the
> beginning of the line containing AB when hitting C-S-<backspace>.  Then
> the expected behavior happens.  And according to the source of
> kill-whole-line, the intended effect indeed is to kill a whole _visible_
> line.
> 
> The following patch fixes the issue:
> 
> diff --git a/lisp/simple.el b/lisp/simple.el
> index abd587245fe..44221f3fc24 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -6649,9 +6649,7 @@ kill-whole-line
>  			     (unless (bobp) (backward-char))
>  			     (point))))
>  	(t
> -	 (save-excursion
> -	   (kill-region (point) (progn (forward-visible-line 0) (point))))
> -	 (kill-region (point)
> +         (kill-region (save-excursion (forward-visible-line 0) (point))
>  		      (progn (forward-visible-line arg) (point))))))
>  
>  (defun forward-visible-line (arg)
> 
> The reason for the issue probably is: Without the patch, the killing
> happens in two stages.  The first kill-region kills from the beginning
> of the line until after the A.  That kills the leading *.  That probably
> somehow triggers Org visibility changes.  With the patch applied the
> whole killing happens in one stage, probably without causing an
> intermediate change of visibility.

I'm not sure I understand why this is deemed a problem in Emacs.
Shouldn't Org redefine C-S-<backspace> if the default binding doesn't
suit what happens in Org buffers?  Did you discuss this with Org
developers?

Thanks.

bug#65734: 29.1.50; kill-whole-line and visibility of Org subtrees

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 65734@debbugs.gnu.org, Andrea Corallo <acorallo@gnu.org>, iota@whxvd.name,
>>  monnier@iro.umontreal.ca, stefankangas@gmail.com
>> Date: Sat, 22 Jun 2024 12:00:13 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> > From: Ihor Radchenko <yantar92@posteo.net>
>> > Cc: Sebastian Miele <iota@whxvd.name>, Stefan Monnier
>> >  <monnier@iro.umontreal.ca>, Stefan Kangas <stefankangas@gmail.com>,
>> >  65734@debbugs.gnu.org
>> > Date: Wed, 19 Jun 2024 14:01:12 +0000
>> > 
>> > Eli Zaretskii <eliz@gnu.org> writes:
>> > 
>> > > The patches lack suitable ChangeLog-style commit log messages (see
>> > > CONTRIBUTE for details and you can use "git log" to show examples of
>> > > how we do this).
>> > >
>> > > I'd also ask Stefan Monnier and Stefan Kangas to review the patch,
>> > > since this is an important command and I would like to avoid breaking
>> > > it.
>> > 
>> > Please see the attached edited patches with proper commit messages.
>> 
>> Stefan, Stefan and Andrea, could you please review this?  If you see
>> no problems, let's install this in Emacs 30.
>
> Oops, I see that Stefan Monnier already chimed in.  But I'd like
> Stefan Kangas and Andrea to do so as well.

Agreed on the behavior and proposed implementation.

  Andrea