[BUG] Changing TODO states sometimes modifies the scheduling of the next heading

[BUG] Changing TODO states sometimes modifies the scheduling of the next heading

[Tom]

I have a heading where the scheduling changed misteriuosly. It is
set to repeat weekly and I found it jumped to the next week
without me touching it at all. I set up a background watcher and
after a week it detected when the unwanted change happens.

I could not yet create a simple org file to reproduce the
problem, but I found the problematic part in the code. It happens
when I change the TODO state of a heading with repeating
scheduling. When org modifies the scheduling of the heading
according to the repeat period it also modifies the scheduling of
the heading under it.

Here's the buggy part of the code:


(defun org-auto-repeat-maybe (done-word)
...
    (while (re-search-forward
              re (save-excursion (outline-next-heading) (point)) t)


http://repo.or.cz/w/org-mode.git/blob/HEAD:/lisp/org.el#l11546


The problem is the bound for the search is determined within the
while loop. The problem occurs when the scheduling of the heading
is changed and cursor is put on the beginning of the next line
after that. If that next line happens to be the next heading line
then the (outline-next-heading) call in the re-search-forward
skips over the next heading, setting the bound of the search to
the end of it, so the search includes the contents of the next
heading too, so its scheduling is also modified behind the user's
back.

The solution is simple: determine the bound of the search (the
end of the heading) before the while loop and use that value in
re-search-forward, instead of computing it within the loop.

Re: [BUG] Changing TODO states sometimes modifies the scheduling of the next heading

[Tom]

Tom <adatgyujto <at> gmail.com> writes:
> 
> I could not yet create a simple org file to reproduce the
> problem


Here's an org file which demonstrates the bug with org 7.5:

    * TODO test1
    SCHEDULED: <2011-04-02 Szo +1d>
    * TODO test2 
    SCHEDULED: <2011-04-03 H .+1w>


If you (setq org-log-repeat nil)  and press C-c C-t on test1
then the scheduling of test2 is modified too.

Re: [BUG] Changing TODO states sometimes modifies the scheduling of the next heading

[Matt Lundin]

Tom <adatgyujto@gmail.com> writes:

> Tom <adatgyujto <at> gmail.com> writes:
>> 
>> I could not yet create a simple org file to reproduce the
>> problem
>
>
> Here's an org file which demonstrates the bug with org 7.5:
>
>     * TODO test1
>     SCHEDULED: <2011-04-02 Szo +1d>
>     * TODO test2 
>     SCHEDULED: <2011-04-03 H .+1w>
>
>
> If you (setq org-log-repeat nil)  and press C-c C-t on test1
> then the scheduling of test2 is modified too.

I cannot reproduce this. 

--8<---------------cut here---------------start------------->8---
* TODO test1
  SCHEDULED: <2011-04-02 Sat +1d>
* TODO test2 
  SCHEDULED: <2011-04-03 Sun .+1w>
--8<---------------cut here---------------end--------------->8---

When I set org-log-repeat to nil and mark the first task DONE, I get the
following results:

--8<---------------cut here---------------start------------->8---
* TODO test1
  SCHEDULED: <2011-04-03 Sun +1d>
* TODO test2 
  SCHEDULED: <2011-04-03 Sun .+1w>
--8<---------------cut here---------------end--------------->8---

Best,
Matt

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Tom]

Matt Lundin <mdl <at> imapmail.org> writes:
> 
> I cannot reproduce this. 
> 

Did you start emacs without any initialization? I started it with
-Q and loaded org 7.5 manually to avoid affecting the test with 
my own org customizations.

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Matt Lundin]

Tom <adatgyujto@gmail.com> writes:

> Matt Lundin <mdl <at> imapmail.org> writes:
>> 
>> I cannot reproduce this. 
>> 
>
> Did you start emacs without any initialization? I started it with
> -Q and loaded org 7.5 manually to avoid affecting the test with 
> my own org customizations.

I still cannot reproduce it.

Steps followed: 

1. /usr/bin/emacs -Q

2. (setq org-log-repeat nil)

3. (add-to-list 'load-path "~/org-mode/lisp/")

4. M-x org-reload

5. M-x org-version => Org-mode version 7.5 (release_7.5.144.g174e)

6. C-c C-t on first headline.

Results:

--8<---------------cut here---------------start------------->8---
* TODO test1
  SCHEDULED: <2011-04-03 Sun +1d>
* TODO test2 
  SCHEDULED: <2011-04-03 Sun .+1w>
--8<---------------cut here---------------end--------------->8---

Best,
Matt

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Tom]

Matt Lundin <mdl <at> imapmail.org> writes:

> 
> I still cannot reproduce it.
>

I tried your test file which was different from the test file
I suggested in the second mail and I couldn't reproduce the problem
with it either.

So I looked into this and turns out the problem occurs only if
after the state change the new day abbreviation in the timestamp
is shorter than the previous one.

In your case Sat switches to Sun, both 3 characters length.
No problem.

But in my case Szo changes to V which is two characters shorter:


--8<---------------cut here---------------start------------->8---
* TODO test1
SCHEDULED: <2011-04-02 Szo +1d>
* TODO test2 
SCHEDULED: <2011-04-03 V .+1w>
--8<---------------cut here---------------end--------------->8---


Why is it a problem?

Because org-timestamp-change starts with storing the current
cursor position which is the end of the timestamp:

15400 (defun org-timestamp-change (n &optional what updown)
15401   "Change the date in the time stamp at point.
15402 The date will be changed by N times WHAT.  WHAT can be `day', `month',
15403 `year', `minute', `second'.  If WHAT is not given, the cursor position
15404 in the timestamp determines what will be changed."
15405   (let ((pos (point))
...

http://repo.or.cz/w/org-mode.git/blob/HEAD:/lisp/org.el#l15405



and later it simply restores the position with:

15468       (goto-char pos)

http://repo.or.cz/w/org-mode.git/blob/HEAD:/lisp/org.el#l15468


The problem is in my case the new day abbreviation is two char
shorter, so the whole line is shorter, therefore the goto-char
puts the cursor in the next line (instead of at the end of the
timestamp) which triggers the bound problem I described in the
first mail:

http://article.gmane.org/gmane.emacs.orgmode/40502



Bottom line: the problem does not occur in the English locale,
because there all day abbreviations are 3 chars long, so the
above described simple way of restoring the cursor position
always works. But this is not true for all locales, so org
shouldn't rely on that.

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Tom]

Tom <adatgyujto <at> gmail.com> writes:
> 
> Bottom line: the problem does not occur in the English locale,
> because there all day abbreviations are 3 chars long, so the
> above described simple way of restoring the cursor position
> always works. But this is not true for all locales, so org
> shouldn't rely on that.
> 
> 


I created a temporary fix for the problem with advice until it
is fixed in the code properly.


Here it is if someone needs it:


(defadvice org-todo (around my-org-todo activate)
  (save-restriction
    (narrow-to-region (save-excursion (org-back-to-heading t) (point))
                      (save-excursion (outline-next-heading) (point)))
    ad-do-it))

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Nicolas Goaziou]

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

Hello,

Tom <adatgyujto@gmail.com> writes:

> Bottom line: the problem does not occur in the English locale,
> because there all day abbreviations are 3 chars long, so the
> above described simple way of restoring the cursor position
> always works. But this is not true for all locales, so org
> shouldn't rely on that.

Would you mind telling me if the following patch fixes your problem?
If so, I'll apply it to the code base.

Regards,

-- 
Nicolas Goaziou

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-bug-with-TODO-states-changes-modifying-schedulin.patch --]
[-- Type: text/x-patch, Size: 1438 bytes --]

From 6ab4222325f304d89bb161085956bc3c2d1d7617 Mon Sep 17 00:00:00 2001
From: Nicolas Goaziou <n.goaziou@gmail.com>
Date: Wed, 20 Jul 2011 10:18:31 +0200
Subject: [PATCH] Fix bug with TODO states changes modifying scheduling of
 next headline

* lisp/org.el (org-timestamp-change): some locales don't use the same
  length for date abbreviations. Set a marker at origin in case length
  of new timestamp is different.

Thanks to Tom for analyzing this.
---
 lisp/org.el |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index fee13b7..c08ab75 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15567,7 +15567,7 @@ With prefix ARG, change that many days."
 The date will be changed by N times WHAT.  WHAT can be `day', `month',
 `year', `minute', `second'.  If WHAT is not given, the cursor position
 in the timestamp determines what will be changed."
-  (let ((pos (point))
+  (let ((pos (copy-marker (point)))
 	with-hm inactive
 	(dm (max (nth 1 org-time-stamp-rounding-minutes) 1))
 	org-ts-what
@@ -15631,6 +15631,7 @@ in the timestamp determines what will be changed."
 	    (org-insert-time-stamp time with-hm inactive nil nil extra))
       (org-clock-update-time-maybe)
       (goto-char pos)
+      (move-marker pos nil)
       ;; Try to recenter the calendar window, if any
       (if (and org-calendar-follow-timestamp-change
 	       (get-buffer-window "*Calendar*" t)
-- 
1.7.6

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Tom]

Nicolas Goaziou <n.goaziou <at> gmail.com> writes:
 
> Would you mind telling me if the following patch fixes your problem?
> If so, I'll apply it to the code base.
> 

Yes, it seems to work with the test file I mentioned previously in
the thread.

I'm a bit suprised it took so long until someone fixed the problem
though it was a major bug (changing scheduling of items unintentionally),
the problem was well analyzed and the solution was simple.

Anyway, thanks for the fix. Now I can remove my defadvice workaround
from my setup as it apparently is no longer needed.

Re: [BUG] Changing TODO states sometimes modifies the schedulingof the next heading

[Bastien]

Hi Tom,

Tom <adatgyujto@gmail.com> writes:

> I'm a bit suprised it took so long until someone fixed the problem
> though it was a major bug (changing scheduling of items unintentionally),
> the problem was well analyzed and the solution was simple.

Apparently not.  

And the problem only hit people using locales other than english, 
maybe that explains why there was not that many complaints.

Anyway, we will provide a solution for this.

Thanks for your patience,

-- 
 Bastien