[PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Alex Kosorukoff]

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

I noticed a regression in the capture functionality after upgrading org.
Capture fails with error in subj

Here is a simple config to reproduce the problem and a patch that fixes it.

emacs -q -l capfail.el

Best,
Alex

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

[-- Attachment #2: 0001-fix-org-capture-error-The-mark-is-not-set-now-so-the.patch --]
[-- Type: text/x-patch, Size: 959 bytes --]

From ac50a5300e35d7abd5f50317069b2a795fde4ad8 Mon Sep 17 00:00:00 2001
From: Alex Kosorukoff <alex@3form.com>
Date: Mon, 17 Mar 2014 12:56:09 -0700
Subject: [PATCH] fix org-capture error "The mark is not set now, so there is no region"

---
 lisp/org.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index dc4f2cc..bc5a69e 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14611,7 +14611,7 @@ When JUST-ALIGN is non-nil, only align tags.
 When JUST-ALIGN is 'ignore-column, align tags without trying to set
 the column by ignoring invisible text."
   (interactive "P")
-  (if (and (org-region-active-p) org-loop-over-headlines-in-active-region)
+  (if (and (mark t) (org-region-active-p) org-loop-over-headlines-in-active-region)
       (let ((cl (if (eq org-loop-over-headlines-in-active-region 'start-level)
 		    'region-start-level 'region))
 	    org-loop-over-headlines-in-active-region)
-- 
1.7.0.4


[-- Attachment #3: capfail.el --]
[-- Type: text/x-emacs-lisp, Size: 605 bytes --]

;; capfail.el org-mode capture failure when region is active
;; $ emacs -q -l capfail.el

(setq inhibit-splash-screen t)
(add-to-list 'load-path "~/.emacs.d/org/lisp")
(require 'org)

(setq org-capture-templates
      '(("t" "Todo" entry (file "test.org")
         "* TODO Test %^g\n  %?")))

(define-key global-map (kbd "C-c c") 'org-capture)

(find-file "test.org")
(insert
 "Select some text to make a region, then try C-c c t\ntest\n"
 "Emacs 23.1.1/23.3.1/24.1/24.2 & Org-mode version 8.2.6 result:\n"
 "Capture abort: (error: The mark is not set now, so there is no region)\n")

(provide 'capfail)

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Bastien]

Hi Alex,

Alex Kosorukoff <alex@3form.com> writes:

> I noticed a regression in the capture functionality after upgrading
> org. Capture fails with error in subj

fixed, thanks,

-- 
 Bastien

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Alex Kosorukoff]

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

After I replaced my patch and merged Bastien's fix, I started seeing the
error though less frequently than before. It didn't occur in the template I
posted, but I started seeing it again in another template.

("w" "org-protocol tag" entry (file "~/org/bookmarks.org")
               "* %:description %(org-set-tags)\n  %i\n\n  %:link\n%?"
               :prepend t :empty-lines-after 1 :clock-in t :clock-resume t)

I switched back to my initial patch that was checking if the mark was set
before trying to access the region. This worked: the errors disappeared.
However, I couldn't understand why mark would be unset in the first place.
It turns out that this is a result of the function
org-capture-steal-local-variables that is copying mark-active variable from
another buffer. This results in an inconsistent state where mark is not set
and yet the variable mark-active is set. Emacs functions region-active-p
and use-region-p expect the state to be consistent and fail with this error
that they won't produce under normal circumstances. Here is the minimal
patch that fixes the root cause of this problem:

From 3d84403964dec1ac55810883e4e8a812c3ff94fc Mon Sep 17 00:00:00 2001
From: Alex Kosorukoff <alex@3form.com>
Date: Thu, 8 May 2014 20:27:59 -0700
Subject: [PATCH] org-capture: better fix for error "The mark is not set
now, ..."

---
 lisp/org-capture.el |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index d57b9e0..3c62b1d 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1590,6 +1590,7 @@ The template may still contain \"%?\" for cursor
positioning."
       (goto-char (point-min))
       (org-capture-steal-local-variables buffer)
       (setq buffer-file-name nil)
+      (setq mark-active nil)

       ;; %[] Insert contents of a file.
       (goto-char (point-min))
-- 
1.7.0.4





On Tue, Apr 29, 2014 at 4:05 AM, Bastien <bzg@gnu.org> wrote:

> Hi Alex,
>
> Alex Kosorukoff <alex@3form.com> writes:
>
> > I noticed a regression in the capture functionality after upgrading
> > org. Capture fails with error in subj
>
> fixed, thanks,
>
> --
>  Bastien
>

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

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Bastien]

Hi Alex,

Alex Kosorukoff <alex@3form.com> writes:

> After I replaced my patch and merged Bastien's fix, I started seeing
> the error though less frequently than before. It didn't occur in the
> template I posted, but I started seeing it again in another template.
>
> ("w" "org-protocol tag" entry (file "~/org/bookmarks.org")
>                "* %:description %(org-set-tags)\n  %i\n\n  %:link\n%?
> "
>                :prepend t :empty-lines-after 1 :clock-in t
> :clock-resume t)

If I may ask, why using %(org-set-tags) instead of %^g or %^G?

> I switched back to my initial patch that was checking if the mark was
> set before trying to access the region. This worked: the errors
> disappeared.

I think the right fix is to exclude `mark-active' from the local
variable that are imported through `org-capture-steal-local-variables'.
I installed such a fix in maint, please update Org and let me know if
this works for you.

Thanks,

-- 
 Bastien

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Alex Kosorukoff]

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

On Fri, May 23, 2014 at 5:03 AM, Bastien <bzg@gnu.org> wrote:

> Hi Alex,
>
> Alex Kosorukoff <alex@3form.com> writes:
>
> > After I replaced my patch and merged Bastien's fix, I started seeing
> > the error though less frequently than before. It didn't occur in the
> > template I posted, but I started seeing it again in another template.
> >
> > ("w" "org-protocol tag" entry (file "~/org/bookmarks.org")
> >                "* %:description %(org-set-tags)\n  %i\n\n  %:link\n%?
> > "
> >                :prepend t :empty-lines-after 1 :clock-in t
> > :clock-resume t)
>
> If I may ask, why using %(org-set-tags) instead of %^g or %^G?
>

I am using org-set-tags to avoid autocompletion, both %^g and %^G take too
much time because my org files have many tagged items. Capture should be
fast to be effective.

> I switched back to my initial patch that was checking if the mark was
> > set before trying to access the region. This worked: the errors
> > disappeared.
>
> I think the right fix is to exclude `mark-active' from the local
> variable that are imported through `org-capture-steal-local-variables'.
> I installed such a fix in maint, please update Org and let me know if
> this works for you.
>

Excluding mark-active will work, the result will be the same as after my
patch, except performance will not be the same. Excluding variable requires
filtering the list of variables which takes O(n) whereas my patch takes
O(1). Mark-active is nil before org-capture-steal-local-variables because
this is a new buffer. It seems in this case setting it back to nil is
faster than trying to preserve its original value nil.


> Thanks,
>
> --
>  Bastien
>

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

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Bastien]

Hi Alex,

Alex Kosorukoff <alex@3form.com> writes:

> Excluding mark-active will work, the result will be the same as after
> my patch, except performance will not be the same. Excluding variable
> requires filtering the list of variables which takes O(n) whereas my
> patch takes O(1). Mark-active is nil before
> org-capture-steal-local-variables because this is a new buffer. It
> seems in this case setting it back to nil is faster than trying to
> preserve its original value nil.

I see what you mean but there is no performance issue here and not
copying the value of mark-active is cleaner than setting it back to
nil -- we never want to copy the value of the mark at all.

-- 
 Bastien

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Alex Kosorukoff]

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

In fact, there is some performance issue. The steal function copies a lot
of variables as I can tell. Do you know where those variables are used? I
replaced the steal function with an advice like this

(defadvice org-capture-steal-local-variables (around do-not-steal activate))

My capture became very fast after that and I didn't notice any adverse
effects so far (using this for more than a week). The only reason I didn't
propose a patch like this is that I am still testing it for possible
regressions.



On Fri, May 23, 2014 at 10:28 AM, Bastien <bzg@gnu.org> wrote:

> Hi Alex,
>
> Alex Kosorukoff <alex@3form.com> writes:
>
> > Excluding mark-active will work, the result will be the same as after
> > my patch, except performance will not be the same. Excluding variable
> > requires filtering the list of variables which takes O(n) whereas my
> > patch takes O(1). Mark-active is nil before
> > org-capture-steal-local-variables because this is a new buffer. It
> > seems in this case setting it back to nil is faster than trying to
> > preserve its original value nil.
>
> I see what you mean but there is no performance issue here and not
> copying the value of mark-active is cleaner than setting it back to
> nil -- we never want to copy the value of the mark at all.
>
> --
>  Bastien
>

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

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Bastien]

Alex Kosorukoff <alex@3form.com> writes:

> In fact, there is some performance issue.

This is what I observe (using elp-instrument-function on
`org-capture-steal-local-variables' and calling org-capture
10 times):

A: org-capture-steal-local-variables  10  0.005087601   0.0005087601
B: org-capture-steal-local-variables  10  0.0028751399  0.0002875139

A difference of 0.2 ms is not really perceivable.

Do you get difference results?

-- 
 Bastien

Re: [PATCH] Fix: Capture abort: (error: The mark is not set now, so there is no region)

[Bastien]

Bastien <bzg@gnu.org> writes:

> Do you get difference results?

You're right after all.  Since `org-capture-steal-local-variables'
isn't used anywhere else and since `org-capture' already set
`buffer-file-name' "manuall", I applied your change.

Thanks!

-- 
 Bastien