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/)
[-- 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
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
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
[-- 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
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.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
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