Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[James Harkins]

Remember to cover the basics, that is, what you expected to happen and
what in fact did happen.  You don't know how to make a good report?  See

     http://orgmode.org/manual/Feedback.html#Feedback

Your bug report will be posted to the Org-mode mailing list.
------------------------------------------------------------------------

I'm resending the issue that I reported the other day, now with a MCE.

SHORT DESCRIPTION:

Recent versions of MobileOrg for Android allow the user to create new headings under specific nodes. This creates an "addheading" edit, which is processed by finding the target parent node and then calling org-insert-heading-respect-content to make a new heading at the same level as the given parent. (Then that heading is demoted, and the content is inserted.)

If the target heading is invisible at this time, org-insert-heading-respect-content looks upward to the target heading's parents, finding the closest one that is visible. Then it inserts the new heading at this level.

That is, given a tree like this:

* I
** A
*** 1
** B
* II

If we try to insert a new heading "2" under "A," we should get this (expected behavior):

* I
** A
*** 1
*** 2
** B
* II

Instead, we get:

* I
** A
*** 1
** B
** 2
* II

If the user doesn't discover the mistake, the subtree structure is corrupted and it may be hard to untangle.

Note: Several MobileOrg users have seen the same problem. This issue is not specific to my configuration.


STEPS TO REPRODUCE:

1. Create a file "test.org" with the following content:

* I
** A
*** 1
** B
* II


2. Cycle global visibility so that all top level headings are folded. You should see this in the buffer.

* I...
* II


3. From the scratch buffer, execute the following. The second expression extracts the bare minimum function calls from org-mobile-apply and org-mobile-edit to illustrate the issue. (I spent a lot of time with edebug, and I'm certain this is the actual sequence of events.)

(require 'org-mobile)

(org-with-point-at (org-mobile-locate-entry "olp:test.org:I/A")
  (progn
    (end-of-line 1)
    (org-insert-heading-respect-content)
    (org-demote)
    (insert "2. This should be a third-level heading, but it isn't")
  )
)

4. Go back to test.org and expand all headings.

* I
** A
*** 1
** B
** 2. This should be a third-level heading, but it isn't
* II


FURTHER DETAILS:

With edebug, I found that the problem within org-insert-heading is the use of org-end-of-subtree here:

          (when (featurep 'org-inlinetask)
        (while (and (not (eobp))
                (looking-at "\\(\\*+\\)[ \t]+")
                (>= (length (match-string 1))
                org-inlinetask-min-level))
          (org-end-of-subtree nil t)))

I tried changing this to (org-end-of-subtree t t) -- using the invisible-ok option -- but it didn't make a difference in the result.

Alternately, the problem might be:

	 (t
	  ;; somewhere in the line
          (save-excursion
	    (setq previous-pos (point-at-bol))
            (end-of-line)
            (setq hide-previous (outline-invisible-p)))
	  (and org-insert-heading-respect-content (org-show-subtree))

... where org-show-subtree does not actually show the desired parent node. I think org-end-of-subtree expects the proper parent to be visible at that point, but for some reason, that isn't actually happening.


Emacs  : GNU Emacs 23.3.1 (x86_64-pc-linux-gnu, GTK+ Version 2.24.10)
 of 2012-09-22 on batsu, modified by Debian
Package: Org-mode version 7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)

current state:
==============
(setq
 org-export-latex-after-initial-vars-hook '(org-beamer-after-initial-vars)
 org-speed-command-hook '(org-speed-command-default-hook org-babel-speed-command-hook)
 org-agenda-files '("~/Documents/mobileorg/semester.org" "~/Documents/mobileorg/agenda_main.org"
		    "~/Documents/mobileorg/usconcert.org")
 org-agenda-window-setup 'current-window
 org-hide-leading-stars t
 org-metaup-hook '(org-babel-load-in-session-maybe)
 org-after-todo-state-change-hook '(org-clock-out-if-current)
 org-mobile-files '(org-agenda-files "~/Documents/mobileorg/test.org")
 org-id-link-to-org-use-id 'create-if-interactive-and-no-custom-id
 org-export-latex-format-toc-function 'org-export-latex-format-toc-default
 org-mobile-inbox-for-pull "~/Documents/mobileorg/from-mobile.org"
 org-tab-first-hook '(org-hide-block-toggle-maybe org-src-native-tab-command-maybe
		      org-babel-hide-result-toggle-maybe org-babel-header-arg-expand)
 org-src-mode-hook '(org-src-babel-configure-edit-buffer org-src-mode-configure-edit-buffer)
 org-confirm-shell-link-function 'yes-or-no-p
 org-export-first-hook '(org-beamer-initialize-open-trackers)
 org-todo-keywords '((sequence "TODO" "MAYBE" "INPROG" "MTG" "|" "POSTPONED" "DONE"))
 org-agenda-before-write-hook '(org-agenda-add-entry-text)
 org-directory "~/Documents/mobileorg"
 org-blank-before-new-entry '((heading) (plain-list-item))
 org-url-hexify-p nil
 org-babel-pre-tangle-hook '(save-buffer)
 org-cycle-hook '(org-cycle-hide-archived-subtrees org-cycle-hide-drawers
		  org-cycle-hide-inline-tasks org-cycle-show-empty-lines
		  org-optimize-window-after-visibility-change)
 org-export-preprocess-before-normalizing-links-hook '(org-remove-file-link-modifiers)
 org-timeline-show-empty-dates nil
 org-mode-hook '((lambda nil
		  (org-add-hook (quote change-major-mode-hook) (quote org-show-block-all)
		   (quote append) (quote local))
		  )
		 (lambda nil
		  (org-add-hook (quote change-major-mode-hook) (quote org-babel-show-result-all)
		   (quote append) (quote local))
		  )
		 org-babel-result-hide-spec org-babel-hide-all-hashes)
 org-cycle-include-plain-lists 'integrate
 org-ctrl-c-ctrl-c-hook '(org-babel-hash-at-point org-babel-execute-safely-maybe)
 org-confirm-elisp-link-function 'yes-or-no-p
 org-clock-out-hook '(org-clock-remove-empty-clock-drawer)
 org-occur-hook '(org-first-headline-recenter)
 org-from-is-user-regexp "\\<James Harkins\\>"
 org-mobile-directory "/var/www/mobileorg/web"
 org-mobile-post-push-hook '((lambda nil (shell-command "chmod 664 /var/www/mobileorg/web/*")))
 org-export-preprocess-before-selecting-backend-code-hook '(org-beamer-select-beamer-code)
 org-agenda-cmp-user-defined 'bh/agenda-sort-by-heading-date
 org-modules '(org-bbdb org-bibtex org-docview org-gnus org-id org-info org-jsinfo org-irc org-mew
	       org-mhe org-rmail org-vm org-wl org-w3m)
 org-columns-default-format "%42ITEM %PRIORITY %14TIMESTAMP %14DEADLINE %TAGS"
 org-export-latex-final-hook '(org-beamer-amend-header org-beamer-fix-toc
			       org-beamer-auto-fragile-frames
			       org-beamer-place-default-actions-for-lists)
 org-metadown-hook '(org-babel-pop-to-session-maybe)
 )
--

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[Bastien]

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

Hi James,

James Harkins <jamshark70@gmail.com> writes:

> I'm resending the issue that I reported the other day, now with a
> MCE.

Sorry for the delay on this -- and thanks for the detailed reports.  

I tried not to get lost in the details actually... so I ended up using
the attached fix.  It works here, i.e. C-u C-RET inserts a new heading
at the right place, but I'm not using org-mobile.el so I'm not 100%
sure if it works for you.

Can you test and confirm?

Thanks!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-mobile.el-org-mobile-edit-DTRT-when-insert-a-hea.patch --]
[-- Type: text/x-patch, Size: 1967 bytes --]

From 808779ada5a35b69aca12e35723b22725aebf0f3 Mon Sep 17 00:00:00 2001
From: Bastien Guerry <bzg@altern.org>
Date: Mon, 11 Feb 2013 18:27:21 +0100
Subject: [PATCH] Fix `org-insert-heading-respect-content'

* org-mobile.el (org-mobile-edit): DTRT when insert a heading
 in an invisible region.

* org.el (org-insert-heading-respect-content): New
`invisible-ok' parameter.  Add docstring.
(org-insert-todo-heading-respect-content): Add docstring.

Thanks to James Harkins for the extra detailed reports and
the proposed solutions, both for org.el and org-mobile.el.
---
 lisp/org-mobile.el | 2 +-
 lisp/org.el        | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/org-mobile.el b/lisp/org-mobile.el
index a410de0..293d2a0 100644
--- a/lisp/org-mobile.el
+++ b/lisp/org-mobile.el
@@ -1064,7 +1064,7 @@ be returned that indicates what went wrong."
       (if (org-on-heading-p) ; if false we are in top-level of file
 	  (progn
 	    (end-of-line 1)
-	    (org-insert-heading-respect-content)
+	    (org-insert-heading-respect-content t)
 	    (org-demote))
 	(beginning-of-line)
 	(insert "* "))
diff --git a/lisp/org.el b/lisp/org.el
index 623c374..10168a5 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -7231,12 +7231,14 @@ This is a list with the following elements:
   (org-move-subtree-down)
   (end-of-line 1))
 
-(defun org-insert-heading-respect-content ()
-  (interactive)
+(defun org-insert-heading-respect-content (invisible-ok)
+  "Insert heading with `org-insert-heading-respect-content' set to t."
+  (interactive "P")
   (let ((org-insert-heading-respect-content t))
-    (org-insert-heading t)))
+    (org-insert-heading t invisible-ok)))
 
 (defun org-insert-todo-heading-respect-content (&optional force-state)
+  "Insert TODO heading with `org-insert-heading-respect-content' set to t."
   (interactive "P")
   (let ((org-insert-heading-respect-content t))
     (org-insert-todo-heading force-state t)))
-- 
1.8.1.2


[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


-- 
 Bastien

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[James Harkins]

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

On Feb 12, 2013 1:31 AM, "Bastien" <bzg@altern.org> wrote:
>
> Hi James,
>
> James Harkins <jamshark70@gmail.com> writes:
>
> > I'm resending the issue that I reported the other day, now with a
> > MCE.
>
> Sorry for the delay on this -- and thanks for the detailed reports.
>
> I tried not to get lost in the details actually... so I ended up using
> the attached fix.  It works here, i.e. C-u C-RET inserts a new heading
> at the right place, but I'm not using org-mobile.el so I'm not 100%
> sure if it works for you.
>
> Can you test and confirm?

I can, in about a week. I'm traveling, without my laptop (first time in
years I've left it at home - phone and tablet only for this trip).

I'm creating new entries in MobileOrg on the road. When I get home, I'll
apply the patch and see what happens.

One concern: When you tested with C-u C-RET, was the point on a hidden
headline? The problem only occurs if the current heading is folded up
underneath a parent heading. AFAIK cursor movement in org-mode ensures that
the point is never on invisible text, which is why I wrote a short lisp
function to demonstrate. It seems to me the issue reproduces only when
calling org-insert-heading non-interactively, then, so I wanted to check if
your test reproduces the problem without the patch.

hjh

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

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[Bastien]

Hi James,

James Harkins <jamshark70@gmail.com> writes:

> One concern: When you tested with C-u C-RET, was the point on a
> hidden headline? 

Yes.

> The problem only occurs if the current heading is
> folded up underneath a parent heading. AFAIK cursor movement in
> org-mode ensures that the point is never on invisible text, which is
> why I wrote a short lisp function to demonstrate. It seems to me the
> issue reproduces only when calling org-insert-heading
> non-interactively, then, so I wanted to check if your test reproduces
> the problem without the patch.

Looking forward reading your feedback on this!

-- 
 Bastien

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[Bastien]

Hi James,

James Harkins <jamshark70@gmail.com> writes:

> I can, in about a week. I'm traveling, without my laptop (first time
> in years I've left it at home - phone and tablet only for this
> trip).

FYI I applied the patch to the maint branch, so you can simply test it
from there.  Let me know, thanks,

-- 
 Bastien

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[James Harkins]

On Tue, Mar 5, 2013 at 1:41 AM, Bastien <bzg@gnu.org> wrote:
> There is some obscure issues here... I fixed various things in
> `org-insert-heading' in master, but inserting in invisible parts of
> the subtree is still unstable.  So I went and used the workaround you
> suggested (i.e. org-show-subtree) in the maint branch, so that it will
> be in 7.9.4.

Apologies -- this seems to be the issue that wouldn't die -- but I
have to report another failure with this.

I just did org-mobile-pull with the following entry:

~~
* F(edit:addheading) [[olp:semester.org:Dates][Dates]]
** Old value

** New value
Graduating exams
the modern music Important Notice:.....
** End of edit
~~

This means the new heading should be at the second level, underneath
Dates. Instead, it was added as the topmost *first* level heading:

~~ BEFORE
#+LAST_MOBILE_CHANGE: 2013-03-13 16:38:42

* Dates
** DONE Teachers' meeting
~~

~~ AFTER
* Graduating exams
the modern music Important Notice:....#+LAST_MOBILE_CHANGE: 2013-03-13 16:38:42
* Dates
** DONE Teachers' meeting
~~

~~ EXPECTED
#+LAST_MOBILE_CHANGE: 2013-03-13 16:38:42

* Dates
** DONE Teachers' meeting
** ... a few other level 2 headings ...
** Graduating exams
the modern music Important Notice:....
~~

Also, "#+LAST_MOBILE_CHANGE:" should be the first line in any org file
that's being synced to MobileOrg, but
(org-insert-heading-respect-content '(4) t) ignores this.

BTW, I believe the arg '(4) is incorrect. In MobileOrg (at least in
android -- actually, I think the iPhone doesn't support this feature
yet), new nodes go to the bottom of the parent's children. They should
not be inserted at the top in Emacs.

This is slightly different from the earlier bug. "* Dates" *is* a
top-level heading in the target file -- hence, it *can't* be
invisible. Nonetheless, when inserting the new heading, it seems to be
reading the level of the target node as level 0, rather than level 1.
After that, the new node goes at the top of level 1 (which will also
obliterate any #+ preamble the file might have).

This may be a recent issue. I updated via git after you said that you
had put my workaround in place, and I don't remember seeing this
problem before. New second-level nodes did go into the right places. I
saw this issue only today (git pull yesterday).

Thanks...
hjh

Re: Bug: org-insert-heading-respect-content inserts at the wrong level if target heading is invisible [7.9.2 (release_7.9.2-883-g6fb36e.dirty @ /home/dlm/share/org-mode.git/lisp/)]

[Bastien]

Hi James,

James Harkins <jamshark70@gmail.com> writes:

> On Tue, Mar 5, 2013 at 1:41 AM, Bastien <bzg@gnu.org> wrote:
>> There is some obscure issues here... I fixed various things in
>> `org-insert-heading' in master, but inserting in invisible parts of
>> the subtree is still unstable.  So I went and used the workaround you
>> suggested (i.e. org-show-subtree) in the maint branch, so that it will
>> be in 7.9.4.
>
> Apologies -- this seems to be the issue that wouldn't die -- but I
> have to report another failure with this.

Thanks for the detailed report -- I pushed a fix.

I used (org-insert-heading-respect-content '(16) t) so that new
headings are added at the end of the parent subtree.

Can you confirm it works fine for you?

Thanks,

-- 
 Bastien