Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Jan Seeger]

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

Greetings!

I've discovered that org-capture-refile's documentation states that
org-capture-refile finalizes the buffer first, and then refiles the
entry. This is incorrect, the implementation of org-capture-refile
first refiles the entry and then calls org-capture-finalize.

While this is a small inconsistency, it leads to errors with
org-finalize: The cursor possition during finalization is that of the
entry before the refile, while normally, the cursor is positioned at
the start of the newly inserted entry when org-capture-finalize is
called. This leads to superfluous newlines, and possibly other
problems, such as timestamps and checkboxes not being updated.

I've fixed this bug for me by reversing the order of finalization and
refiling, as stated in the documentation. I've attached a patch to do
this, but I'm not entirely sure about the side effects caused by the
patch. The test suite does pass though. If someone could weigh in on
this, I'd be grateful!

Regards,
Jan


Emacs  : GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.16.6)
 of 2015-09-22 on bean
Package: Org-mode version 8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)



[-- Attachment #2: org-capture-refile.patch --]
[-- Type: text/plain, Size: 710 bytes --]

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 67dc319..d60929d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -793,14 +793,15 @@ already gone.  Any prefix argument will be passed to the refile command."
   (let ((pos (point))
 	(base (buffer-base-buffer (current-buffer)))
 	(org-refile-for-capture t))
+    (org-capture-finalize))
     (save-window-excursion
       (with-current-buffer (or base (current-buffer))
 	(save-excursion
 	  (save-restriction
 	    (widen)
 	    (goto-char pos)
-	    (call-interactively 'org-refile)))))
-    (org-capture-finalize)))
+	    (call-interactively 'org-refile))))))
+
 
 (defun org-capture-kill ()
   "Abort the current capture process."

Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Nicolas Goaziou]

Hello,

Jan Seeger <jan.seeger@thenybble.de> writes:

> I've fixed this bug for me by reversing the order of finalization and
> refiling, as stated in the documentation. I've attached a patch to do
> this, but I'm not entirely sure about the side effects caused by the
> patch. The test suite does pass though. If someone could weigh in on
> this, I'd be grateful!

Thank you for the patch. However, it contradicts
47b0eec8dc39990731f400640d64ab70a8356ccd.

See thread starting
<http://permalink.gmane.org/gmane.emacs.orgmode/78027> for an
explanation.


Regards,

-- 
Nicolas Goaziou

OT - gmane problems? [was: Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]]

[Nick Dokos]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Jan Seeger <jan.seeger@thenybble.de> writes:
>
>> I've fixed this bug for me by reversing the order of finalization and
>> refiling, as stated in the documentation. I've attached a patch to do
>> this, but I'm not entirely sure about the side effects caused by the
>> patch. The test suite does pass though. If someone could weigh in on
>> this, I'd be grateful!
>
> Thank you for the patch. However, it contradicts
> 47b0eec8dc39990731f400640d64ab70a8356ccd.
>
> See thread starting
> <http://permalink.gmane.org/gmane.emacs.orgmode/78027> for an
> explanation.
>

Anybody else having trouble getting to gmane? I'm getting

Couldn't contact ciao:8010
The web interface is down for maintenance.

-- 
Nick

Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Jan Seeger]

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

Hello! 
Thanks for your reply. However, the current behavior definitely results in bugs when refiling from the capture buffer (newlines don't get deleted properly, for example). 

I'll look into how to correctly set the position for org-capture-finalize after the refile then, and try to make a patch for that.
Regards,
Jan

Am 24. September 2015 22:54:38 MESZ, schrieb Nicolas Goaziou <mail@nicolasgoaziou.fr>:
>Hello,
>
>Jan Seeger <jan.seeger@thenybble.de> writes:
>
>> I've fixed this bug for me by reversing the order of finalization and
>> refiling, as stated in the documentation. I've attached a patch to do
>> this, but I'm not entirely sure about the side effects caused by the
>> patch. The test suite does pass though. If someone could weigh in on
>> this, I'd be grateful!
>
>Thank you for the patch. However, it contradicts
>47b0eec8dc39990731f400640d64ab70a8356ccd.
>
>See thread starting
><http://permalink.gmane.org/gmane.emacs.orgmode/78027> for an
>explanation.
>
>
>Regards,
>
>-- 
>Nicolas Goaziou

-- 
Diese Nachricht wurde von meinem Mobiltelefon gesendet.

[-- Attachment #2: Type: text/html, Size: 1624 bytes --]

Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Nicolas Goaziou]

Hello,

Jan Seeger <jan.seeger@thenybble.de> writes:

> Thanks for your reply. However, the current behavior definitely
> results in bugs when refiling from the capture buffer (newlines don't
> get deleted properly, for example).

I agree. I'm just pointing out we shouldn't step back and re-introduce
a fixed issue.

> I'll look into how to correctly set the position for
> org-capture-finalize after the refile then, and try to make a patch
> for that.

Thank you.


Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Jan Seeger]

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

Hello!

I've reworked the patch to restore the original order of
org-capture-finalize and org-capture-refile, but working around the
interaction with :kill-buffer that required reversing the order in the
first place.

org-capture-finalize requires the capture buffer to be set up
correctly, and restoring this correctly after refiling appears to
require a lot of work.

Regards,
Jan


[-- Attachment #2: org-capture-refile.patch --]
[-- Type: text/plain, Size: 988 bytes --]

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 67dc319..3d5df83 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -792,7 +792,10 @@ already gone.  Any prefix argument will be passed to the refile command."
      "Refiling from a capture buffer makes only sense for `entry'-type templates"))
   (let ((pos (point))
 	(base (buffer-base-buffer (current-buffer)))
-	(org-refile-for-capture t))
+	(org-refile-for-capture t)
+	(kill-buffer (org-capture-get :kill-buffer 'local)))
+    (org-capture-put :kill-buffer nil)
+    (org-capture-finalize)
     (save-window-excursion
       (with-current-buffer (or base (current-buffer))
 	(save-excursion
@@ -800,7 +803,8 @@ already gone.  Any prefix argument will be passed to the refile command."
 	    (widen)
 	    (goto-char pos)
 	    (call-interactively 'org-refile)))))
-    (org-capture-finalize)))
+    (if kill-buffer
+	(kill-buffer base))))
 
 (defun org-capture-kill ()
   "Abort the current capture process."

Re: Bug: org-capture-refile doc inconsistency, refile bug [8.3.1 (8.3.1-129-ga7c8d2-elpaplus @ /home/jeeger/.emacs.d/elpa/org-plus-contrib-20150921/)]

[Nicolas Goaziou]

Hello,

Jan Seeger <jan.seeger@thenybble.de> writes:

> I've reworked the patch to restore the original order of
> org-capture-finalize and org-capture-refile, but working around the
> interaction with :kill-buffer that required reversing the order in the
> first place.
>
> org-capture-finalize requires the capture buffer to be set up
> correctly, and restoring this correctly after refiling appears to
> require a lot of work.

Thank you. 

Could you provide an appropriate commit message and send it with git
format-patch? You may need to add TINYCHANGE at the end of the message.

> +    (if kill-buffer
> +	(kill-buffer base))))

(when kill-buffer (kill-buffer base))


Regards,

-- 
Nicolas Goaziou

[PATCH] Reverse capture finalize and refile, hack buffer killing.

[Jan Seeger]

* org-capture.el (org-capture-refile): Call finalize first, then
  refile. If the capture buffer needs to be killed afterwards, kill
  after refiling.

Refiling before finalizing led to an incorrect cursor position during
refile. Since adjusting the buffer position and bounds for the refiled
entry is difficult, I reverted the org-capture-refile function to its
original order of finalizing and then refiling. However, this leads to a
bug with killing the buffer on capture, described in
http://permalink.gmane.org/gmane.emacs.orgmode/78027/. To work around
this, now the :kill-buffer flag is cleared before calling finalize, and
the capture buffer is killed after org-refile is called.

TINYCHANGE
---
 lisp/org-capture.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 67dc319..b1cda3d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -792,7 +792,10 @@ already gone.  Any prefix argument will be passed to the refile command."
      "Refiling from a capture buffer makes only sense for `entry'-type templates"))
   (let ((pos (point))
        (base (buffer-base-buffer (current-buffer)))
-       (org-refile-for-capture t))
+       (org-refile-for-capture t)
+       (kill-buffer (org-capture-get :kill-buffer 'local)))
+    (org-capture-put :kill-buffer nil)
+    (org-capture-finalize)
     (save-window-excursion
       (with-current-buffer (or base (current-buffer))
        (save-excursion
@@ -800,7 +803,7 @@ already gone.  Any prefix argument will be passed to the refile command."
            (widen)
            (goto-char pos)
            (call-interactively 'org-refile)))))
-    (org-capture-finalize)))
+    (when kill-buffer (kill-buffer base))))
 
 (defun org-capture-kill ()
   "Abort the current capture process."
-- 
2.4.9

Re: [PATCH] Reverse capture finalize and refile, hack buffer killing.

[Nicolas Goaziou]

Hello,

Jan Seeger <jan.seeger@thenybble.de> writes:

> * org-capture.el (org-capture-refile): Call finalize first, then
>   refile. If the capture buffer needs to be killed afterwards, kill
>   after refiling.

Thank you.

For some reason, I cannot apply it on maint branch. Could you send it
again as an attachment ?

Thank you.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Reverse capture finalize and refile, hack buffer killing.

[Jan Seeger]

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

Hello!

That might be because I copy-pasted the patch into an email. Sorry for the delay!

I attached the patch.

Regards,
Jan


[-- Attachment #2: org-refile-fixes.patch --]
[-- Type: text/plain, Size: 988 bytes --]

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 67dc319..b1cda3d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -792,7 +792,10 @@ already gone.  Any prefix argument will be passed to the refile command."
      "Refiling from a capture buffer makes only sense for `entry'-type templates"))
   (let ((pos (point))
 	(base (buffer-base-buffer (current-buffer)))
-	(org-refile-for-capture t))
+	(org-refile-for-capture t)
+	(kill-buffer (org-capture-get :kill-buffer 'local)))
+    (org-capture-put :kill-buffer nil)
+    (org-capture-finalize)
     (save-window-excursion
       (with-current-buffer (or base (current-buffer))
 	(save-excursion
@@ -800,7 +803,7 @@ already gone.  Any prefix argument will be passed to the refile command."
 	    (widen)
 	    (goto-char pos)
 	    (call-interactively 'org-refile)))))
-    (org-capture-finalize)))
+    (when kill-buffer (kill-buffer base))))
 
 (defun org-capture-kill ()
   "Abort the current capture process."

Re: [PATCH] Reverse capture finalize and refile, hack buffer killing.

[Nicolas Goaziou]

Jan Seeger <jan.seeger@thenybble.de> writes:

> That might be because I copy-pasted the patch into an email. Sorry for the delay!
>
> I attached the patch.

Thank you. However, you forgot the commit message. Could you send it
again using git format-patch?


Regards,

Re: [PATCH] Reverse capture finalize and refile, hack buffer killing.

[Jan Seeger]

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

Hello!

Attached is the full output of git-format patch (should have done it
like that earlier^^).


[-- Attachment #2: 0001-Reverse-capture-finalize-and-refile-hack-buffer-kill.patch --]
[-- Type: application/octet-stream, Size: 2014 bytes --]

From c5a5080ec739658095446c30004cfb7b6f1ca0d6 Mon Sep 17 00:00:00 2001
From: Jan Seeger <jan.seeger@thenybble.de>
Date: Fri, 25 Sep 2015 14:54:31 +0200
Subject: [PATCH] Reverse capture finalize and refile, hack buffer killing.

* org-capture.el (org-capture-refile): Call finalize first, then
  refile. If the capture buffer needs to be killed afterwards, kill
  after refiling.

Refiling before finalizing led to an incorrect cursor position during
refile. Since adjusting the buffer position and bounds for the refiled
entry is difficult, I reverted the org-capture-refile function to its
original order of finalizing and then refiling. However, this leads to a
bug with killing the buffer on capture, described in
http://permalink.gmane.org/gmane.emacs.orgmode/78027/. To work around
this, now the :kill-buffer flag is cleared before calling finalize, and
the capture buffer is killed after org-refile is called.

TINYCHANGE
---
 lisp/org-capture.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 67dc319..b1cda3d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -792,7 +792,10 @@ already gone.  Any prefix argument will be passed to the refile command."
      "Refiling from a capture buffer makes only sense for `entry'-type templates"))
   (let ((pos (point))
 	(base (buffer-base-buffer (current-buffer)))
-	(org-refile-for-capture t))
+	(org-refile-for-capture t)
+	(kill-buffer (org-capture-get :kill-buffer 'local)))
+    (org-capture-put :kill-buffer nil)
+    (org-capture-finalize)
     (save-window-excursion
       (with-current-buffer (or base (current-buffer))
 	(save-excursion
@@ -800,7 +803,7 @@ already gone.  Any prefix argument will be passed to the refile command."
 	    (widen)
 	    (goto-char pos)
 	    (call-interactively 'org-refile)))))
-    (org-capture-finalize)))
+    (when kill-buffer (kill-buffer base))))
 
 (defun org-capture-kill ()
   "Abort the current capture process."
-- 
2.4.9

Re: [PATCH] Reverse capture finalize and refile, hack buffer killing.

[Nicolas Goaziou]

Hello,

Jan Seeger <jan.seeger@thenybble.de> writes:

> Attached is the full output of git-format patch (should have done it
> like that earlier^^).

Applied, finally! Thank you.

Regards,

-- 
Nicolas Goaziou