Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Hi Ihor, Ihor Radchenko writes: > Bhavin Gandhi writes: > >> I was able to reproduce this, and here are my findings as well as a >> reproducible configuration with only a few settings. > > The breakage was introduced in commit c67037: > > [c670379adfbdc4883d3cfa230289fd2829993265] Fix `org-agenda-todo' undo > behavior when logging (not adding note) > > The fix is attached. Applied in the bugfix branch, thanks. This area is quite fragile, so please let's test this heavily. Thanks! -- Bastien
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Hi Bastien, On Sat, 25 Sep 2021 at 17:25, Bastien wrote: Ihor Radchenko writes: And yet another update fixing a typo in previous patch. Applied, thanks! Thank you! Ihor, thank you for the patch! And thanks to all who chimed in. Best, Gustavo.
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Ihor Radchenko writes: > And yet another update fixing a typo in previous patch. Applied, thanks!
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Bhavin Gandhi writes: > I was trying to understand your change. So, when we call > `org-agenda-todo', it calls `org-todo' which adds the > post-command-hook. This hook is supposed to run when `org-agenda-todo' > finishes, but instead of that we call it directly. This makes sure that > the change is recorded in `buffer-undo-list'. You are almost correct. To be able to use undo from agenda, we must have all the changes happen inside org-with-remote-undo body. Only then the changes are recorded into agenda's buffer `buffer-undo-list'. post-command-hook, if ran after `org-agenda-todo', will only record changes in the actual org buffer's `buffer-undo-list', but not inside the agenda's `buffer-undo-list'. > Sorry if that's too much to ask, but why don't we need something similar > when org-log-note-how is 'note? Can you please help me understand that? > I tried reading org-add-log-note and org-store-log-note, but I think I'm > missing something basic here. AFAIK, it is quite hard to do with current log note implementation. `org-add-log-note' itself does not record the note text. Instead, it only creates and pre-populates the note buffer and returns the control to the function calling `org-add-log-note'. Regardless where we call `org-add-log-note', the actual note text will only be added to the org buffer when the user presses C-c C-c in the note buffer. And the user input will only be possible after the current command (in our case `org-agenda-todo') finishes. Thus, user note will always be added outside `org-with-remote-undo' and cannot be recorded by agenda. IMHO, the proper way to handle this would be rewriting the log-note code using recursive editing. But that's not a trivial change and should be implemented in a separate patch. > Also, should this line from org.el (org-store-log-note) be removed? > > ;; Don't add undo information when called from `org-agenda-todo'. I think so. It appears to be irrelevant to current state of the code. Someone forgot to remove this comment in one of the past patches. But removing the comment should be a separate patch itself. Best, Ihor
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
On Sun, 11 Jul 2021 at 06:59, Ihor Radchenko wrote: > You are right. I believe that I fixed the breakage in the attached > patch. Also, I noticed that c67037 did not fix the original bug when > state change requests interactive note. Thanks! I tested your latest patch, and it is fixing both the original issues which `c67037' solved as well as this one. I was trying to understand your change. So, when we call `org-agenda-todo', it calls `org-todo' which adds the post-command-hook. This hook is supposed to run when `org-agenda-todo' finishes, but instead of that we call it directly. This makes sure that the change is recorded in `buffer-undo-list'. Sorry if that's too much to ask, but why don't we need something similar when org-log-note-how is 'note? Can you please help me understand that? I tried reading org-add-log-note and org-store-log-note, but I think I'm missing something basic here. Also, should this line from org.el (org-store-log-note) be removed? ;; Don't add undo information when called from `org-agenda-todo'.
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
And yet another update fixing a typo in previous patch. Sorry >From 89a60d654663b68f1c149fb3341a50191027f45e Mon Sep 17 00:00:00 2001 Message-Id: <89a60d654663b68f1c149fb3341a50191027f45e.1625983004.git.yanta...@gmail.com> From: Ihor Radchenko Date: Sat, 10 Jul 2021 21:43:44 +0800 Subject: [PATCH] Fix duplicate logbook entry for repeated tasks * lisp/org.el (org-add-log-setup): Always run `org-add-log-note' via `post-command-hook'. Otherwise, there is no way to know if a note was requested for `this-command'. Running `org-add-log-note' directly would, for example, break `org-auto-repeat-maybe' as reported in [1]. * lisp/org-agenda.el (org-agenda-todo): Avoid reintroducing the bug fixed in c670379adf. * testing/lisp/test-org.el: Add test checking the reported bug. [1] https://orgmode.org/list/CAOn=hbcaW1R6vtun-E2r4LS=j3dp=vjqmjgtzy8uc1sypar...@mail.gmail.com --- lisp/org-agenda.el | 6 +- lisp/org.el | 4 +--- testing/lisp/test-org.el | 23 ++- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el index 44acd035a..4cd527e5b 100644 --- a/lisp/org-agenda.el +++ b/lisp/org-agenda.el @@ -9433,7 +9433,11 @@ (defun org-agenda-todo ( arg) (goto-char pos) (org-show-context 'agenda) (let ((current-prefix-arg arg)) - (call-interactively 'org-todo)) + (call-interactively 'org-todo) + ;; Make sure that log is recorded in current undo. + (when (and org-log-setup + (not (eq org-log-note-how 'note))) + (org-add-log-note))) (and (bolp) (forward-char 1)) (setq newhead (org-get-heading)) (when (and org-agenda-headline-snapshot-before-repeat diff --git a/lisp/org.el b/lisp/org.el index ffcc5945d..3d15771a2 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -10939,9 +10939,7 @@ (defun org-add-log-setup ( purpose state prev-state how extra) org-log-note-extra extra org-log-note-effective-time (org-current-effective-time) org-log-setup t) - (if (eq how 'note) - (add-hook 'post-command-hook 'org-add-log-note 'append) -(org-add-log-note purpose))) + (add-hook 'post-command-hook 'org-add-log-note 'append)) (defun org-skip-over-state-notes () "Skip past the list of State notes in an entry." diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el index de3c6f3c9..c2267c32d 100644 --- a/testing/lisp/test-org.el +++ b/testing/lisp/test-org.el @@ -7396,7 +7396,28 @@ (ert-deftest test-org/auto-repeat-maybe () SCHEDULED: <2012-03-29 Thu +2y> CLOCK: [2012-03-29 Thu 10:00]--[2012-03-29 Thu 16:40] => 6:40" (org-todo "DONE") - (buffer-string)) + (buffer-string) + ;; Make sure that logbook state change record does not get + ;; duplicated when `org-log-repeat' `org-log-done' are non-nil. + (should + (string-match-p +(rx "* TODO Read book +SCHEDULED: <2021-06-16 Wed +1d> +:PROPERTIES: +:LAST_REPEAT:" (1+ nonl) " +:END: +- State \"DONE\" from \"TODO\"" (1+ nonl) buffer-end) +(let ((org-log-repeat 'time) + (org-todo-keywords '((sequence "TODO" "|" "DONE(d!)"))) + (org-log-into-drawer nil)) + (org-test-with-temp-text + "* TODO Read book +SCHEDULED: <2021-06-15 Tue +1d>" +(org-todo "DONE") +(when (memq 'org-add-log-note post-command-hook) + (org-add-log-note)) +(buffer-string)) + ;;; Timestamps API -- 2.31.1
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Updating the patch with relevant new test >From 5b89c95318a38df9f0a6706b6eb5320baafbd4d8 Mon Sep 17 00:00:00 2001 Message-Id: <5b89c95318a38df9f0a6706b6eb5320baafbd4d8.1625981997.git.yanta...@gmail.com> From: Ihor Radchenko Date: Sat, 10 Jul 2021 21:43:44 +0800 Subject: [PATCH] Fix duplicate logbook entry for repeated tasks * lisp/org.el (org-add-log-setup): Always run `org-add-log-note' via `post-command-hook'. Otherwise, there is no way to know if a note was requested for `this-command'. Running `org-add-log-note' directly would, for example, break `org-auto-repeat-maybe' as reported in [1]. * lisp/org-agenda.el (org-agenda-todo): Avoid reintroducing the bug fixed in c670379adf. * testing/lisp/test-org.el: Add test checking the reported bug. [1] https://orgmode.org/list/CAOn=hbcaW1R6vtun-E2r4LS=j3dp=vjqmjgtzy8uc1sypar...@mail.gmail.com --- lisp/org-agenda.el | 6 +- lisp/org.el | 4 +--- testing/lisp/test-org.el | 21 + 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el index 44acd035a..4cd527e5b 100644 --- a/lisp/org-agenda.el +++ b/lisp/org-agenda.el @@ -9433,7 +9433,11 @@ (defun org-agenda-todo ( arg) (goto-char pos) (org-show-context 'agenda) (let ((current-prefix-arg arg)) - (call-interactively 'org-todo)) + (call-interactively 'org-todo) + ;; Make sure that log is recorded in current undo. + (when (and org-log-setup + (not (eq org-log-note-how 'note))) + (org-add-log-note))) (and (bolp) (forward-char 1)) (setq newhead (org-get-heading)) (when (and org-agenda-headline-snapshot-before-repeat diff --git a/lisp/org.el b/lisp/org.el index ffcc5945d..3d15771a2 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -10939,9 +10939,7 @@ (defun org-add-log-setup ( purpose state prev-state how extra) org-log-note-extra extra org-log-note-effective-time (org-current-effective-time) org-log-setup t) - (if (eq how 'note) - (add-hook 'post-command-hook 'org-add-log-note 'append) -(org-add-log-note purpose))) + (add-hook 'post-command-hook 'org-add-log-note 'append)) (defun org-skip-over-state-notes () "Skip past the list of State notes in an entry." diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el index de3c6f3c9..0634ba608 100644 --- a/testing/lisp/test-org.el +++ b/testing/lisp/test-org.el @@ -7397,6 +7397,27 @@ (ert-deftest test-org/auto-repeat-maybe () CLOCK: [2012-03-29 Thu 10:00]--[2012-03-29 Thu 16:40] => 6:40" (org-todo "DONE") (buffer-string)) + ;; Make sure that logbook state change record does not get + ;; duplicated when `org-log-repeat' `org-log-done' are non-nil. + (should + (string-match-p +(rx "* TODO Read book +SCHEDULED: <2021-06-16 Wed +1d> +:PROPERTIES: +:LAST_REPEAT:" (1+ nonl) " +:END: +- State \"DONE\" from \"TODO\"" (1+ nonl) buffer-end) +(let ((org-log-repeat 'time) + (org-todo-keywords '((sequence "TODO" "|" "DONE(d!)"))) + (org-log-into-drawer nil)) + (org-test-with-temp-text + "* TODO Read book +SCHEDULED: <2021-06-15 Tue +1d>" +(org-todo "DONE") +(when (memq 'org-add-log-note post-command-hook) + (org-add-log-note)) +(buffer-string)) + ;;; Timestamps API -- 2.31.1
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Bhavin Gandhi writes: > Thank you! I tried the patch, but the original bug[1] which the commit > `c67037' tried to fix, gets introduced again. Basically, > `org-agenda-undo' just removes the logbook entry and the scheduled date > remains new. You are right. I believe that I fixed the breakage in the attached patch. Also, I noticed that c67037 did not fix the original bug when state change requests interactive note. Best, Ihor >From f012648b350013e33ef0e88afc85b4fcf048734b Mon Sep 17 00:00:00 2001 Message-Id: From: Ihor Radchenko Date: Sat, 10 Jul 2021 21:43:44 +0800 Subject: [PATCH] Fix duplicate logbook entry for repeated tasks * lisp/org.el (org-add-log-setup): Always run `org-add-log-note' via `post-command-hook'. Otherwise, there is no way to know if a note was requested for `this-command'. Running `org-add-log-note' directly would, for example, break `org-auto-repeat-maybe' as reported in [1]. * lisp/org-agenda.el (org-agenda-todo): Avoid reintroducing the bug fixed in c670379adf. [1] https://orgmode.org/list/CAOn=hbcaW1R6vtun-E2r4LS=j3dp=vjqmjgtzy8uc1sypar...@mail.gmail.com --- lisp/org-agenda.el | 6 +- lisp/org.el| 4 +--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el index 44acd035a..4cd527e5b 100644 --- a/lisp/org-agenda.el +++ b/lisp/org-agenda.el @@ -9433,7 +9433,11 @@ (defun org-agenda-todo ( arg) (goto-char pos) (org-show-context 'agenda) (let ((current-prefix-arg arg)) - (call-interactively 'org-todo)) + (call-interactively 'org-todo) + ;; Make sure that log is recorded in current undo. + (when (and org-log-setup + (not (eq org-log-note-how 'note))) + (org-add-log-note))) (and (bolp) (forward-char 1)) (setq newhead (org-get-heading)) (when (and org-agenda-headline-snapshot-before-repeat diff --git a/lisp/org.el b/lisp/org.el index ffcc5945d..3d15771a2 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -10939,9 +10939,7 @@ (defun org-add-log-setup ( purpose state prev-state how extra) org-log-note-extra extra org-log-note-effective-time (org-current-effective-time) org-log-setup t) - (if (eq how 'note) - (add-hook 'post-command-hook 'org-add-log-note 'append) -(org-add-log-note purpose))) + (add-hook 'post-command-hook 'org-add-log-note 'append)) (defun org-skip-over-state-notes () "Skip past the list of State notes in an entry." -- 2.31.1
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Hello Ihor, On Sat, 10 Jul 2021 at 19:18, Ihor Radchenko wrote: > The breakage was introduced in commit c67037: > > [c670379adfbdc4883d3cfa230289fd2829993265] Fix `org-agenda-todo' undo > behavior when logging (not adding note) > > The fix is attached. Thank you! I tried the patch, but the original bug[1] which the commit `c67037' tried to fix, gets introduced again. Basically, `org-agenda-undo' just removes the logbook entry and the scheduled date remains new. [1] https://orgmode.org/list/87v98a8mes@gnu.org/
Re: [PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Hi Ihor, On Sat, 10 Jul 2021 at 10:48, Ihor Radchenko wrote: The breakage was introduced in commit c67037: [c670379adfbdc4883d3cfa230289fd2829993265] Fix `org-agenda-todo' undo behavior when logging (not adding note) The fix is attached. Thank you very much! Best, Gustavo.
[PATCH] Re: Bug: Duplicate logbook entry for repeated tasks [9.4.6 (9.4.6-gab9f2a @ /home/gustavo/.emacs.d/elpa/org-9.4.6/)]
Bhavin Gandhi writes: > I was able to reproduce this, and here are my findings as well as a > reproducible configuration with only a few settings. The breakage was introduced in commit c67037: [c670379adfbdc4883d3cfa230289fd2829993265] Fix `org-agenda-todo' undo behavior when logging (not adding note) The fix is attached. Best, Ihor >From ff3c0f6524d4165518ec0c53b49a58162ff7b2a9 Mon Sep 17 00:00:00 2001 Message-Id: From: Ihor Radchenko Date: Sat, 10 Jul 2021 21:43:44 +0800 Subject: [PATCH] Fix duplicate logbook entry for repeated tasks * lisp/org.el (org-add-log-setup): Always run `org-add-log-note' via `post-command-hook'. Otherwise, there is no way to know if a note was requested for `this-command'. Running `org-add-log-note' directly would, for example, break `org-auto-repeat-maybe' as reported in [1]. [1] https://orgmode.org/list/CAOn=hbcaW1R6vtun-E2r4LS=j3dp=vjqmjgtzy8uc1sypar...@mail.gmail.com --- lisp/org.el | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index ffcc5945d..3d15771a2 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -10939,9 +10939,7 @@ (defun org-add-log-setup ( purpose state prev-state how extra) org-log-note-extra extra org-log-note-effective-time (org-current-effective-time) org-log-setup t) - (if (eq how 'note) - (add-hook 'post-command-hook 'org-add-log-note 'append) -(org-add-log-note purpose))) + (add-hook 'post-command-hook 'org-add-log-note 'append)) (defun org-skip-over-state-notes () "Skip past the list of State notes in an entry." -- 2.31.1
