[BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Allen Li]

The docstring for org-todo (C-c C-t) is unclear about the handling of -
prefix compared to -1 prefix.

Here is the relevant part of the docstring:

    With ‘C-u’ prefix ARG, force logging the state change and take a
    logging note.
    With a ‘C-u C-u’ prefix, switch to the next set of TODO keywords (nextset).
    Another way to achieve this is ‘S-C-<right>’.
    With a ‘C-u C-u C-u’ prefix, circumvent any state blocking.
    With numeric prefix arg, switch to the Nth state.

    With a numeric prefix arg of 0, inhibit note taking for the change.
    With a numeric prefix arg of -1, cancel repeater to allow marking as DONE.

The actual behavior is that - (along with any other negative numeric
prefix excluding -1) switches to the first state and -1 cancels
the repeater.

It seems like the right fix here is to make - behave the same as -1, and
raise a user error for any other negative numeric prefix, since it is
likely not doing whatever the user wanted.

Emacs  : GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.27, cairo version 1.17.4)
 of 2021-03-26
Package: Org mode version 9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Allen Li]

[-- Attachment #1.1: Type: text/plain, Size: 482 bytes --]

On Sat, Jan 1, 2022 at 9:31 AM Allen Li <darkfeline@felesatra.moe> wrote:

> It seems like the right fix here is to make - behave the same as -1, and
> raise a user error for any other negative numeric prefix, since it is
> likely not doing whatever the user wanted.
>

Attached a small patch fixing this, which I tested manually.

Feel free to remove the negative arg error if it's not desired (although I
don't think there is a use case for, e.g., typing M-- M-2 instead of M-1).

[-- Attachment #1.2: Type: text/html, Size: 782 bytes --]

[-- Attachment #2: 0001-org-Improve-org-todo-handling-of-negative-prefix-arg.patch --]
[-- Type: text/x-patch, Size: 1056 bytes --]

From 16ff89c309b8bd9aa11183cc9620c56ed96e3ff7 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sat, 1 Jan 2022 01:38:35 -0800
Subject: [PATCH] org: Improve org-todo handling of negative prefix args

* lisp/org.el (org-todo): Handle -1 prefix args consistently and error
on other negative args.
---
 lisp/org.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index e2f315a4c..6b48f660e 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9785,7 +9785,8 @@ When called through ELisp, arg is also interpreted in the following way:
 	 nil cl
 	 (when (org-invisible-p) (org-end-of-subtree nil t))))
     (when (equal arg '(16)) (setq arg 'nextset))
-    (when (equal arg -1) (org-cancel-repeater) (setq arg nil))
+    (when (equal (prefix-numeric-value arg) -1) (org-cancel-repeater) (setq arg nil))
+    (when (< (prefix-numeric-value arg) -1) (user-error "Prefix argument %d not supported" arg))
     (let ((org-blocker-hook org-blocker-hook)
 	  commentp
 	  case-fold-search)
-- 
2.34.1

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Ihor Radchenko]

Allen Li <darkfeline@felesatra.moe> writes:

> On Sat, Jan 1, 2022 at 9:31 AM Allen Li <darkfeline@felesatra.moe> wrote:
>
>> It seems like the right fix here is to make - behave the same as -1, and
>> raise a user error for any other negative numeric prefix, since it is
>> likely not doing whatever the user wanted.
>>
>
> Attached a small patch fixing this, which I tested manually.

The patch looks reasonable. Marking the message as a patch to make it
appear in https://updates.orgmode.org/

Best,
Ihor

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Ihor Radchenko]

Ihor Radchenko <yantar92@gmail.com> writes:

> Allen Li <darkfeline@felesatra.moe> writes:
>
>> On Sat, Jan 1, 2022 at 9:31 AM Allen Li <darkfeline@felesatra.moe> wrote:
>>
>>> It seems like the right fix here is to make - behave the same as -1, and
>>> raise a user error for any other negative numeric prefix, since it is
>>> likely not doing whatever the user wanted.
>>>
>>
>> Attached a small patch fixing this, which I tested manually.

Applied onto main via bfd63cc4f.

Would you also be interested to write a test checking org-todo ARGS?

Best,
Ihor

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Allen Li]

[-- Attachment #1.1: Type: text/plain, Size: 689 bytes --]

On Fri, Jun 17, 2022 at 11:09 PM Ihor Radchenko <yantar92@gmail.com> wrote:

> Ihor Radchenko <yantar92@gmail.com> writes:
>
> > Allen Li <darkfeline@felesatra.moe> writes:
> >
> >> On Sat, Jan 1, 2022 at 9:31 AM Allen Li <darkfeline@felesatra.moe>
> wrote:
> >>
> >>> It seems like the right fix here is to make - behave the same as -1,
> and
> >>> raise a user error for any other negative numeric prefix, since it is
> >>> likely not doing whatever the user wanted.
> >>>
> >>
> >> Attached a small patch fixing this, which I tested manually.
>
> Applied onto main via bfd63cc4f.
>
> Would you also be interested to write a test checking org-todo ARGS?
>

Attached


>
> Best,
> Ihor
>

[-- Attachment #1.2: Type: text/html, Size: 1308 bytes --]

[-- Attachment #2: 0001-test-org-Add-test-for-org-todo-prefix-behavior.patch --]
[-- Type: text/x-patch, Size: 1391 bytes --]

From 9255819ee05b9f2bdbe8de1cf0acbd6ae6553152 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sat, 25 Jun 2022 21:27:20 -0700
Subject: [PATCH] test-org: Add test for org-todo prefix behavior

* testing/lisp/test-org.el (test-org/org-todo-prefix): Add test.
---
 testing/lisp/test-org.el | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 121f9efd5..304dddc08 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -7753,6 +7753,25 @@ CLOSED: %s
           (org-add-log-note))
         (buffer-string))))))
 
+(ert-deftest test-org/org-todo-prefix ()
+  "Test `org-todo' prefix arg behavior."
+  ;; -1 prefix arg should cancel repeater and mark DONE.
+  (should-not
+   (string-prefix-p
+    "* TODO H"
+    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
+      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
+	                       (org-todo -1)
+	                       (buffer-string)))))
+  ;; - prefix arg should cancel repeater and mark DONE.
+  (should-not
+   (string-prefix-p
+    "* TODO H"
+    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
+      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
+	                       (org-todo '-)
+	                       (buffer-string))))))
+
 \f
 ;;; Timestamps API
 
-- 
2.36.1

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Ihor Radchenko]

Allen Li <darkfeline@felesatra.moe> writes:

>> Would you also be interested to write a test checking org-todo ARGS?
>>
>
> Attached

Thanks! However, it is unclear what this test is checking for.

> +(ert-deftest test-org/org-todo-prefix ()
> +  "Test `org-todo' prefix arg behavior."
> +  ;; -1 prefix arg should cancel repeater and mark DONE.
> +  (should-not
> +   (string-prefix-p
> +    "* TODO H"
> +    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
> +      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
> +	                       (org-todo -1)
> +	                       (buffer-string)))))

This test does not check if the task is actually marked DONE and also
does not check if the repeater is canceled.

Best,
Ihor

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Allen Li]

[-- Attachment #1.1: Type: text/plain, Size: 891 bytes --]

Attached

On Sat, Jun 25, 2022 at 9:48 PM Ihor Radchenko <yantar92@gmail.com> wrote:

> Allen Li <darkfeline@felesatra.moe> writes:
>
> >> Would you also be interested to write a test checking org-todo ARGS?
> >>
> >
> > Attached
>
> Thanks! However, it is unclear what this test is checking for.
>
> > +(ert-deftest test-org/org-todo-prefix ()
> > +  "Test `org-todo' prefix arg behavior."
> > +  ;; -1 prefix arg should cancel repeater and mark DONE.
> > +  (should-not
> > +   (string-prefix-p
> > +    "* TODO H"
> > +    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
> > +      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
> > +                            (org-todo -1)
> > +                            (buffer-string)))))
>
> This test does not check if the task is actually marked DONE and also
> does not check if the repeater is canceled.
>
> Best,
> Ihor
>
>

[-- Attachment #1.2: Type: text/html, Size: 1409 bytes --]

[-- Attachment #2: 0001-test-org-Add-test-for-org-todo-prefix-behavior.patch --]
[-- Type: text/x-patch, Size: 1351 bytes --]

From 42156ba1e8b3c589394212ef423f99e122544c5f Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sat, 25 Jun 2022 21:27:20 -0700
Subject: [PATCH] test-org: Add test for org-todo prefix behavior

* testing/lisp/test-org.el (test-org/org-todo-prefix): Add test.
---
 testing/lisp/test-org.el | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 121f9efd5..d686c0e3b 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -7753,6 +7753,25 @@ CLOSED: %s
           (org-add-log-note))
         (buffer-string))))))
 
+(ert-deftest test-org/org-todo-prefix ()
+  "Test `org-todo' prefix arg behavior."
+  ;; -1 prefix arg should cancel repeater and mark DONE.
+  (should
+   (string-match-p
+    "DONE H\\(.*\n\\)*<2012-03-29 Thu \\+0y>"
+    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
+      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
+	(org-todo -1)
+	(buffer-string)))))
+  ;; - prefix arg should cancel repeater and mark DONE.
+  (should
+   (string-match-p
+    "DONE H\\(.*\n\\)*<2012-03-29 Thu \\+0y>"
+    (let ((org-todo-keywords '((sequence "TODO" "DONE"))))
+      (org-test-with-temp-text "* TODO H\n<2012-03-29 Thu +2y>"
+	(org-todo '-)
+	(buffer-string))))))
+
 \f
 ;;; Timestamps API
 
-- 
2.36.1

Re: [BUG] org-todo (C-c C-t) bad negative prefix behavior [9.5.2 (9.5.2-gfbff08 @ /home/ionasal/.emacs.d/elpa/org-9.5.2/)]

[Ihor Radchenko]

Allen Li <darkfeline@felesatra.moe> writes:

> Attached

Thanks!
Applied onto main via 711ada5ac.
I changed the patch slightly adding
;; FIXME: Add tests for all other allowed prefix arguments.
line to the test.

Best,
Ihor