bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Sebastian Miele <iota@whxvd.name> writes:

> I first reported this to bug-gnu-emacs@gnu.org (see
> https://debbugs.gnu.org/65734).  However, Eli asks:
>
>> 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?

Confirmed.

I am CCing debbugs as I'd like to clarify things to be in sync with
Emacs.


> In an emacs -Q, create an Org buffer with the following contents:
>
> --8<---------------cut here---------------start------------->8---
> * AB
> ** C
> --8<---------------cut here---------------end--------------->8---

This will produce

* AB...

> Fold the subtree under the heading AB, so that only a single line is
> displayed (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.

This indeed happens because `kill-whole-line' deletes the line in two
steps: "* A" and then the rest.

The first deletion leaves

B<begin invisible>
** C<end invisible>

which drastically alters the outline structure and triggers or to
automatically unfold the subtree, leaving

B
** C

visible.
Then, `kill-whole-line' proceeds with the second part of the deletion
and deletes the now visible line, leading to the observed behaviour.

The first deletion would be an equivalent of deleting "(defun"

(defun foo ()...

in outline-mode and would make it hard to unfold the body, if such
single deletion where performed.

In Org mode, because of frequent user requests about accidental
deletions of hidden text, we try our best to protect deletions of
invisible folded outlines. Automatic unfolding is one of the ways to
attract user's attention to potential accidental edit.

> 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.  According to the source of
> kill-whole-line, the intended effect indeed is to kill a whole _visible_
> line.

Currently, Org mode, similar to Eli's suggestion re-binds `kill-line' to
Org's own version - `org-kill-line'. But not `kill-whole-line'.

We can certainly do the same for `kill-whole-line', but in our previous
discussion https://yhetil.org/emacs-devel/87tu8rq2l6.fsf@localhost/, you
asked to consider extending the built-in Emacs commands instead of
overriding them.

As I described in the above, Org needs more control over the behaviour of
`kill-line'/`kill-whole-line' when the visible line contains multiple
lines of hidden text - to protect accidental deletions.
A hook, where Org can intervene with a yes/no prompt, would be useful.
It would also make sense to group the two edits together via
`combine-after-change-calls', although a more universal way to know that
certain edits are a part of the same known command (even when called
non-interactively) would be useful.

In addition, `org-kill-line' acts specially in certain scenarios:

For
* Heading <point> text :tag1:tag2:

`org-kill-line' will keep and re-align ":tag1:tag2:":

* Heading <point>      :tag1:tag2:

It would be nice if we could express such behavior without overriding
the `kill-line' command.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> Cc: 65734@debbugs.gnu.org, emacs-orgmode@gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Tue, 05 Sep 2023 10:29:20 +0000
> 
> As I described in the above, Org needs more control over the behaviour of
> `kill-line'/`kill-whole-line' when the visible line contains multiple
> lines of hidden text - to protect accidental deletions.
> A hook, where Org can intervene with a yes/no prompt, would be useful.
> It would also make sense to group the two edits together via
> `combine-after-change-calls', although a more universal way to know that
> certain edits are a part of the same known command (even when called
> non-interactively) would be useful.

The command kills in two parts for a good reason, which is explained
in the comments to the code.  So making a single group will not work,
I think, at least not in all situations.  And relying on after-change
hooks to fix this use case sounds too obscure and fragile to me.

Moreover, I don't think this is specific to Org: any mode that folds
or hides portions of text might hit the same problem.

So we could decide that this command needs to become smarter when the
visual line includes invisible text.  That is, improve the command
without making any Org-specific changes anywhere.  Patches to that
effect are welcome.

> In addition, `org-kill-line' acts specially in certain scenarios:
> 
> For
> * Heading <point> text :tag1:tag2:
> 
> `org-kill-line' will keep and re-align ":tag1:tag2:":
> 
> * Heading <point>      :tag1:tag2:
> 
> It would be nice if we could express such behavior without overriding
> the `kill-line' command.

This could be handled by a suitable extension to end-of-visible-line.
For example, introduce a new text property which end-of-visible-line
would then handle the same as it currently handles invisible text.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 2023-09-05 14:54 +0300
>
> […]
>
> So we could decide that this command needs to become smarter when the
> visual line includes invisible text.  That is, improve the command
> without making any Org-specific changes anywhere.  Patches to that
> effect are welcome.

The following would do it.  I think I tested it rather thoroughly.
During testing I found another bug that is addressed by the let-binding
of kill-read-only-ok during the first kill-region below.

diff --git a/lisp/simple.el b/lisp/simple.el
index abd587245fe..d983cb85ab3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6631,28 +6631,50 @@ kill-whole-line
   (unless (eq last-command 'kill-region)
     (kill-new "")
     (setq last-command 'kill-region))
-  (cond ((zerop arg)
-	 ;; We need to kill in two steps, because the previous command
-	 ;; could have been a kill command, in which case the text
-	 ;; before point needs to be prepended to the current kill
-	 ;; ring entry and the text after point appended.  Also, we
-	 ;; need to use save-excursion to avoid copying the same text
-	 ;; twice to the kill ring in read-only buffers.
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point) (progn (end-of-visible-line) (point))))
-	((< arg 0)
-	 (save-excursion
-	   (kill-region (point) (progn (end-of-visible-line) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line (1+ arg))
-			     (unless (bobp) (backward-char))
-			     (point))))
-	(t
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line arg) (point))))))
+  ;; - We need to kill in two steps, because the previous command
+  ;;   could have been a kill command, in which case the text before
+  ;;   point needs to be prepended to the current kill ring entry and
+  ;;   the text after point appended.
+  ;; - We need to be careful to avoid copying text twice to the kill
+  ;;   ring in read-only buffers.
+  ;; - We need to determine the boundaries of visible lines before we
+  ;;   do the first kill, because that may change visibility
+  ;;   (bug#65734).
+  (let ((regions-begin (point-marker))
+        region1-end)
+    (cond ((zerop arg)
+           (setq region1-end (save-excursion
+                               (forward-visible-line 0)
+                               (point-marker)))
+           (end-of-visible-line))
+	  ((< arg 0)
+	   (setq region1-end (save-excursion
+                               (end-of-visible-line)
+                               (point-marker)))
+           (forward-visible-line (1+ arg))
+	   (unless (bobp) (backward-char)))
+	  (t
+	   (setq region1-end (save-excursion
+                               (forward-visible-line 0)
+                               (point-marker)))
+	   (progn (forward-visible-line arg))))
+    ;; - Pass the marker positions and not the markers themselves.
+    ;;   kill-region determines whether to prepend or append to a
+    ;;   previous kill by checking the direction of the region.  But
+    ;;   it deletes the content and hence moves the markers before
+    ;;   that.  That effectively makes every region delimited by
+    ;;   markers an (empty) forward region.
+    ;; - Make the first kill-region emit a non-local exit only if the
+    ;;   second kill-region below would not operate on a non-empty
+    ;;   region.
+    (let ((kill-read-only-ok (or kill-read-only-ok
+                                 (/= regions-begin (point)))))
+      (kill-region (marker-position regions-begin)
+                   (marker-position region1-end)))
+    (kill-region (marker-position regions-begin)
+                 (point))
+    (set-marker regions-begin nil)
+    (set-marker region1-end nil)))
 
 (defun forward-visible-line (arg)
   "Move forward by ARG lines, ignoring currently invisible newlines only.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Sebastian Miele <iota@whxvd.name>
> Cc: Ihor Radchenko <yantar92@posteo.net>, 65734@debbugs.gnu.org,
>  emacs-orgmode@gnu.org
> Date: Tue, 05 Sep 2023 17:25:38 +0200
> 
> > From: Eli Zaretskii <eliz@gnu.org>
> > Date: Tue, 2023-09-05 14:54 +0300
> >
> > […]
> >
> > So we could decide that this command needs to become smarter when the
> > visual line includes invisible text.  That is, improve the command
> > without making any Org-specific changes anywhere.  Patches to that
> > effect are welcome.
> 
> The following would do it.  I think I tested it rather thoroughly.
> During testing I found another bug that is addressed by the let-binding
> of kill-read-only-ok during the first kill-region below.

Thanks.  Sadly, we don't have any tests for this function in our test
suite, so verifying this non-trivial change will not be easy...

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> The following would do it.  I think I tested it rather thoroughly.
>> During testing I found another bug that is addressed by the let-binding
>> of kill-read-only-ok during the first kill-region below.
>
> Thanks.  Sadly, we don't have any tests for this function in our test
> suite, so verifying this non-trivial change will not be easy...

Then, what should we do to move things forward? I guess the first step
will be writing these missing tests. Anything else?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Sebastian Miele <iota@whxvd.name>, 65734@debbugs.gnu.org,
>  emacs-orgmode@gnu.org
> Date: Wed, 06 Sep 2023 08:23:23 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> The following would do it.  I think I tested it rather thoroughly.
> >> During testing I found another bug that is addressed by the let-binding
> >> of kill-read-only-ok during the first kill-region below.
> >
> > Thanks.  Sadly, we don't have any tests for this function in our test
> > suite, so verifying this non-trivial change will not be easy...
> 
> Then, what should we do to move things forward? I guess the first step
> will be writing these missing tests.

Yes, that'd be most welcome.

> Anything else?

How about asking on emacs-devel that people who use kill-whole-line
frequently install the patch and run with it for some time?  (We could
do that after installing the changes on master, of course.)

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 2023-09-06 15:16 +0300
>
>> From: Ihor Radchenko <yantar92@posteo.net>
>> Date: Wed, 06 Sep 2023 08:23:23 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> The following would do it.  I think I tested it rather thoroughly.
>> >> During testing I found another bug that is addressed by the let-binding
>> >> of kill-read-only-ok during the first kill-region below.
>> >
>> > Thanks.  Sadly, we don't have any tests for this function in our test
>> > suite, so verifying this non-trivial change will not be easy...
>> 
>> Then, what should we do to move things forward? I guess the first step
>> will be writing these missing tests.
>
> Yes, that'd be most welcome.

I will write the tests.  And I will probably come up with an updated
version of the original patch.  There is at least one cosmetic change.
And something else that I want to have tried.  May take some time.

>> Anything else?
>
> How about asking on emacs-devel that people who use kill-whole-line
> frequently install the patch and run with it for some time?  (We could
> do that after installing the changes on master, of course.)

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Sebastian Miele <iota@whxvd.name> writes:

>>> Then, what should we do to move things forward? I guess the first step
>>> will be writing these missing tests.
>>
>> Yes, that'd be most welcome.
>
> I will write the tests.  And I will probably come up with an updated
> version of the original patch.  There is at least one cosmetic change.
> And something else that I want to have tried.  May take some time.

Thanks in advance!
Feel free to ask us anything if you need any help.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

I removed emacs-orgmode@gnu.org from CC.

> From: Sebastian Miele <iota@whxvd.name>
> Date: Wed, 2023-09-06 15:30 +0200
>
> I will write the tests.  And I will probably come up with an updated
> version of the original patch.  There is at least one cosmetic change.
> And something else that I want to have tried.  May take some time.

Please have a look at the following patch.  For now it contains three
tests, two of them with :expected-result :failed.  (They do not fail on
the bug-fixed version of `kill-whole-line'.)

There probably will be more tests and further questions.  But for now, I
would like to basically have a statement of whether the style of writing
the tests goes in an acceptable direction.

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index 28d8120f143..c15b0059536 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -22,6 +22,7 @@
 ;;; Code:
 
 (require 'ert)
+(require 'ert-x)
 (eval-when-compile (require 'cl-lib))
 
 (defun simple-test--buffer-substrings ()
@@ -40,6 +41,112 @@ simple-test--dummy-buffer
      ,@body
      (with-no-warnings (simple-test--buffer-substrings))))
 
+(defconst simple-tests-point-tag "<POINT>")
+(defconst simple-tests-mark-tag "<MARK>")
+
+(defun simple-tests--set-buffer-text-point-mark (description)
+  "Set the current buffers text, point and mark according to DESCRIPTION.
+
+Erase current buffer and insert DESCRIPTION.  Set point to the
+first occurrence of `simple-tests-point-tag' (\"<POINT>\") in the
+buffer, removing it.  If there is no `simple-tests-point-tag',
+set point to the beginning of the buffer.  Similar for the active
+mark (`simple-tests-mark-tag', \"<MARK>\")."
+  (erase-buffer)
+  (insert description)
+  (goto-char (point-min))
+  (when (search-forward simple-tests-mark-tag nil t)
+    (delete-char (- (length simple-tests-mark-tag)))
+    (push-mark (point) nil 'activate))
+  (goto-char (point-min))
+  (when (search-forward simple-tests-point-tag nil t)
+    (delete-char (- (length simple-tests-point-tag)))))
+
+(defun simple-tests--get-buffer-text-point-mark ()
+  "Inverse of `simple-tests--set-buffer-text-point-mark'."
+  (if (not mark-active)
+      (concat (buffer-substring-no-properties (point-min) (point))
+              simple-tests-point-tag
+              (buffer-substring-no-properties (point) (point-max)))
+    (if (< (mark) (point))
+        (concat (buffer-substring-no-properties (point-min) (mark))
+                simple-tests-mark-tag
+                (buffer-substring-no-properties (mark) (point))
+                simple-tests-point-tag
+                (buffer-substring-no-properties (point) (point-max)))
+      (concat (buffer-substring-no-properties (point-min) (point))
+              simple-tests-point-tag
+              (buffer-substring-no-properties (point) (mark))
+              simple-tests-mark-tag
+              (buffer-substring-no-properties (mark) (point-max))))))
+
+(ert-deftest simple-tests--buffer-text-point-mark-helpers ()
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "")
+    (should (equal "" (buffer-substring-no-properties
+                       (point-min) (point-max))))
+    (should-not mark-active)
+    (should (equal 1 (point)))
+    (should (equal "<POINT>" (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "<POINT><MARK>")
+    (should (equal "" (buffer-substring-no-properties
+                       (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 1 (point)))
+    (should (equal 1 (mark)))
+    (should (equal "<POINT><MARK>" (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "<MARK><POINT>")
+    (should (equal "" (buffer-substring-no-properties
+                       (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 1 (point)))
+    (should (equal 1 (mark)))
+    (should (equal "<POINT><MARK>" (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "A<POINT><MARK>B")
+    (should (equal "AB" (buffer-substring-no-properties
+                         (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 2 (point)))
+    (should (equal 2 (mark)))
+    (should (equal "A<POINT><MARK>B"
+                   (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "A<MARK><POINT>B")
+    (should (equal "AB" (buffer-substring-no-properties
+                         (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 2 (point)))
+    (should (equal 2 (mark)))
+    (should (equal "A<POINT><MARK>B"
+                   (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "A<POINT>X<MARK>B")
+    (should (equal "AXB" (buffer-substring-no-properties
+                          (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 2 (point)))
+    (should (equal 3 (mark)))
+    (should (equal "A<POINT>X<MARK>B"
+                   (simple-tests--get-buffer-text-point-mark))))
+
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "A<MARK>X<POINT>B")
+    (should (equal "AXB" (buffer-substring-no-properties
+                          (point-min) (point-max))))
+    (should mark-active)
+    (should (equal 3 (point)))
+    (should (equal 2 (mark)))
+    (should (equal "A<MARK>X<POINT>B"
+                   (simple-tests--get-buffer-text-point-mark)))))
+
 \f
 ;;; `count-words'
 (ert-deftest simple-test-count-words-bug-41761 ()
@@ -1046,5 +1153,109 @@ simple-tests-zap-to-char
     (with-zap-to-char-test "abcdeCXYZ" "XYZ"
       (zap-to-char 1 ?C 'interactive))))
 
+\f
+;;; Tests for `kill-whole-line'
+
+(ert-deftest kill-whole-line-invisible ()
+  :expected-result :failed
+  (cl-macrolet ((test (kill-whole-line-arg &rest expected-lines)
+                  `(ert-with-test-buffer-selected nil
+                     (simple-tests--set-buffer-text-point-mark
+                      (string-join
+                       '("* -2" "hidden"
+                         "* -1" "hidden"
+                         "* A<POINT>B" "hidden"
+                         "* 1" "hidden"
+                         "* 2" "hidden"
+                         "")
+                       "\n"))
+                     (ert-simulate-command '(org-mode))
+                     (ert-simulate-command '(org-fold-hide-sublevels 1))
+                     (ert-simulate-command
+                      '(kill-whole-line ,kill-whole-line-arg))
+                     (should
+                      (equal (string-join ',expected-lines "\n")
+                             (simple-tests--get-buffer-text-point-mark))))))
+    (test 0
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 1
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 2
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 2" "hidden"
+          "")
+    (test 3
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test 9
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test -1
+          "* -2" "hidden"
+          "* -1" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -2
+          "* -2" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -3
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -9
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")))
+
+(ert-deftest kill-whole-line-read-only ()
+  :expected-result :failed
+  (cl-macrolet
+      ((test (kill-whole-line-arg expected-kill-lines expected-buffer-lines)
+         `(ert-with-test-buffer-selected nil
+            (simple-tests--set-buffer-text-point-mark
+             (string-join '("-2" "-1" "A<POINT>B" "1" "2" "") "\n"))
+            (ert-simulate-command '(read-only-mode 1))
+            (should-error (ert-simulate-command
+                           '(kill-whole-line ,kill-whole-line-arg)))
+            (should (equal (string-join ,expected-kill-lines "\n")
+                           (car kill-ring)))
+            (should (equal (string-join ,expected-buffer-lines "\n")
+                           (simple-tests--get-buffer-text-point-mark))))))
+    (test 0 '("AB") '("-2" "-1" "AB<POINT>" "1" "2" ""))
+    (test 1 '("AB" "") '("-2" "-1" "AB" "<POINT>1" "2" ""))
+    (test 2 '("AB" "1" "") '("-2" "-1" "AB" "1" "<POINT>2" ""))
+    (test 3 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test 9 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test -1 '("" "AB") '("-2" "-1<POINT>" "AB" "1" "2" ""))
+    (test -2 '("" "-1" "AB") '("-2<POINT>" "-1" "AB" "1" "2" ""))
+    (test -3 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))
+    (test -9 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))))
+
+(ert-deftest kill-whole-line-after-other-kill ()
+  (ert-with-test-buffer-selected nil
+    (simple-tests--set-buffer-text-point-mark "A<POINT>X<MARK>B")
+    (ert-simulate-command '(kill-region (mark) (point) 'region))
+    (ert-simulate-command '(kill-whole-line))
+    (should (equal "AXB" (car kill-ring)))
+    (should (equal "<POINT>"
+                   (simple-tests--get-buffer-text-point-mark)))))
+
 (provide 'simple-test)
 ;;; simple-tests.el ends here

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Sebastian Miele <iota@whxvd.name>
> Cc: Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>
> Date: Sun, 10 Sep 2023 18:31:20 +0200
> 
> I removed emacs-orgmode@gnu.org from CC.
> 
> > From: Sebastian Miele <iota@whxvd.name>
> > Date: Wed, 2023-09-06 15:30 +0200
> >
> > I will write the tests.  And I will probably come up with an updated
> > version of the original patch.  There is at least one cosmetic change.
> > And something else that I want to have tried.  May take some time.
> 
> Please have a look at the following patch.  For now it contains three
> tests, two of them with :expected-result :failed.  (They do not fail on
> the bug-fixed version of `kill-whole-line'.)

Yes, there should be more tests, ideally: there are situations where
kill-whole-line signals an error, and I don't think I see tests where
some of the text is invisible (as the function uses
forward-visible-line and end-of-visual-line).

> There probably will be more tests and further questions.  But for now, I
> would like to basically have a statement of whether the style of writing
> the tests goes in an acceptable direction.

Looks reasonable, but I'm not sure I understand what will the test
show if one of the tests fails: will the information shown then tell
enough to understand which of the sub-tests failed and why?

Thanks.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Sun, 2023-09-10 19:57 +0300
>
>> From: Sebastian Miele <iota@whxvd.name>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>
>> Date: Sun, 10 Sep 2023 18:31:20 +0200
>> 
>> I removed emacs-orgmode@gnu.org from CC.
>> 
>> > From: Sebastian Miele <iota@whxvd.name>
>> > Date: Wed, 2023-09-06 15:30 +0200
>> >
>> > I will write the tests.  And I will probably come up with an updated
>> > version of the original patch.  There is at least one cosmetic change.
>> > And something else that I want to have tried.  May take some time.
>> 
>> Please have a look at the following patch.  For now it contains three
>> tests, two of them with :expected-result :failed.  (They do not fail on
>> the bug-fixed version of `kill-whole-line'.)
>
> Yes, there should be more tests, ideally: there are situations where
> kill-whole-line signals an error,

Those tests are on the radar.

> and I don't think I see tests where some of the text is invisible (as
> the function uses forward-visible-line and end-of-visual-line).

That already is covered via (org-mode) and (org-fold-hide-sublevels 1)
in test `kill-whole-line-invisible'.

>> There probably will be more tests and further questions.  But for now, I
>> would like to basically have a statement of whether the style of writing
>> the tests goes in an acceptable direction.
>
> Looks reasonable, but I'm not sure I understand what will the test
> show if one of the tests fails: will the information shown then tell
> enough to understand which of the sub-tests failed and why?

That almost certainly would not be immediately obvious in the current
state.  I have next to no experience in working with testing frameworks.
But I assumed that regressions do not happen that often, and that it
would be good enough if the code of the test can be grasped quickly.
Then, in case of a regression, the test code can be temporarily
sprinkled with some printf-like debugging to find out the exact location
in the test.

However, enough of that printf-like debugging could also be hard-coded,
like in the following definition (see the line ending in the comment
"Provide some context"):

  (ert-deftest kill-whole-line-read-only ()
    ;;:expected-result :failed
    (cl-flet
        ((subtest (kill-whole-line-arg expected-kill-lines expected-buffer-lines)
           (should `(subtest ,kill-whole-line-arg)) ; Provide some context
           (ert-with-test-buffer-selected nil
             (simple-tests--set-buffer-text-point-mark
              (string-join '("-2" "-1" "A<POINT>B" "1" "2" "") "\n"))
             (ert-simulate-command '(read-only-mode 1))
             (should-error (ert-simulate-command
                            `(kill-whole-line ,kill-whole-line-arg))
                           :type 'buffer-read-only)
             (should (equal (string-join expected-kill-lines "\n")
                            (car kill-ring)))
             (should (equal (string-join expected-buffer-lines "\n")
                            (simple-tests--get-buffer-text-point-mark))))))
      (subtest 0 '("AB") '("-2" "-1" "AB<POINT>" "1" "2" ""))
      (subtest 1 '("AB" "") '("-2" "-1" "AB" "<POINT>1" "2" ""))
      (subtest 2 '("AB" "1" "") '("-2" "-1" "AB" "1" "<POINT>2" ""))
      (subtest 3 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
      (subtest 9 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
      (subtest -1 '("" "AB") '("-2" "-1<POINT>" "AB" "1" "2" ""))
      (subtest -2 '("" "-1" "AB") '("-2<POINT>" "-1" "AB" "1" "2" ""))
      (subtest -3 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))
      (subtest -9 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))))

With the always succeeding

  (should `(subtest ,kill-whole-line-arg)) ; Provide some context

at the beginning of ervery subtest, the context would be clear after
pressing l in a buffer showing the ERT test results (but never on the
console).

An alternative would be to use `message'.  That would also provide the
context on the console.  However, that also may be a bit noisy.

Another possibility would be to define every subtest as a top-level
test, by a macro like:

  (defmacro simple-test--define-kill-whole-line-read-only-test (kill-whole-line-arg)
    ...)

But that feels a bit over the top and unflexible.

What to me feels like an ideal solution would be the concept of a
current context during an ERT test.  Just something like a (defvar
ert-current-context) that always initially is dynamically let-bound to
nil during a test.  That could be setq'ed at different locations in a
test to different arbitrary values (somewhat like ERT explanations).
When a should fails, and the ert-current-context is non-nil, ERT would
display the current context as the first information on the failed test.

WDYT?

For now I assume that providing context via

  (should `(subtest ,kill-whole-line-arg)) ; Provide some context

is good enough.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Sebastian Miele <iota@whxvd.name>
> Cc: 65734@debbugs.gnu.org, yantar92@posteo.net
> Date: Tue, 12 Sep 2023 15:04:41 +0200
> 
> > Looks reasonable, but I'm not sure I understand what will the test
> > show if one of the tests fails: will the information shown then tell
> > enough to understand which of the sub-tests failed and why?
> 
> That almost certainly would not be immediately obvious in the current
> state.  I have next to no experience in working with testing frameworks.
> But I assumed that regressions do not happen that often, and that it
> would be good enough if the code of the test can be grasped quickly.
> Then, in case of a regression, the test code can be temporarily
> sprinkled with some printf-like debugging to find out the exact location
> in the test.
> 
> However, enough of that printf-like debugging could also be hard-coded,
> like in the following definition (see the line ending in the comment
> "Provide some context"):

It should be enough to have an indication of what sub-test failed.
Finding that out in a series of tests one of which fails is frequently
a very frustrating experience, especially if the test is written with
heavy use of macros, or generates testing code on the fly (or both).

> With the always succeeding
> 
>   (should `(subtest ,kill-whole-line-arg)) ; Provide some context
> 
> at the beginning of ervery subtest, the context would be clear after
> pressing l in a buffer showing the ERT test results (but never on the
> console).

I mostly run tests in batch mode, so my preference is to see some
telltale indication of the failed code when ERT reports the failure.

> An alternative would be to use `message'.  That would also provide the
> context on the console.  However, that also may be a bit noisy.

The "noise" is required only when there's a failure.

One idea is to include the test ID in the string(s) you test for
equality, such that the diagnostics printed by ERT will inherently
include the test ID, and will allow finding the problematic code
easily.  For example, instead of

   "AB<POINT>"

you could use "TEST#1<POINT>" or something along those lines.

Thanks.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

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

Attached are two patches.  The first patch introduces the tests,
including two tests that are expected to be failing for the current
`kill-whole-line'.

The test `kill-whole-line-read-only' does not fail because of the bug
reported in this bug report, but because of another bug that I stumbled
upon while investigating and testing: `kill-whole-line' always kills by
two calls to `kill-region'.  When the buffer is readonly, the first of
the two calls to `kill-region' errors out / exits non-locally.  That
causes `kill-region' to omit to put the remaining stuff (from the second
`kill-region') into the kill ring.

The second patch fixes both bugs, and removes the corresponding
`:expected-result :failed' from the tests.

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Sun, 2023-09-10 19:57 +0300
>
>> From: Sebastian Miele <iota@whxvd.name>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>
>> Date: Sun, 10 Sep 2023 18:31:20 +0200
>>
>> I removed emacs-orgmode@gnu.org from CC.
>>
>> > From: Sebastian Miele <iota@whxvd.name>
>> > Date: Wed, 2023-09-06 15:30 +0200
>> >
>> > I will write the tests.  And I will probably come up with an updated
>> > version of the original patch.  There is at least one cosmetic change.
>> > And something else that I want to have tried.  May take some time.
>>
>> Please have a look at the following patch.  For now it contains three
>> tests, two of them with :expected-result :failed.  (They do not fail on
>> the bug-fixed version of `kill-whole-line'.)
>
> Yes, there should be more tests, ideally: there are situations where
> kill-whole-line signals an error, and I don't think I see tests where
> some of the text is invisible (as the function uses
> forward-visible-line and end-of-visual-line).

I added tests for cases when `kill-whole-line' signals errors.

The tests for the invisible stuff were already there, in the test
`kill-whole-line-invisible'.  The calls that introduce invisibility and
`after-change-functions' in the test are:

  (org-mode)
  (org-fold-hide-sublevels 1)

Even though its only five tests, I think they are rather very thorough,
now.  Except one test, all tests tests have several subtests.  All
branches of `kill-whole-line' are covered.

>> There probably will be more tests and further questions.  But for
>> now, I would like to basically have a statement of whether the style
>> of writing the tests goes in an acceptable direction.
>
> Looks reasonable, but I'm not sure I understand what will the test
> show if one of the tests fails: will the information shown then tell
> enough to understand which of the sub-tests failed and why?

I found the `ert-info' macro which allows to add arbitrary annotiations
to `should's in its body, used it for all subtests.  They now are
clearly distinguishable in the output of `ert', even when run from a
terminal.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tests.patch --]
[-- Type: text/x-patch, Size: 9840 bytes --]

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index 28d8120f143..e6d3ffe8d34 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -22,6 +22,7 @@
 ;;; Code:
 
 (require 'ert)
+(require 'ert-x)
 (eval-when-compile (require 'cl-lib))
 
 (defun simple-test--buffer-substrings ()
@@ -40,6 +41,49 @@ simple-test--dummy-buffer
      ,@body
      (with-no-warnings (simple-test--buffer-substrings))))
 
+(defconst simple-test-point-tag "<POINT>")
+(defconst simple-test-mark-tag "<MARK>")
+
+(defun simple-test--set-buffer-text-point-mark (description)
+  "Set the current buffer's text, point and mark according to
+DESCRIPTION.
+
+Erase current buffer and insert DESCRIPTION.  Set point to the
+first occurrence of `simple-test-point-tag' (\"<POINT>\") in the
+buffer, removing it.  If there is no `simple-test-point-tag', set
+point to the beginning of the buffer.  If there is a
+`simple-test-mark-tag' (\"<MARK>\"), remove it, and set an active
+mark there."
+  (erase-buffer)
+  (insert description)
+  (goto-char (point-min))
+  (when (search-forward simple-test-mark-tag nil t)
+    (delete-char (- (length simple-test-mark-tag)))
+    (push-mark (point) nil 'activate))
+  (goto-char (point-min))
+  (when (search-forward simple-test-point-tag nil t)
+    (delete-char (- (length simple-test-point-tag)))))
+
+(defun simple-test--get-buffer-text-point-mark ()
+  "Inverse of `simple-test--set-buffer-text-point-mark'."
+  (cond
+   ((not mark-active)
+    (concat (buffer-substring-no-properties (point-min) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (point-max))))
+   ((< (mark) (point))
+    (concat (buffer-substring-no-properties (point-min) (mark))
+            simple-test-mark-tag
+            (buffer-substring-no-properties (mark) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (point-max))))
+   (t
+    (concat (buffer-substring-no-properties (point-min) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (mark))
+            simple-test-mark-tag
+            (buffer-substring-no-properties (mark) (point-max))))))
+
 \f
 ;;; `count-words'
 (ert-deftest simple-test-count-words-bug-41761 ()
@@ -1046,5 +1090,190 @@ simple-tests-zap-to-char
     (with-zap-to-char-test "abcdeCXYZ" "XYZ"
       (zap-to-char 1 ?C 'interactive))))
 
+\f
+;;; Tests for `kill-whole-line'
+
+(ert-deftest kill-whole-line-invisible ()
+  :expected-result :failed
+  (cl-flet ((test (kill-whole-line-arg &rest expected-lines)
+              (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")
+                (ert-with-test-buffer-selected nil
+                  (simple-test--set-buffer-text-point-mark
+                   (string-join
+                    '("* -2" "hidden"
+                      "* -1" "hidden"
+                      "* A<POINT>B" "hidden"
+                      "* 1" "hidden"
+                      "* 2" "hidden"
+                      "")
+                    "\n"))
+                  (org-mode)
+                  (org-fold-hide-sublevels 1)
+                  (kill-whole-line kill-whole-line-arg)
+                  (should
+                   (equal (string-join expected-lines "\n")
+                          (simple-test--get-buffer-text-point-mark)))))))
+    (test 0
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 1
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 2
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 2" "hidden"
+          "")
+    (test 3
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test 9
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test -1
+          "* -2" "hidden"
+          "* -1" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -2
+          "* -2" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -3
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -9
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")))
+
+(ert-deftest kill-whole-line-read-only ()
+  :expected-result :failed
+  (cl-flet
+      ((test (kill-whole-line-arg expected-kill-lines expected-buffer-lines)
+         (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")
+           (ert-with-test-buffer-selected nil
+             (simple-test--set-buffer-text-point-mark
+              (string-join '("-2" "-1" "A<POINT>B" "1" "2" "") "\n"))
+             (read-only-mode 1)
+             (setq last-command #'ignore)
+             (should-error (kill-whole-line kill-whole-line-arg)
+                           :type 'buffer-read-only)
+             (should (equal (string-join expected-kill-lines "\n")
+                            (car kill-ring)))
+             (should (equal (string-join expected-buffer-lines "\n")
+                            (simple-test--get-buffer-text-point-mark)))))))
+    (test 0 '("AB") '("-2" "-1" "AB<POINT>" "1" "2" ""))
+    (test 1 '("AB" "") '("-2" "-1" "AB" "<POINT>1" "2" ""))
+    (test 2 '("AB" "1" "") '("-2" "-1" "AB" "1" "<POINT>2" ""))
+    (test 3 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test 9 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test -1 '("" "AB") '("-2" "-1<POINT>" "AB" "1" "2" ""))
+    (test -2 '("" "-1" "AB") '("-2<POINT>" "-1" "AB" "1" "2" ""))
+    (test -3 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))
+    (test -9 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))))
+
+(ert-deftest kill-whole-line-after-other-kill ()
+  (ert-with-test-buffer-selected nil
+    (simple-test--set-buffer-text-point-mark "A<POINT>X<MARK>B")
+    (setq last-command #'ignore)
+    (kill-region (point) (mark))
+    (deactivate-mark 'force)
+    (setq last-command #'kill-region)
+    (kill-whole-line)
+    (should (equal "AXB" (car kill-ring)))
+    (should (equal "<POINT>"
+                   (simple-test--get-buffer-text-point-mark)))))
+
+(ert-deftest kill-whole-line-buffer-boundaries ()
+  (ert-with-test-buffer-selected nil
+    (ert-info ("0" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>")
+      (should-error (kill-whole-line -1)
+                    :type 'beginning-of-buffer)
+      (should-error (kill-whole-line 1)
+                    :type 'end-of-buffer))
+    (ert-info ("1a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>")
+      (should-error (kill-whole-line 1)
+                    :type 'end-of-buffer))
+    (ert-info ("1b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A" (car kill-ring))))
+    (ert-info ("2" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>\n1")
+      (should-error (kill-whole-line -1)
+                    :type 'beginning-of-buffer))
+    (ert-info ("2b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>A\n1")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "<POINT>1"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))))
+
+(ert-deftest kill-whole-line-line-boundaries ()
+  (ert-with-test-buffer-selected nil
+    (ert-info ("1a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\n" (car kill-ring))))
+    (ert-info ("1b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\n" (car kill-ring))))
+    (ert-info ("2a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))
+    (ert-info ("2b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\nA" (car kill-ring))))
+    (ert-info ("3a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>A\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))
+    (ert-info ("3b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>A\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\nA" (car kill-ring))))))
+
 (provide 'simple-test)
 ;;; simple-tests.el ends here

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: fix.patch --]
[-- Type: text/x-patch, Size: 4662 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 6453dfbcd2b..1fd087538b7 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6640,28 +6640,53 @@ kill-whole-line
   (unless (eq last-command 'kill-region)
     (kill-new "")
     (setq last-command 'kill-region))
-  (cond ((zerop arg)
-	 ;; We need to kill in two steps, because the previous command
-	 ;; could have been a kill command, in which case the text
-	 ;; before point needs to be prepended to the current kill
-	 ;; ring entry and the text after point appended.  Also, we
-	 ;; need to use save-excursion to avoid copying the same text
-	 ;; twice to the kill ring in read-only buffers.
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point) (progn (end-of-visible-line) (point))))
-	((< arg 0)
-	 (save-excursion
-	   (kill-region (point) (progn (end-of-visible-line) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line (1+ arg))
-			     (unless (bobp) (backward-char))
-			     (point))))
-	(t
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line arg) (point))))))
+  ;; - We need to kill in two steps, because the previous command
+  ;;   could have been a kill command, in which case the text before
+  ;;   point needs to be prepended to the current kill ring entry and
+  ;;   the text after point appended.
+  ;; - We need to be careful to avoid copying text twice to the kill
+  ;;   ring in read-only buffers.
+  ;; - We need to determine the boundaries of visible lines before we
+  ;;   do the first kill.  Otherwise `after-change-functions' may
+  ;;   change visibility (bug#65734).
+  (let (;; The beginning of both regions to kill
+        (regions-begin (point-marker))
+        ;; The end of the first region to kill.  Moreover, after
+        ;; evaluation of the value form, (point) will be the end of
+        ;; the second region to kill.
+        (region1-end (cond ((zerop arg)
+                            (prog1 (save-excursion
+                                     (forward-visible-line 0)
+                                     (point-marker))
+                              (end-of-visible-line)))
+	                   ((< arg 0)
+	                    (prog1 (save-excursion
+                                     (end-of-visible-line)
+                                     (point-marker))
+                              (forward-visible-line (1+ arg))
+	                      (unless (bobp) (backward-char))))
+	                   (t
+	                    (prog1 (save-excursion
+                                     (forward-visible-line 0)
+                                     (point-marker))
+	                      (forward-visible-line arg))))))
+    ;; - Pass the marker positions and not the markers themselves.
+    ;;   kill-region determines whether to prepend or append to a
+    ;;   previous kill by checking the direction of the region.  But
+    ;;   it deletes the content and hence moves the markers before
+    ;;   that.  That effectively makes every region delimited by
+    ;;   markers an (empty) forward region.
+    ;; - Make the first kill-region emit a non-local exit only if the
+    ;;   second kill-region below would not operate on a non-empty
+    ;;   region.
+    (let ((kill-read-only-ok (or kill-read-only-ok
+                                 (/= regions-begin (point)))))
+      (kill-region (marker-position regions-begin)
+                   (marker-position region1-end)))
+    (kill-region (marker-position regions-begin)
+                 (point))
+    (set-marker regions-begin nil)
+    (set-marker region1-end nil)))
 
 (defun forward-visible-line (arg)
   "Move forward by ARG lines, ignoring currently invisible newlines only.
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index e6d3ffe8d34..15f2db7e610 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -1094,7 +1094,6 @@ simple-tests-zap-to-char
 ;;; Tests for `kill-whole-line'
 
 (ert-deftest kill-whole-line-invisible ()
-  :expected-result :failed
   (cl-flet ((test (kill-whole-line-arg &rest expected-lines)
               (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")
                 (ert-with-test-buffer-selected nil
@@ -1162,7 +1161,6 @@ kill-whole-line-invisible
           "")))
 
 (ert-deftest kill-whole-line-read-only ()
-  :expected-result :failed
   (cl-flet
       ((test (kill-whole-line-arg expected-kill-lines expected-buffer-lines)
          (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Sebastian Miele <iota@whxvd.name>
> Cc: 65734@debbugs.gnu.org, yantar92@posteo.net
> Date: Mon, 25 Dec 2023 19:53:36 +0100
> 
> Attached are two patches.  The first patch introduces the tests,
> including two tests that are expected to be failing for the current
> `kill-whole-line'.
> 
> The test `kill-whole-line-read-only' does not fail because of the bug
> reported in this bug report, but because of another bug that I stumbled
> upon while investigating and testing: `kill-whole-line' always kills by
> two calls to `kill-region'.  When the buffer is readonly, the first of
> the two calls to `kill-region' errors out / exits non-locally.  That
> causes `kill-region' to omit to put the remaining stuff (from the second
> `kill-region') into the kill ring.
> 
> The second patch fixes both bugs, and removes the corresponding
> `:expected-result :failed' from the tests.

Thanks.

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.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

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

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-kill-whole-line-Honor-visibility-fix-kill-ring-when-.patch --]
[-- Type: text/x-patch, Size: 4701 bytes --]

From 340e1a6a9d394c89c23ef34cdb37fa9124b4bd84 Mon Sep 17 00:00:00 2001
Message-ID: <340e1a6a9d394c89c23ef34cdb37fa9124b4bd84.1718805590.git.yantar92@posteo.net>
From: Sebastian Miele <iota@whxvd.name>
Date: Wed, 19 Jun 2024 15:48:59 +0200
Subject: [PATCH 1/2] kill-whole-line: Honor visibility; fix kill-ring when
 read-only (bug#65734)

* lisp/simple.el (kill-whole-line): Use visibility state before
performing any edits as reference instead of expecting that visibility
cannot change.  First of the two calls to `kill-region' may trigger
`after-change-functions' that might alter the visibility state.
Make sure that we populate the `kill-ring' with the contents of the
whole line when buffer is in `read-only-mode'.
---
 lisp/simple.el | 69 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index b48f46fc711..76dffcdd327 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6703,28 +6703,53 @@ kill-whole-line
   (unless (eq last-command 'kill-region)
     (kill-new "")
     (setq last-command 'kill-region))
-  (cond ((zerop arg)
-	 ;; We need to kill in two steps, because the previous command
-	 ;; could have been a kill command, in which case the text
-	 ;; before point needs to be prepended to the current kill
-	 ;; ring entry and the text after point appended.  Also, we
-	 ;; need to use save-excursion to avoid copying the same text
-	 ;; twice to the kill ring in read-only buffers.
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point) (progn (end-of-visible-line) (point))))
-	((< arg 0)
-	 (save-excursion
-	   (kill-region (point) (progn (end-of-visible-line) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line (1+ arg))
-			     (unless (bobp) (backward-char))
-			     (point))))
-	(t
-	 (save-excursion
-	   (kill-region (point) (progn (forward-visible-line 0) (point))))
-	 (kill-region (point)
-		      (progn (forward-visible-line arg) (point))))))
+  ;; - We need to kill in two steps, because the previous command
+  ;;   could have been a kill command, in which case the text before
+  ;;   point needs to be prepended to the current kill ring entry and
+  ;;   the text after point appended.
+  ;; - We need to be careful to avoid copying text twice to the kill
+  ;;   ring in read-only buffers.
+  ;; - We need to determine the boundaries of visible lines before we
+  ;;   do the first kill.  Otherwise `after-change-functions' may
+  ;;   change visibility (bug#65734).
+  (let (;; The beginning of both regions to kill
+        (regions-begin (point-marker))
+        ;; The end of the first region to kill.  Moreover, after
+        ;; evaluation of the value form, (point) will be the end of
+        ;; the second region to kill.
+        (region1-end (cond ((zerop arg)
+                            (prog1 (save-excursion
+                                     (forward-visible-line 0)
+                                     (point-marker))
+                              (end-of-visible-line)))
+	                   ((< arg 0)
+	                    (prog1 (save-excursion
+                                     (end-of-visible-line)
+                                     (point-marker))
+                              (forward-visible-line (1+ arg))
+	                      (unless (bobp) (backward-char))))
+	                   (t
+	                    (prog1 (save-excursion
+                                     (forward-visible-line 0)
+                                     (point-marker))
+	                      (forward-visible-line arg))))))
+    ;; - Pass the marker positions and not the markers themselves.
+    ;;   kill-region determines whether to prepend or append to a
+    ;;   previous kill by checking the direction of the region.  But
+    ;;   it deletes the content and hence moves the markers before
+    ;;   that.  That effectively makes every region delimited by
+    ;;   markers an (empty) forward region.
+    ;; - Make the first kill-region emit a non-local exit only if the
+    ;;   second kill-region below would not operate on a non-empty
+    ;;   region.
+    (let ((kill-read-only-ok (or kill-read-only-ok
+                                 (/= regions-begin (point)))))
+      (kill-region (marker-position regions-begin)
+                   (marker-position region1-end)))
+    (kill-region (marker-position regions-begin)
+                 (point))
+    (set-marker regions-begin nil)
+    (set-marker region1-end nil)))
 
 (defun forward-visible-line (arg)
   "Move forward by ARG lines, ignoring currently invisible newlines only.
-- 
2.45.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-tests-for-kill-whole-line-bug-65734.patch --]
[-- Type: text/x-patch, Size: 10796 bytes --]

From 40a14348da6680b2213133aa3909e47223459e14 Mon Sep 17 00:00:00 2001
Message-ID: <40a14348da6680b2213133aa3909e47223459e14.1718805590.git.yantar92@posteo.net>
In-Reply-To: <340e1a6a9d394c89c23ef34cdb37fa9124b4bd84.1718805590.git.yantar92@posteo.net>
References: <340e1a6a9d394c89c23ef34cdb37fa9124b4bd84.1718805590.git.yantar92@posteo.net>
From: Sebastian Miele <iota@whxvd.name>
Date: Wed, 19 Jun 2024 15:58:24 +0200
Subject: [PATCH 2/2] Add tests for `kill-whole-line' (bug#65734)

* test/lisp/simple-tests.el (simple-test-point-tag):
(simple-test-mark-tag):
(simple-test--set-buffer-text-point-mark):
(simple-test--get-buffer-text-point-mark): Add helper functions used by
the tests.
(kill-whole-line-invisible):
(kill-whole-line-read-only):
(kill-whole-line-after-other-kill):
(kill-whole-line-buffer-boundaries):
(kill-whole-line-line-boundaries): Add tests for `kill-whole-line'.
---
 test/lisp/simple-tests.el | 227 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 227 insertions(+)

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index afd75786804..9e3e71bd69b 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -22,6 +22,7 @@
 ;;; Code:
 
 (require 'ert)
+(require 'ert-x)
 (eval-when-compile (require 'cl-lib))
 
 (defun simple-test--buffer-substrings ()
@@ -40,6 +41,49 @@ simple-test--dummy-buffer
      ,@body
      (with-no-warnings (simple-test--buffer-substrings))))
 
+(defconst simple-test-point-tag "<POINT>")
+(defconst simple-test-mark-tag "<MARK>")
+
+(defun simple-test--set-buffer-text-point-mark (description)
+  "Set the current buffer's text, point and mark according to
+DESCRIPTION.
+
+Erase current buffer and insert DESCRIPTION.  Set point to the
+first occurrence of `simple-test-point-tag' (\"<POINT>\") in the
+buffer, removing it.  If there is no `simple-test-point-tag', set
+point to the beginning of the buffer.  If there is a
+`simple-test-mark-tag' (\"<MARK>\"), remove it, and set an active
+mark there."
+  (erase-buffer)
+  (insert description)
+  (goto-char (point-min))
+  (when (search-forward simple-test-mark-tag nil t)
+    (delete-char (- (length simple-test-mark-tag)))
+    (push-mark (point) nil 'activate))
+  (goto-char (point-min))
+  (when (search-forward simple-test-point-tag nil t)
+    (delete-char (- (length simple-test-point-tag)))))
+
+(defun simple-test--get-buffer-text-point-mark ()
+  "Inverse of `simple-test--set-buffer-text-point-mark'."
+  (cond
+   ((not mark-active)
+    (concat (buffer-substring-no-properties (point-min) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (point-max))))
+   ((< (mark) (point))
+    (concat (buffer-substring-no-properties (point-min) (mark))
+            simple-test-mark-tag
+            (buffer-substring-no-properties (mark) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (point-max))))
+   (t
+    (concat (buffer-substring-no-properties (point-min) (point))
+            simple-test-point-tag
+            (buffer-substring-no-properties (point) (mark))
+            simple-test-mark-tag
+            (buffer-substring-no-properties (mark) (point-max))))))
+
 \f
 ;;; `count-words'
 (ert-deftest simple-test-count-words-bug-41761 ()
@@ -1046,5 +1090,188 @@ simple-tests-zap-to-char
     (with-zap-to-char-test "abcdeCXYZ" "XYZ"
       (zap-to-char 1 ?C 'interactive))))
 
+\f
+;;; Tests for `kill-whole-line'
+
+(ert-deftest kill-whole-line-invisible ()
+  (cl-flet ((test (kill-whole-line-arg &rest expected-lines)
+              (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")
+                (ert-with-test-buffer-selected nil
+                  (simple-test--set-buffer-text-point-mark
+                   (string-join
+                    '("* -2" "hidden"
+                      "* -1" "hidden"
+                      "* A<POINT>B" "hidden"
+                      "* 1" "hidden"
+                      "* 2" "hidden"
+                      "")
+                    "\n"))
+                  (org-mode)
+                  (org-fold-hide-sublevels 1)
+                  (kill-whole-line kill-whole-line-arg)
+                  (should
+                   (equal (string-join expected-lines "\n")
+                          (simple-test--get-buffer-text-point-mark)))))))
+    (test 0
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 1
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test 2
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>* 2" "hidden"
+          "")
+    (test 3
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test 9
+          "* -2" "hidden"
+          "* -1" "hidden"
+          "<POINT>")
+    (test -1
+          "* -2" "hidden"
+          "* -1" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -2
+          "* -2" "hidden<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -3
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")
+    (test -9
+          "<POINT>"
+          "* 1" "hidden"
+          "* 2" "hidden"
+          "")))
+
+(ert-deftest kill-whole-line-read-only ()
+  (cl-flet
+      ((test (kill-whole-line-arg expected-kill-lines expected-buffer-lines)
+         (ert-info ((format "%s" kill-whole-line-arg) :prefix "Subtest: ")
+           (ert-with-test-buffer-selected nil
+             (simple-test--set-buffer-text-point-mark
+              (string-join '("-2" "-1" "A<POINT>B" "1" "2" "") "\n"))
+             (read-only-mode 1)
+             (setq last-command #'ignore)
+             (should-error (kill-whole-line kill-whole-line-arg)
+                           :type 'buffer-read-only)
+             (should (equal (string-join expected-kill-lines "\n")
+                            (car kill-ring)))
+             (should (equal (string-join expected-buffer-lines "\n")
+                            (simple-test--get-buffer-text-point-mark)))))))
+    (test 0 '("AB") '("-2" "-1" "AB<POINT>" "1" "2" ""))
+    (test 1 '("AB" "") '("-2" "-1" "AB" "<POINT>1" "2" ""))
+    (test 2 '("AB" "1" "") '("-2" "-1" "AB" "1" "<POINT>2" ""))
+    (test 3 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test 9 '("AB" "1" "2" "") '("-2" "-1" "AB" "1" "2" "<POINT>"))
+    (test -1 '("" "AB") '("-2" "-1<POINT>" "AB" "1" "2" ""))
+    (test -2 '("" "-1" "AB") '("-2<POINT>" "-1" "AB" "1" "2" ""))
+    (test -3 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))
+    (test -9 '("-2" "-1" "AB") '("<POINT>-2" "-1" "AB" "1" "2" ""))))
+
+(ert-deftest kill-whole-line-after-other-kill ()
+  (ert-with-test-buffer-selected nil
+    (simple-test--set-buffer-text-point-mark "A<POINT>X<MARK>B")
+    (setq last-command #'ignore)
+    (kill-region (point) (mark))
+    (deactivate-mark 'force)
+    (setq last-command #'kill-region)
+    (kill-whole-line)
+    (should (equal "AXB" (car kill-ring)))
+    (should (equal "<POINT>"
+                   (simple-test--get-buffer-text-point-mark)))))
+
+(ert-deftest kill-whole-line-buffer-boundaries ()
+  (ert-with-test-buffer-selected nil
+    (ert-info ("0" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>")
+      (should-error (kill-whole-line -1)
+                    :type 'beginning-of-buffer)
+      (should-error (kill-whole-line 1)
+                    :type 'end-of-buffer))
+    (ert-info ("1a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>")
+      (should-error (kill-whole-line 1)
+                    :type 'end-of-buffer))
+    (ert-info ("1b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A" (car kill-ring))))
+    (ert-info ("2" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>\n1")
+      (should-error (kill-whole-line -1)
+                    :type 'beginning-of-buffer))
+    (ert-info ("2b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "<POINT>A\n1")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "<POINT>1"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))))
+
+(ert-deftest kill-whole-line-line-boundaries ()
+  (ert-with-test-buffer-selected nil
+    (ert-info ("1a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\n" (car kill-ring))))
+    (ert-info ("1b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\n" (car kill-ring))))
+    (ert-info ("2a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))
+    (ert-info ("2b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\nA<POINT>\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\nA" (car kill-ring))))
+    (ert-info ("3a" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>A\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line 1)
+      (should (equal "-1\n<POINT>1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "A\n" (car kill-ring))))
+    (ert-info ("3b" :prefix "Subtest: ")
+      (simple-test--set-buffer-text-point-mark "-1\n<POINT>A\n1\n")
+      (setq last-command #'ignore)
+      (kill-whole-line -1)
+      (should (equal "-1<POINT>\n1\n"
+                     (simple-test--get-buffer-text-point-mark)))
+      (should (equal "\nA" (car kill-ring))))))
+
 (provide 'simple-test)
 ;;; simple-tests.el ends here
-- 
2.45.1


[-- Attachment #4: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

> +  ;; - We need to kill in two steps, because the previous command
> +  ;;   could have been a kill command, in which case the text before
> +  ;;   point needs to be prepended to the current kill ring entry and
> +  ;;   the text after point appended.
> +  ;; - We need to be careful to avoid copying text twice to the kill
> +  ;;   ring in read-only buffers.
> +  ;; - We need to determine the boundaries of visible lines before we
> +  ;;   do the first kill.  Otherwise `after-change-functions' may
> +  ;;   change visibility (bug#65734).
> +  (let (;; The beginning of both regions to kill
> +        (regions-begin (point-marker))
> +        ;; The end of the first region to kill.  Moreover, after
> +        ;; evaluation of the value form, (point) will be the end of
> +        ;; the second region to kill.
> +        (region1-end (cond ((zerop arg)
> +                            (prog1 (save-excursion
> +                                     (forward-visible-line 0)
> +                                     (point-marker))
> +                              (end-of-visible-line)))
> +	                   ((< arg 0)
> +	                    (prog1 (save-excursion
> +                                     (end-of-visible-line)
> +                                     (point-marker))
> +                              (forward-visible-line (1+ arg))
> +	                      (unless (bobp) (backward-char))))
> +	                   (t
> +	                    (prog1 (save-excursion
> +                                     (forward-visible-line 0)
> +                                     (point-marker))
> +	                      (forward-visible-line arg))))))
> +    ;; - Pass the marker positions and not the markers themselves.
> +    ;;   kill-region determines whether to prepend or append to a
> +    ;;   previous kill by checking the direction of the region.  But
> +    ;;   it deletes the content and hence moves the markers before
> +    ;;   that.  That effectively makes every region delimited by
> +    ;;   markers an (empty) forward region.
> +    ;; - Make the first kill-region emit a non-local exit only if the
> +    ;;   second kill-region below would not operate on a non-empty
> +    ;;   region.
> +    (let ((kill-read-only-ok (or kill-read-only-ok
> +                                 (/= regions-begin (point)))))
> +      (kill-region (marker-position regions-begin)
> +                   (marker-position region1-end)))
> +    (kill-region (marker-position regions-begin)
> +                 (point))
> +    (set-marker regions-begin nil)
> +    (set-marker region1-end nil)))

Thanks, that seems sensible.
[ Tho at the same time, it's sad to think such a "trivial" operation
  requires such delicate treatment.  ]


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

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

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

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

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

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

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

The patch and proposed behavior makes sense to me.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 22 Jun 2024 12:00:26 -0700
> Cc: iota@whxvd.name, monnier@iro.umontreal.ca, 65734@debbugs.gnu.org, 
> 	Andrea Corallo <acorallo@gnu.org>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> 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.
> 
> The patch and proposed behavior makes sense to me.

Thanks, I've now installed this on the emacs-30 branch, and I'm
closing the bug.

Btw, the new tests broke simple-tests (because they caused packages be
loaded that affected the completion tested by one of the tests.  I
fixed that, but please always run the relevant test files whenever you
modify them, don't assume that adding or modifying a test will never
affect other tests in the same file.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Sebastian Miele <iota@whxvd.name> writes:

>> I will write the tests.  And I will probably come up with an updated
>> version of the original patch.  There is at least one cosmetic change.
>> And something else that I want to have tried.  May take some time.
> ...

A gentle bump.
If you encountered any difficulties, feel free to ask us anything. We
will try to help.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

Hi Ihor,

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Mon, 2023-12-04 12:42 +0000
>
> Sebastian Miele <iota@whxvd.name> writes:
>
>>> I will write the tests.  And I will probably come up with an updated
>>> version of the original patch.  There is at least one cosmetic change.
>>> And something else that I want to have tried.  May take some time.
>> ...
>
> A gentle bump.
> If you encountered any difficulties, feel free to ask us anything. We
> will try to help.

I did not forget.  It is mostly finished since quite a while.  I will
come back to it and finish/polish it probably before the end of the
month.

Best wishes
Sebastian

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> It would also make sense to group the two edits together via
>> `combine-after-change-calls', although a more universal way to know that
>> certain edits are a part of the same known command (even when called
>> non-interactively) would be useful.
>
> The command kills in two parts for a good reason, which is explained
> in the comments to the code.  So making a single group will not work,
> I think, at least not in all situations.

I think there is misunderstanding. `combine-after-change-calls' will not
affect the two-step modification of the kill ring, if we put it around
`kill-whole-line'. Or do I miss something?

> ...  And relying on after-change
> hooks to fix this use case sounds too obscure and fragile to me.

Indeed. I did not talk about this particular bug report. What I meant is
some way to group change hooks executed by the same function/command.

>> In addition, `org-kill-line' acts specially in certain scenarios:
>> 
>> For
>> * Heading <point> text :tag1:tag2:
>> 
>> `org-kill-line' will keep and re-align ":tag1:tag2:":
>> 
>> * Heading <point>      :tag1:tag2:
>> 
>> It would be nice if we could express such behavior without overriding
>> the `kill-line' command.
>
> This could be handled by a suitable extension to end-of-visible-line.
> For example, introduce a new text property which end-of-visible-line
> would then handle the same as it currently handles invisible text.

I am not sure if I like the idea of text property - marking all the tags
in buffer with text property is expensive. What about something like
`end-of-visible-line-function'?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: iota@whxvd.name, 65734@debbugs.gnu.org, emacs-orgmode@gnu.org
> Date: Wed, 06 Sep 2023 08:30:36 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> In addition, `org-kill-line' acts specially in certain scenarios:
> >> 
> >> For
> >> * Heading <point> text :tag1:tag2:
> >> 
> >> `org-kill-line' will keep and re-align ":tag1:tag2:":
> >> 
> >> * Heading <point>      :tag1:tag2:
> >> 
> >> It would be nice if we could express such behavior without overriding
> >> the `kill-line' command.
> >
> > This could be handled by a suitable extension to end-of-visible-line.
> > For example, introduce a new text property which end-of-visible-line
> > would then handle the same as it currently handles invisible text.
> 
> I am not sure if I like the idea of text property - marking all the tags
> in buffer with text property is expensive.

Then perhaps just a special value for buffer-invisibility-spec, or
some other simple variation of a property Org already uses?

> What about something like `end-of-visible-line-function'?

That is also a possibility, but it will then affect kill-line
_anywhere_ in the buffer, whereas a text property can have a more
localized effect.  Are you sure kill-line will need this customization
on the whole buffer?

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> In addition, `org-kill-line' acts specially in certain scenarios:
>> >> 
>> >> For
>> >> * Heading <point> text :tag1:tag2:
>> >> 
>> >> `org-kill-line' will keep and re-align ":tag1:tag2:":
>> >> 
>> >> * Heading <point>      :tag1:tag2:
>> >> 
>> >> It would be nice if we could express such behavior without overriding
>> >> the `kill-line' command.
>> ...
>> I am not sure if I like the idea of text property - marking all the tags
>> in buffer with text property is expensive.
>
> Then perhaps just a special value for buffer-invisibility-spec, or
> some other simple variation of a property Org already uses?

We may have a misunderstanding here.
In "* Heading text :tag1:tag2:", everything is visible yet Org needs to
protect ":tag1:tag2: from being killed by `kill-line', but not from
`kill-whole-line'. Moreover, the behaviour also depends on the point
position - if point is inside ":tag1:tag2:", we fall back to the default
behaviour. And the whole "special" behaviour can also be switched off by
flipping `org-special-ctrl-k'.

Invisibility has nothing to do with this need.

>> What about something like `end-of-visible-line-function'?
>
> That is also a possibility, but it will then affect kill-line
> _anywhere_ in the buffer, whereas a text property can have a more
> localized effect.  Are you sure kill-line will need this customization
> on the whole buffer?

Applying text property is not free - (1) we need to do it across the
whole buffer, which scales poorly; (2) we need to keep it updated as the
buffer changes, which scales even worse. In addition, adding more text
properties slow down redisplay, which Org already strains to its limits.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: iota@whxvd.name, 65734@debbugs.gnu.org, emacs-orgmode@gnu.org
> Date: Thu, 07 Sep 2023 10:00:49 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Then perhaps just a special value for buffer-invisibility-spec, or
> > some other simple variation of a property Org already uses?
> 
> We may have a misunderstanding here.
> In "* Heading text :tag1:tag2:", everything is visible yet Org needs to
> protect ":tag1:tag2: from being killed by `kill-line', but not from
> `kill-whole-line'. Moreover, the behaviour also depends on the point
> position - if point is inside ":tag1:tag2:", we fall back to the default
> behaviour. And the whole "special" behaviour can also be switched off by
> flipping `org-special-ctrl-k'.
> 
> Invisibility has nothing to do with this need.

Isn't it true that invisibility is what causes the user expectations
in this case to begin with?  Then saying that "invisibility has
nothing to do with this" is not really accurate, is it?

> >> What about something like `end-of-visible-line-function'?
> >
> > That is also a possibility, but it will then affect kill-line
> > _anywhere_ in the buffer, whereas a text property can have a more
> > localized effect.  Are you sure kill-line will need this customization
> > on the whole buffer?
> 
> Applying text property is not free - (1) we need to do it across the
> whole buffer, which scales poorly; (2) we need to keep it updated as the
> buffer changes, which scales even worse. In addition, adding more text
> properties slow down redisplay, which Org already strains to its limits.

I understand, but in my book correctness tramps performance, and I was
trying to make the point that perhaps modifying the behavior of
kill-line everywhere in Org buffers might be incorrect for some cases.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 2023-09-07 13:19 +0300
>
>> From: Ihor Radchenko <yantar92@posteo.net>
>> Cc: iota@whxvd.name, 65734@debbugs.gnu.org, emacs-orgmode@gnu.org
>> Date: Thu, 07 Sep 2023 10:00:49 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Then perhaps just a special value for buffer-invisibility-spec, or
>> > some other simple variation of a property Org already uses?
>> 
>> We may have a misunderstanding here.
>> In "* Heading text :tag1:tag2:", everything is visible yet Org needs to
>> protect ":tag1:tag2: from being killed by `kill-line', but not from
>> `kill-whole-line'. Moreover, the behaviour also depends on the point
>> position - if point is inside ":tag1:tag2:", we fall back to the default
>> behaviour. And the whole "special" behaviour can also be switched off by
>> flipping `org-special-ctrl-k'.
>> 
>> Invisibility has nothing to do with this need.
>
> Isn't it true that invisibility is what causes the user expectations
> in this case to begin with?  Then saying that "invisibility has
> nothing to do with this" is not really accurate, is it?

I am not 100 % sure what exactly the misunderstanding is.  My first
guess would be: You assume that the tags are invisible, too.  But that
is not the case.  Because of that the special handling of tags by
`org-kill-line' has nothing to do with visibility.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> We may have a misunderstanding here.
>> In "* Heading text :tag1:tag2:", everything is visible yet Org needs to
>> protect ":tag1:tag2: from being killed by `kill-line', but not from
>> `kill-whole-line'. Moreover, the behaviour also depends on the point
>> position - if point is inside ":tag1:tag2:", we fall back to the default
>> behaviour. And the whole "special" behaviour can also be switched off by
>> flipping `org-special-ctrl-k'.
>> 
>> Invisibility has nothing to do with this need.
>
> Isn't it true that invisibility is what causes the user expectations
> in this case to begin with?  Then saying that "invisibility has
> nothing to do with this" is not really accurate, is it?

There are two things I raised in the previous messages:
1. The specific bug reported, related to invisibility changes in
   after-change-functions
2. A request to extend `kill-whole-line' and `kill-line' to cater Org
   needs:
   - for invisible text in folded headings
   - for visible text, when `kill-line' is called on a heading

In this branch of the discussion, I am describing a request to deal with
visible text.

Hope I made things more clear now.

>> >> What about something like `end-of-visible-line-function'?
>> >
>> > That is also a possibility, but it will then affect kill-line
>> > _anywhere_ in the buffer, whereas a text property can have a more
>> > localized effect.  Are you sure kill-line will need this customization
>> > on the whole buffer?
>> 
>> Applying text property is not free - (1) we need to do it across the
>> whole buffer, which scales poorly; (2) we need to keep it updated as the
>> buffer changes, which scales even worse. In addition, adding more text
>> properties slow down redisplay, which Org already strains to its limits.
>
> I understand, but in my book correctness tramps performance, and I was
> trying to make the point that perhaps modifying the behavior of
> kill-line everywhere in Org buffers might be incorrect for some cases.

I think that `end-of-visible-line-function' might be more appropriate -
it does not reduce correctness (we can simply fall back to the default
behaviour when needed) and less problematic performance-wise, as I
described.

Or maybe some other idea. But not text properties - they are
problematic performance-wise. In addition, `org-kill-line' is sensitive
to point position (do different things at bol, before ":tag:...", and
inside it). I am not sure how to make things depend on point position
with text properties only.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Sebastian Miele]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Wed, 2023-09-06 08:30 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> It would also make sense to group the two edits together via
>>> `combine-after-change-calls', although a more universal way to know that
>>> certain edits are a part of the same known command (even when called
>>> non-interactively) would be useful.
>>
>> The command kills in two parts for a good reason, which is explained
>> in the comments to the code.  So making a single group will not work,
>> I think, at least not in all situations.
>
> I think there is misunderstanding. `combine-after-change-calls' will not
> affect the two-step modification of the kill ring, if we put it around
> `kill-whole-line'. Or do I miss something?

I tried to wrap the problematic portion of `kill-whole-line' into
`combine-after-change-calls'.  It seems to have no effect.  The
after-change function `org-fold-core--fix-folded-region' still gets
called twice, not fixing the bug.  I did not dig deeper, because the
stuff that makes `combine-after-change-calls' work at least partially
goes in C and seems to be scattered over several places.

The Emacs Lisp manual states that `combine-after-change-calls' "arranges
to call the after-change functions just once for a series of several
changes—if that seems safe."  So this case does not seem safe.  Apart
from that, there is no stated guarantee for when it would seem it safe.

I conclude that, although this path looked possibly elegant at first,
and I wanted to give it a try, this cannot work out.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Sebastian Miele <iota@whxvd.name> writes:

>> I think there is misunderstanding. `combine-after-change-calls' will not
>> affect the two-step modification of the kill ring, if we put it around
>> `kill-whole-line'. Or do I miss something?
>
> I tried to wrap the problematic portion of `kill-whole-line' into
> `combine-after-change-calls'.  It seems to have no effect.  The
> after-change function `org-fold-core--fix-folded-region' still gets
> called twice, not fixing the bug.  I did not dig deeper, because the
> stuff that makes `combine-after-change-calls' work at least partially
> goes in C and seems to be scattered over several places.

Oops. Of course, I meant `combine-change-calls'.
`combine-after-change-calls' does not always have effect. In particular,
Org sets `before-change-functions' effectively disabling the macro you tried.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Max Nikulin]

On 05/09/2023 17:29, Ihor Radchenko wrote:
> Confirmed.

It is a regression in comparison to e.g. org-mode-9.3.1.

I noticed it for multiline plain list items

- ab
   cd

Only first line is removed by C-S-<backspace> when the item is folded by 
TAB.

For headings I usually use C-c C-x C-w, so I had less chance to face 
this issue.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> Cc: emacs-orgmode@gnu.org, 65734@debbugs.gnu.org
> Date: Tue, 5 Sep 2023 21:30:58 +0700
> From: Max Nikulin <manikulin@gmail.com>
> 
> On 05/09/2023 17:29, Ihor Radchenko wrote:
> > Confirmed.
> 
> It is a regression in comparison to e.g. org-mode-9.3.1.

What changed since org-mode-9.3.1?  Was it some change in Emacs, and
if so, which one?

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> On 05/09/2023 17:29, Ihor Radchenko wrote:
>> > Confirmed.
>> 
>> It is a regression in comparison to e.g. org-mode-9.3.1.
>
> What changed since org-mode-9.3.1?  Was it some change in Emacs, and
> if so, which one?

The reported bug is a side effect of a feature when Org automatically
reveals hidden outlines that are "broken" due to edits and thus could
not be unfolded easily. For example, when destroying parent heading in a
folding subtree:

<point>* Heading...

   |
   V

 Heading
<unfolded>

The feature was introduced in Or 9.5.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Max Nikulin]

On 05/09/2023 22:50, Ihor Radchenko wrote:
> Eli Zaretskii writes:
> 
>>> On 05/09/2023 17:29, Ihor Radchenko wrote:
>>>> Confirmed.
>>>
>>> It is a regression in comparison to e.g. org-mode-9.3.1.
>>
>> What changed since org-mode-9.3.1?  Was it some change in Emacs, and
>> if so, which one?

That was a comparison on emacs-26.3

> The reported bug is a side effect of a feature when Org automatically
> reveals hidden outlines that are "broken" due to edits and thus could
> not be unfolded easily. For example, when destroying parent heading in a
> folding subtree:
> 
> <point>* Heading...
> 
>     |
>     V
> 
>   Heading
> <unfolded>
> 
> The feature was introduced in Or 9.5.

Or in 9.6? On Emacs-28.2 I have compared Org-9.5.5 and 9.6.8. Unfolding 
on removing of a heading marker does not work in 9.5.5, but collapsed 
list items and headers are removed correctly.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> The feature was introduced in Or 9.5.
>
> Or in 9.6?

Oops. Yup. Org 9.6 - the latest release. Together with org-fold library.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Max Nikulin <manikulin@gmail.com>, iota@whxvd.name,
>  65734@debbugs.gnu.org, emacs-orgmode@gnu.org
> Date: Tue, 05 Sep 2023 15:50:58 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> On 05/09/2023 17:29, Ihor Radchenko wrote:
> >> > Confirmed.
> >> 
> >> It is a regression in comparison to e.g. org-mode-9.3.1.
> >
> > What changed since org-mode-9.3.1?  Was it some change in Emacs, and
> > if so, which one?
> 
> The reported bug is a side effect of a feature when Org automatically
> reveals hidden outlines that are "broken" due to edits and thus could
> not be unfolded easily. For example, when destroying parent heading in a
> folding subtree:
> 
> <point>* Heading...
> 
>    |
>    V
> 
>  Heading
> <unfolded>
> 
> The feature was introduced in Or 9.5.

Understood, thanks.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

> The reported bug is a side effect of a feature when Org automatically
> reveals hidden outlines that are "broken" due to edits and thus could
> not be unfolded easily. For example, when destroying parent heading in a
> folding subtree:

I'd be in favor of changing `kill-whole-line` to do the kill in a single
step rather than killing the "before" and "after" separately.
I understand why it doesn't do that, but I'm not sure it's worth the
trouble (or we should change the `kill-region` thingy to be more
robust, e.g. record the position of the last kill so that it doesn't
need to rely on (< end beg) to guess whether to append or prepend and
it can automatically notice when the new kill is *around* the previous
one).

But in addition to that, I suspect that Org should probably not modify
visibility directly from the modification hooks.  Instead, its
modification hook function should just stash the info somewhere and then
update the visibility later on, such as in a `post-command-hook`, timer,
`pre-redisplay-functions`, younameit.

As a rule of thumb, I think modification hooks should be treated a bit
like POSIX signal handlers: just record the event somewhere but don't do
any substantial work in there.


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But in addition to that, I suspect that Org should probably not modify
> visibility directly from the modification hooks.  Instead, its
> modification hook function should just stash the info somewhere and then
> update the visibility later on, such as in a `post-command-hook`, timer,
> `pre-redisplay-functions`, younameit.

Good idea. At least, for this specific feature in Org mode.

> As a rule of thumb, I think modification hooks should be treated a bit
> like POSIX signal handlers: just record the event somewhere but don't do
> any substantial work in there.

Yet, it is sometimes necessary to modify text right inside the
modification hooks. Otherwise, it is very hard (and sometimes
impossible) to keep track of the original text region when multiple
modifications happen there one by one.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

>> As a rule of thumb, I think modification hooks should be treated a bit
>> like POSIX signal handlers: just record the event somewhere but don't do
>> any substantial work in there.
> Yet, it is sometimes necessary to modify text right inside the
> modification hooks. Otherwise, it is very hard (and sometimes
> impossible) to keep track of the original text region when multiple
> modifications happen there one by one.

It's always "possible", but yes there can be undesirable effects:
usually as you delay the updates you're forced to reduce the granularity
of the recorded changes, so in you may end up treating a series of small
changes as one big-ass change for which your updates may not be able to
do as good a job.

E.g. in `diff-mode` we try to allow editing the patch, and dynamically
update the hunk headers accordingly.  This works well for an edit within
a single hunk but doing it for larger edits is somewhere between hard
and undesirable so we don't do it.

If a command changes just two lines but in two different hunks, our code
treats it a a single change that spans two hunks and thus refrains from
updating the hunk headers, as if the command had actually deleted and
then reinserted all the text between those two lines.

That's why I said "rule of thumb": there can be tradeoffs.
In practice 99% of Emacs commands modify only a single contiguous chunk
of text, so the tradeoff comes into play fairly rarely.


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> That's why I said "rule of thumb": there can be tradeoffs.
> In practice 99% of Emacs commands modify only a single contiguous chunk
> of text, so the tradeoff comes into play fairly rarely.

It would be nice if this advice were added to the relevant docstrings.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

>> That's why I said "rule of thumb": there can be tradeoffs.
>> In practice 99% of Emacs commands modify only a single contiguous chunk
>> of text, so the tradeoff comes into play fairly rarely.
> It would be nice if this advice were added to the relevant docstrings.

Not sure exactly which advice you're referring to (the text you quoted
above is misleading: it would encourage the naive reader to just do the
work in `after-change-functions` since it's called only once per command
anyway, which they already instinctively do and which is exactly what we
don't want), nor which docstring you're referring to.


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> It would be nice if this advice were added to the relevant docstrings.
>
> Not sure exactly which advice you're referring to (the text you quoted
> above is misleading: it would encourage the naive reader to just do the
> work in `after-change-functions` since it's called only once per command
> anyway, which they already instinctively do and which is exactly what we
> don't want), nor which docstring you're referring to.

I was mostly referring to the docstring of `after-change-functions'.
Maybe also
`modification-hooks'\‘insert-in-front-hooks’\‘insert-behind-hooks’ text
properties.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> But in addition to that, I suspect that Org should probably not modify
>> visibility directly from the modification hooks.  Instead, its
>> modification hook function should just stash the info somewhere and then
>> update the visibility later on, such as in a `post-command-hook`, timer,
>> `pre-redisplay-functions`, younameit.
>
> Good idea. At least, for this specific feature in Org mode.

Now, done.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f9702a09e

Although, I am still interested to pursue feature request to allow
customizing `kill-whole-line' and `kill-line' by major modes.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

>>> But in addition to that, I suspect that Org should probably not modify
>>> visibility directly from the modification hooks.  Instead, its
>>> modification hook function should just stash the info somewhere and then
>>> update the visibility later on, such as in a `post-command-hook`, timer,
>>> `pre-redisplay-functions`, younameit.
>> Good idea. At least, for this specific feature in Org mode.
> Now, done.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f9702a09e

Great, thanks!

> Although, I am still interested to pursue feature request to allow
> customizing `kill-whole-line' and `kill-line' by major modes.

[ I'm sorry I didn't pay very much attention to this part.
  Naively, I'd suggest you use `remap` bindings for that, but I'm sure
  you're aware of that option, so you presumably have good reasons to
  want something else.  ]


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Although, I am still interested to pursue feature request to allow
>> customizing `kill-whole-line' and `kill-line' by major modes.
>
> [ I'm sorry I didn't pay very much attention to this part.
>   Naively, I'd suggest you use `remap` bindings for that, but I'm sure
>   you're aware of that option, so you presumably have good reasons to
>   want something else.  ]

Eli told me in the past that Org mode should not remap keys.
See the discussion branch starting at
https://yhetil.org/emacs-devel/8735gfs3is.fsf@localhost/

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

> Eli told me in the past that Org mode should not remap keys.
> See the discussion branch starting at
> https://yhetil.org/emacs-devel/8735gfs3is.fsf@localhost/

Then I'll let Eli take care of this :-)


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: emacs-orgmode@gnu.org, Eli Zaretskii <eliz@gnu.org>,
>  65734@debbugs.gnu.org, Max Nikulin <manikulin@gmail.com>, iota@whxvd.name
> Date: Tue, 09 Jan 2024 22:33:58 +0000
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> Although, I am still interested to pursue feature request to allow
> >> customizing `kill-whole-line' and `kill-line' by major modes.
> >
> > [ I'm sorry I didn't pay very much attention to this part.
> >   Naively, I'd suggest you use `remap` bindings for that, but I'm sure
> >   you're aware of that option, so you presumably have good reasons to
> >   want something else.  ]
> 
> Eli told me in the past that Org mode should not remap keys.
> See the discussion branch starting at
> https://yhetil.org/emacs-devel/8735gfs3is.fsf@localhost/

I said that remapping widely-used keys to commands that behave
significantly differently places a non-trivial burden on users,
especially on those who use the remapping mode relatively rarely.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Eli told me in the past that Org mode should not remap keys.
>> See the discussion branch starting at
>> https://yhetil.org/emacs-devel/8735gfs3is.fsf@localhost/
>
> I said that remapping widely-used keys to commands that behave
> significantly differently places a non-trivial burden on users,
> especially on those who use the remapping mode relatively rarely.

Sure. From which I concluded that Org mode should avoid remapping and
instead prefer other means to adjust the behaviour of standard Emacs
commands.

Then, we discussed that Emacs commands to not always provide enough
toggles. So, I am asking to add one in this thread.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Wed, 10 Jan 2024 13:05:19 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I said that remapping widely-used keys to commands that behave
> > significantly differently places a non-trivial burden on users,
> > especially on those who use the remapping mode relatively rarely.
> 
> Sure. From which I concluded that Org mode should avoid remapping and
> instead prefer other means to adjust the behaviour of standard Emacs
> commands.
> 
> Then, we discussed that Emacs commands to not always provide enough
> toggles. So, I am asking to add one in this thread.

I don't think I understand what kind of toggle are we talking about.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

>> I said that remapping widely-used keys to commands that behave
>> significantly differently places a non-trivial burden on users,
>> especially on those who use the remapping mode relatively rarely.
>
> Sure. From which I concluded that Org mode should avoid remapping

I don't think that's what it means.  It means that it depends on what is
the end-user-visible effect.  If the remapped version of the command
"does the same" conceptually, then it's OK (even more so if the
non-remapped version of the command ends up misbehaving).

I think the main question is: can we expect that some users out there
will want to use the non-remapped version of the command because they
prefer its behavior?

So, when the alternative is to directly modify the behavior of the
non-remapped command, remapping is always acceptable.
[ But sometimes, remapping just doesn't do what we want, because the
  command is also used as a function from other places.  ]


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-orgmode@gnu.org,
>   65734@debbugs.gnu.org,  manikulin@gmail.com,  iota@whxvd.name
> Date: Wed, 10 Jan 2024 11:26:08 -0500
> 
> >> I said that remapping widely-used keys to commands that behave
> >> significantly differently places a non-trivial burden on users,
> >> especially on those who use the remapping mode relatively rarely.
> >
> > Sure. From which I concluded that Org mode should avoid remapping
> 
> I don't think that's what it means.  It means that it depends on what is
> the end-user-visible effect.  If the remapped version of the command
> "does the same" conceptually, then it's OK (even more so if the
> non-remapped version of the command ends up misbehaving).

Yes.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I said that remapping widely-used keys to commands that behave
>>> significantly differently places a non-trivial burden on users,
>>> especially on those who use the remapping mode relatively rarely.
>>
>> Sure. From which I concluded that Org mode should avoid remapping
>
> I don't think that's what it means.  It means that it depends on what is
> the end-user-visible effect.  If the remapped version of the command
> "does the same" conceptually, then it's OK (even more so if the
> non-remapped version of the command ends up misbehaving).

Ok. I stand corrected.

> I think the main question is: can we expect that some users out there
> will want to use the non-remapped version of the command because they
> prefer its behavior?

We received no bug reports complaining about non-standard behaviour of
remapped functions AFAIK. So, I do not think that Org mode has problems
here.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Then, we discussed that Emacs commands to not always provide enough
>> toggles. So, I am asking to add one in this thread.
>
> I don't think I understand what kind of toggle are we talking about.

What I would like to request is a way to handle the following situation:

* Heading<... few hundreds of lines of invisible text>

If the user calls `kill-whole-line', a large part of the buffer will get
deleted. We had complains from the users about accidentally deleting a
lot of text in Org files in such situations.

So, I'd like some way to configure `kill-whole-line'/`kill-line' to warn
user about killing hidden text when we detect that we are deleting a
folded heading. Something like:

   (y-or-n-p "Kill hidden subtree along with headline? ")

I believe that it might be useful in other situations as well. Like in
outline-mode or outline-minor-mode.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Thu, 11 Jan 2024 15:50:10 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Then, we discussed that Emacs commands to not always provide enough
> >> toggles. So, I am asking to add one in this thread.
> >
> > I don't think I understand what kind of toggle are we talking about.
> 
> What I would like to request is a way to handle the following situation:
> 
> * Heading<... few hundreds of lines of invisible text>
> 
> If the user calls `kill-whole-line', a large part of the buffer will get
> deleted. We had complains from the users about accidentally deleting a
> lot of text in Org files in such situations.
> 
> So, I'd like some way to configure `kill-whole-line'/`kill-line' to warn
> user about killing hidden text when we detect that we are deleting a
> folded heading. Something like:
> 
>    (y-or-n-p "Kill hidden subtree along with headline? ")
> 
> I believe that it might be useful in other situations as well. Like in
> outline-mode or outline-minor-mode.

How would kill-line know that it's about to kill a subtree?  All it
knows is that it is killing some invisible text.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> So, I'd like some way to configure `kill-whole-line'/`kill-line' to warn
>> user about killing hidden text when we detect that we are deleting a
>> folded heading. Something like:
>> 
>>    (y-or-n-p "Kill hidden subtree along with headline? ")
>> 
>> I believe that it might be useful in other situations as well. Like in
>> outline-mode or outline-minor-mode.
>
> How would kill-line know that it's about to kill a subtree?  All it
> knows is that it is killing some invisible text.

I imagine the following:

1. `kill-*-line' function will, by default, test if invisible text of
   length size is killed and query the user when called interactively.

2. Major modes could also set buffer-local `kill-line-query-function'
   that will return nil when killing should proceed without query or a
   string with query text.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Thu, 11 Jan 2024 16:15:13 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So, I'd like some way to configure `kill-whole-line'/`kill-line' to warn
> >> user about killing hidden text when we detect that we are deleting a
> >> folded heading. Something like:
> >> 
> >>    (y-or-n-p "Kill hidden subtree along with headline? ")
> >> 
> >> I believe that it might be useful in other situations as well. Like in
> >> outline-mode or outline-minor-mode.
> >
> > How would kill-line know that it's about to kill a subtree?  All it
> > knows is that it is killing some invisible text.
> 
> I imagine the following:
> 
> 1. `kill-*-line' function will, by default, test if invisible text of
>    length size is killed and query the user when called interactively.
> 
> 2. Major modes could also set buffer-local `kill-line-query-function'
>    that will return nil when killing should proceed without query or a
>    string with query text.

If the command is only sensitive to invisible text, it could warn
about so-and-so many invisible characters being killed, but it could
not warn about "subtrees", which is what you wanted.  Invisible text
in a buffer could have nothing to do with subtrees, even if the buffer
is under org-mode.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> > How would kill-line know that it's about to kill a subtree?  All it
>> > knows is that it is killing some invisible text.
>> 
>> I imagine the following:
>> 
>> 1. `kill-*-line' function will, by default, test if invisible text of
>>    length size is killed and query the user when called interactively.
>> 
>> 2. Major modes could also set buffer-local `kill-line-query-function'
>>    that will return nil when killing should proceed without query or a
>>    string with query text.
>
> If the command is only sensitive to invisible text, it could warn
> about so-and-so many invisible characters being killed, but it could
> not warn about "subtrees", which is what you wanted.  Invisible text
> in a buffer could have nothing to do with subtrees, even if the buffer
> is under org-mode.

Let me elaborate. In Elisp, I am thinking about something like:

(defvar-local kill-line-query-function #'kill-line-query-default)
(defun kill-line-query-default (beg end)
  (let ((nlines <count invisible lines between beg end>))
  (when (> nlines threshold)
    (format "Kill %d invisible lines? " nlines))))

Then, Org mode can instead have

(setq-local kill-line-query-function #'org-kill-line-query)
(defun org-kill-line-query (beg end)
  (org-with-point-at beg
    (when (and (org-at-heading-p)
               (progn
                 (end-of-line)
                 (and (< (point) end)
                      (org-fold-folded-p))))
       "Kill hidden subtree along with headline? ")))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Thu, 11 Jan 2024 18:08:11 +0000
> 
> Then, Org mode can instead have
> 
> (setq-local kill-line-query-function #'org-kill-line-query)
> (defun org-kill-line-query (beg end)
>   (org-with-point-at beg
>     (when (and (org-at-heading-p)
>                (progn
>                  (end-of-line)
>                  (and (< (point) end)
>                       (org-fold-folded-p))))
>        "Kill hidden subtree along with headline? ")))

I don't know what org-with-point-at and org-fold-folded-p do, but my
point is that you should consider the case when kill-line kills
invisible text that has nothing to do with Org's headings and trees,
so I suggest to either make the detection code smarter (so it could
distinguish between the two), or make the prompt text vaguer (to not
claim that the text must be a subtree).

And if that is still not clear or you disagree, let's leave it at
that.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Then, Org mode can instead have
>> 
>> (setq-local kill-line-query-function #'org-kill-line-query)
>> (defun org-kill-line-query (beg end)
>>   (org-with-point-at beg
>>     (when (and (org-at-heading-p)
>>                (progn
>>                  (end-of-line)
>>                  (and (< (point) end)
>>                       (org-fold-folded-p))))
>>        "Kill hidden subtree along with headline? ")))
>
> I don't know what org-with-point-at and org-fold-folded-p do, but my
> point is that you should consider the case when kill-line kills
> invisible text that has nothing to do with Org's headings and trees,
> so I suggest to either make the detection code smarter (so it could
> distinguish between the two), or make the prompt text vaguer (to not
> claim that the text must be a subtree).
>
> And if that is still not clear or you disagree, let's leave it at
> that.

I think that I see what you mean. What we can do to achieve this is
changing `kill-line-query-function' into abnormal hook
`kill-line-query-functions'. Then, `kill-line'/`kill-whole-line' will
use (run-hook-with-args-until-success 'kill-line-query-functions) to
decide whether to show a query.

Does it make sense?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Fri, 12 Jan 2024 12:24:01 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Then, Org mode can instead have
> >> 
> >> (setq-local kill-line-query-function #'org-kill-line-query)
> >> (defun org-kill-line-query (beg end)
> >>   (org-with-point-at beg
> >>     (when (and (org-at-heading-p)
> >>                (progn
> >>                  (end-of-line)
> >>                  (and (< (point) end)
> >>                       (org-fold-folded-p))))
> >>        "Kill hidden subtree along with headline? ")))
> >
> > I don't know what org-with-point-at and org-fold-folded-p do, but my
> > point is that you should consider the case when kill-line kills
> > invisible text that has nothing to do with Org's headings and trees,
> > so I suggest to either make the detection code smarter (so it could
> > distinguish between the two), or make the prompt text vaguer (to not
> > claim that the text must be a subtree).
> >
> > And if that is still not clear or you disagree, let's leave it at
> > that.
> 
> I think that I see what you mean. What we can do to achieve this is
> changing `kill-line-query-function' into abnormal hook
> `kill-line-query-functions'. Then, `kill-line'/`kill-whole-line' will
> use (run-hook-with-args-until-success 'kill-line-query-functions) to
> decide whether to show a query.
> 
> Does it make sense?

It might, yes.  But do we have to _replace_ the hook? cannot we have
both?  I.e. if the new one is defined, call it, otherwise call the old
one.  That would be more backward-compatible, I think.

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think that I see what you mean. What we can do to achieve this is
>> changing `kill-line-query-function' into abnormal hook
>> `kill-line-query-functions'. Then, `kill-line'/`kill-whole-line' will
>> use (run-hook-with-args-until-success 'kill-line-query-functions) to
>> decide whether to show a query.
>> 
>> Does it make sense?
>
> It might, yes.  But do we have to _replace_ the hook? cannot we have
> both?  I.e. if the new one is defined, call it, otherwise call the old
> one.  That would be more backward-compatible, I think.

May you please elaborate what you mean by "old one"?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, emacs-orgmode@gnu.org, 65734@debbugs.gnu.org,
>  manikulin@gmail.com, iota@whxvd.name
> Date: Fri, 12 Jan 2024 12:39:18 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I think that I see what you mean. What we can do to achieve this is
> >> changing `kill-line-query-function' into abnormal hook
> >> `kill-line-query-functions'. Then, `kill-line'/`kill-whole-line' will
> >> use (run-hook-with-args-until-success 'kill-line-query-functions) to
> >> decide whether to show a query.
> >> 
> >> Does it make sense?
> >
> > It might, yes.  But do we have to _replace_ the hook? cannot we have
> > both?  I.e. if the new one is defined, call it, otherwise call the old
> > one.  That would be more backward-compatible, I think.
> 
> May you please elaborate what you mean by "old one"?

kill-line-query-function

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> May you please elaborate what you mean by "old one"?
>
> kill-line-query-function

There is no such variable in Emacs. I proposed to introduce a new
variable `kill-line-query-function' earlier in this thread. There is no
backwards-compatibility problem we need to solve here.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

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

> (setq-local kill-line-query-function #'org-kill-line-query)

Please use `add-function` for such things.
That's its raison d'être.


        Stefan

bug#65734: [BUG] kill-whole-line on folded subtrees [9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)]

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (setq-local kill-line-query-function #'org-kill-line-query)
>
> Please use `add-function` for such things.
> That's its raison d'être.

When comparing imaginary kill-line-query-functions (abnormal hook) +
add-hook vs. kill-line-query-function + add-function, which one will be
more preferred?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>