* [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/)]
@ 2023-09-04 16:30 Sebastian Miele
2023-09-05 10:29 ` Ihor Radchenko
0 siblings, 1 reply; 23+ messages in thread
From: Sebastian Miele @ 2023-09-04 16:30 UTC (permalink / raw)
To: emacs-orgmode
In an emacs -Q, create an Org buffer with the following contents:
--8<---------------cut here---------------start------------->8---
* AB
** C
--8<---------------cut here---------------end--------------->8---
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.
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.
The following patch to the Emacs sources fixes the issue:
diff --git a/lisp/simple.el b/lisp/simple.el
index abd587245fe..44221f3fc24 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6649,9 +6649,7 @@ kill-whole-line
(unless (bobp) (backward-char))
(point))))
(t
- (save-excursion
- (kill-region (point) (progn (forward-visible-line 0) (point))))
- (kill-region (point)
+ (kill-region (save-excursion (forward-visible-line 0) (point))
(progn (forward-visible-line arg) (point))))))
(defun forward-visible-line (arg)
The reason for the issue probably is: Without the patch, the killing
happens in two stages. The first kill-region kills from the beginning
of the line until after the A. That kills the leading *. That probably
somehow triggers Org visibility changes. With the patch applied the
whole killing happens in one stage, probably without causing an
intermediate change of visibility.
I 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?
Emacs : GNU Emacs 29.1.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)
of 2023-09-04
Package: Org mode version 9.6.8 (release_9.6.8-3-g21171d @ /home/w/usr/emacs/0/29/0/lisp/org/)
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [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/)]
2023-09-04 16:30 [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
@ 2023-09-05 10:29 ` Ihor Radchenko
2023-09-05 11:54 ` bug#65734: " Eli Zaretskii
2023-09-05 14:30 ` Max Nikulin
0 siblings, 2 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-05 10:29 UTC (permalink / raw)
To: Sebastian Miele; +Cc: emacs-orgmode, 65734
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 10:29 ` Ihor Radchenko
@ 2023-09-05 11:54 ` Eli Zaretskii
2023-09-05 15:25 ` Sebastian Miele
2023-09-06 8:30 ` Ihor Radchenko
2023-09-05 14:30 ` Max Nikulin
1 sibling, 2 replies; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-05 11:54 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: iota, 65734, emacs-orgmode
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 10:29 ` Ihor Radchenko
2023-09-05 11:54 ` bug#65734: " Eli Zaretskii
@ 2023-09-05 14:30 ` Max Nikulin
2023-09-05 15:42 ` Eli Zaretskii
1 sibling, 1 reply; 23+ messages in thread
From: Max Nikulin @ 2023-09-05 14:30 UTC (permalink / raw)
To: Ihor Radchenko, Sebastian Miele; +Cc: 65734, emacs-orgmode
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 11:54 ` bug#65734: " Eli Zaretskii
@ 2023-09-05 15:25 ` Sebastian Miele
2023-09-05 15:50 ` Eli Zaretskii
2023-09-06 8:30 ` Ihor Radchenko
1 sibling, 1 reply; 23+ messages in thread
From: Sebastian Miele @ 2023-09-05 15:25 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Ihor Radchenko, 65734, emacs-orgmode
> 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.
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 14:30 ` Max Nikulin
@ 2023-09-05 15:42 ` Eli Zaretskii
2023-09-05 15:50 ` Ihor Radchenko
0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-05 15:42 UTC (permalink / raw)
To: Max Nikulin; +Cc: yantar92, iota, 65734, emacs-orgmode
> 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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 15:25 ` Sebastian Miele
@ 2023-09-05 15:50 ` Eli Zaretskii
2023-09-06 8:23 ` Ihor Radchenko
0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-05 15:50 UTC (permalink / raw)
To: Sebastian Miele; +Cc: yantar92, 65734, emacs-orgmode
> 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...
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 15:42 ` Eli Zaretskii
@ 2023-09-05 15:50 ` Ihor Radchenko
2023-09-05 16:02 ` Max Nikulin
2023-09-05 16:14 ` Eli Zaretskii
0 siblings, 2 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-05 15:50 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Max Nikulin, iota, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 15:50 ` Ihor Radchenko
@ 2023-09-05 16:02 ` Max Nikulin
2023-09-05 16:12 ` Ihor Radchenko
2023-09-05 16:14 ` Eli Zaretskii
1 sibling, 1 reply; 23+ messages in thread
From: Max Nikulin @ 2023-09-05 16:02 UTC (permalink / raw)
To: Ihor Radchenko, Eli Zaretskii; +Cc: iota, 65734, emacs-orgmode
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 16:02 ` Max Nikulin
@ 2023-09-05 16:12 ` Ihor Radchenko
0 siblings, 0 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-05 16:12 UTC (permalink / raw)
To: Max Nikulin; +Cc: Eli Zaretskii, iota, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 15:50 ` Ihor Radchenko
2023-09-05 16:02 ` Max Nikulin
@ 2023-09-05 16:14 ` Eli Zaretskii
1 sibling, 0 replies; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-05 16:14 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: manikulin, iota, 65734, emacs-orgmode
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 15:50 ` Eli Zaretskii
@ 2023-09-06 8:23 ` Ihor Radchenko
2023-09-06 12:16 ` Eli Zaretskii
0 siblings, 1 reply; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-06 8:23 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Sebastian Miele, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-05 11:54 ` bug#65734: " Eli Zaretskii
2023-09-05 15:25 ` Sebastian Miele
@ 2023-09-06 8:30 ` Ihor Radchenko
2023-09-06 12:20 ` Eli Zaretskii
2023-09-06 15:04 ` Sebastian Miele
1 sibling, 2 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-06 8:30 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: iota, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 8:23 ` Ihor Radchenko
@ 2023-09-06 12:16 ` Eli Zaretskii
2023-09-06 13:30 ` Sebastian Miele
0 siblings, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-06 12:16 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: iota, 65734, emacs-orgmode
> 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.)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 8:30 ` Ihor Radchenko
@ 2023-09-06 12:20 ` Eli Zaretskii
2023-09-07 10:00 ` Ihor Radchenko
2023-09-06 15:04 ` Sebastian Miele
1 sibling, 1 reply; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-06 12:20 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: iota, 65734, emacs-orgmode
> 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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 12:16 ` Eli Zaretskii
@ 2023-09-06 13:30 ` Sebastian Miele
2023-09-07 13:49 ` Ihor Radchenko
0 siblings, 1 reply; 23+ messages in thread
From: Sebastian Miele @ 2023-09-06 13:30 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Ihor Radchenko, 65734, emacs-orgmode
> 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.)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 8:30 ` Ihor Radchenko
2023-09-06 12:20 ` Eli Zaretskii
@ 2023-09-06 15:04 ` Sebastian Miele
2023-09-07 10:03 ` Ihor Radchenko
1 sibling, 1 reply; 23+ messages in thread
From: Sebastian Miele @ 2023-09-06 15:04 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Eli Zaretskii, 65734, emacs-orgmode
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 12:20 ` Eli Zaretskii
@ 2023-09-07 10:00 ` Ihor Radchenko
2023-09-07 10:19 ` Eli Zaretskii
0 siblings, 1 reply; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-07 10:00 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: iota, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 15:04 ` Sebastian Miele
@ 2023-09-07 10:03 ` Ihor Radchenko
0 siblings, 0 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-07 10:03 UTC (permalink / raw)
To: Sebastian Miele; +Cc: Eli Zaretskii, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-07 10:00 ` Ihor Radchenko
@ 2023-09-07 10:19 ` Eli Zaretskii
2023-09-07 10:27 ` Sebastian Miele
2023-09-07 13:43 ` Ihor Radchenko
0 siblings, 2 replies; 23+ messages in thread
From: Eli Zaretskii @ 2023-09-07 10:19 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: iota, 65734, emacs-orgmode
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-07 10:19 ` Eli Zaretskii
@ 2023-09-07 10:27 ` Sebastian Miele
2023-09-07 13:43 ` Ihor Radchenko
1 sibling, 0 replies; 23+ messages in thread
From: Sebastian Miele @ 2023-09-07 10:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Ihor Radchenko, 65734, emacs-orgmode
> 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-07 10:19 ` Eli Zaretskii
2023-09-07 10:27 ` Sebastian Miele
@ 2023-09-07 13:43 ` Ihor Radchenko
1 sibling, 0 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-07 13:43 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: iota, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: 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/)]
2023-09-06 13:30 ` Sebastian Miele
@ 2023-09-07 13:49 ` Ihor Radchenko
0 siblings, 0 replies; 23+ messages in thread
From: Ihor Radchenko @ 2023-09-07 13:49 UTC (permalink / raw)
To: Sebastian Miele; +Cc: Eli Zaretskii, 65734, emacs-orgmode
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2023-09-07 13:49 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-04 16:30 [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
2023-09-05 10:29 ` Ihor Radchenko
2023-09-05 11:54 ` bug#65734: " Eli Zaretskii
2023-09-05 15:25 ` Sebastian Miele
2023-09-05 15:50 ` Eli Zaretskii
2023-09-06 8:23 ` Ihor Radchenko
2023-09-06 12:16 ` Eli Zaretskii
2023-09-06 13:30 ` Sebastian Miele
2023-09-07 13:49 ` Ihor Radchenko
2023-09-06 8:30 ` Ihor Radchenko
2023-09-06 12:20 ` Eli Zaretskii
2023-09-07 10:00 ` Ihor Radchenko
2023-09-07 10:19 ` Eli Zaretskii
2023-09-07 10:27 ` Sebastian Miele
2023-09-07 13:43 ` Ihor Radchenko
2023-09-06 15:04 ` Sebastian Miele
2023-09-07 10:03 ` Ihor Radchenko
2023-09-05 14:30 ` Max Nikulin
2023-09-05 15:42 ` Eli Zaretskii
2023-09-05 15:50 ` Ihor Radchenko
2023-09-05 16:02 ` Max Nikulin
2023-09-05 16:12 ` Ihor Radchenko
2023-09-05 16:14 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).