Bug: org-map-tree behaves strangely if first heading before point is hidden [8.3.5 (8.3.5-elpa @ /home/tom/.emacs.d/elpa/org-20160725/)]

Bug: org-map-tree behaves strangely if first heading before point is hidden [8.3.5 (8.3.5-elpa @ /home/tom/.emacs.d/elpa/org-20160725/)]

[talwrii talwrii]

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

Org mode file complete with code to reproduce the issue is attached.

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

[-- Attachment #2: map-tree-repro.org --]
[-- Type: application/octet-stream, Size: 4281 bytes --]

* Folded
** Hidden
*** Children
*** More children

** Other child


* Bug description
When calling org-map-tree on a hidden tree, the results are different from
those if the tree were visible: this is not as expected.

* Repro
The following code samples consistently reproduce this behaviour.
Run then with `C-c C-c` when the cursor is over the code block.

The first folds up the first tree in this document and then calls `org-map-tree` with the point at "Hidden";
the second does the same but expands the first tree.

I consider the case with the expanded tree to retrun the correct result.
** Folded
#+begin_src emacs-lisp :results raw
(save-excursion
; Expand first tree
(goto-char 0)
(while (not (equal org-cycle-subtree-status 'folded))
  (org-cycle))

; Run map over `Hidden`
(goto-char 10)
(setq accum nil)
(org-map-tree (lambda () (add-to-list 'accum (point))))
accum
)

#+end_src
#+RESULTS:
(52 33 20 10 1)
** Expanded
#+begin_src emacs-lisp :results raw
(save-excursion 

(goto-char 0)
(setq accum nil)

(while (not (equal org-cycle-subtree-status 'children))
  (message (format "%S" org-cycle-subtree-status))
  (org-cycle))

(goto-char 10)

(org-map-tree (lambda () (add-to-list 'accum (point))))
accum
)
#+end_src

#+RESULTS:
(33 20 10)

* Likely cause and fix

Looking at `org-map-tree`:

(defun org-map-tree (fun)
  "Call FUN for every heading underneath the current one."
  (org-back-to-heading)
  (message (format "org-map-tree start %S" (point)))
  (let ((level (funcall outline-level)))
    (save-excursion
      (funcall fun)
      (while (and (progn
                    (prog1
                      (outline-next-heading)
                      (message (format "org-map-tree outline-ext-point %S" (point)))
                      )
		    (> (funcall outline-level) level))
		  (not (eobp)))
	(funcall fun)))))

We see that it calls `org-back-to-heading`; this skips back to the first visible ancestor heading.

`org-back-to-heading` has the optional argument `invisible-ok` which causes it to skip back to the first
ancestor heading, whether it is visible or not.

(defun org-back-to-heading (&optional invisible-ok)
  "Call `outline-back-to-heading', but provide a better error message."
  (condition-case nil
      (outline-back-to-heading invisible-ok)
    (error (error "Before first headline at position %d in buffer %s"
		  (point) (current-buffer)))))

I think that `org-map-subtree` should call `org-back-to-heading` with the `invisible-ok` argument set to `'t` like so:

(defun fixed-org-map-tree (fun)
  "Call FUN for every heading underneath the current one."
  (org-back-to-heading 't)
  (message (format "org-map-tree start %S" (point)))
  (let ((level (funcall outline-level)))
    (save-excursion
      (funcall fun)
      (while (and (progn
                    (prog1
                      (outline-next-heading)
                      (message (format "org-map-tree outline-ext-point %S" (point)))
                      )
		    (> (funcall outline-level) level))
		  (not (eobp)))
	(funcall fun)))))

We can confirm that this gives us the expected results

#+begin_src emacs-lisp :results raw
(defun fixed-org-map-tree (fun)
  "Call FUN for every heading underneath the current one."
  (org-back-to-heading 't)
  (message (format "org-map-tree start %S" (point)))
  (let ((level (funcall outline-level)))
    (save-excursion
      (funcall fun)
      (while (and (progn
                    (prog1
                      (outline-next-heading)
                      (message (format "org-map-tree outline-ext-point %S" (point)))
                      )
		    (> (funcall outline-level) level))
		  (not (eobp)))
	(funcall fun)))))

(save-excursion
; Expand first tree
(goto-char 0)
(while (not (equal org-cycle-subtree-status 'folded))
  (org-cycle))

; Run map over `Hidden`
(goto-char 10)
(setq accum nil)
(fixed-org-map-tree (lambda () (add-to-list 'accum (point))))
accum
)

#+end_src

#+RESULTS:
(33 20 10)

However, this is changes behavior, and people may be relying upon this behaviour in their scripts... though it would
be rather strange to have your code behave different based on whether subheadings are visible or not. So perhaps
we should just make this change.







* Backtrace
This is a behavioral bug, there is not backtrace.

Re: Bug: org-map-tree behaves strangely if first heading before point is hidden [8.3.5 (8.3.5-elpa @ /home/tom/.emacs.d/elpa/org-20160725/)]

[Nicolas Goaziou]

Hello,

talwrii talwrii <talwrii@gmail.com> writes:

> Org mode file complete with code to reproduce the issue is attached.
>
> * Folded
> ** Hidden
> *** Children
> *** More children
>
> ** Other child
>
>
> * Bug description
> When calling org-map-tree on a hidden tree, the results are different from
> those if the tree were visible: this is not as expected.
>
> * Repro
> The following code samples consistently reproduce this behaviour.
> Run then with `C-c C-c` when the cursor is over the code block.
>
> The first folds up the first tree in this document and then calls `org-map-tree` with the point at "Hidden";
> the second does the same but expands the first tree.
>
> I consider the case with the expanded tree to retrun the correct result.
> ** Folded
> #+begin_src emacs-lisp :results raw
> (save-excursion
> ; Expand first tree
> (goto-char 0)
> (while (not (equal org-cycle-subtree-status 'folded))
>   (org-cycle))
>
> ; Run map over `Hidden`
> (goto-char 10)
> (setq accum nil)
> (org-map-tree (lambda () (add-to-list 'accum (point))))
> accum
> )
>
> #+end_src
> #+RESULTS:
> (52 33 20 10 1)
> ** Expanded
> #+begin_src emacs-lisp :results raw
> (save-excursion 
>
> (goto-char 0)
> (setq accum nil)
>
> (while (not (equal org-cycle-subtree-status 'children))
>   (message (format "%S" org-cycle-subtree-status))
>   (org-cycle))
>
> (goto-char 10)
>
> (org-map-tree (lambda () (add-to-list 'accum (point))))
> accum
> )
> #+end_src
>
> #+RESULTS:
> (33 20 10)
>
> * Likely cause and fix
>
> Looking at `org-map-tree`:
>
> (defun org-map-tree (fun)
>   "Call FUN for every heading underneath the current one."
>   (org-back-to-heading)
>   (message (format "org-map-tree start %S" (point)))
>   (let ((level (funcall outline-level)))
>     (save-excursion
>       (funcall fun)
>       (while (and (progn
>                     (prog1
>                       (outline-next-heading)
>                       (message (format "org-map-tree outline-ext-point %S" (point)))
>                       )
> 		    (> (funcall outline-level) level))
> 		  (not (eobp)))
> 	(funcall fun)))))
>
> We see that it calls `org-back-to-heading`; this skips back to the first visible ancestor heading.
>
> `org-back-to-heading` has the optional argument `invisible-ok` which causes it to skip back to the first
> ancestor heading, whether it is visible or not.
>
> (defun org-back-to-heading (&optional invisible-ok)
>   "Call `outline-back-to-heading', but provide a better error message."
>   (condition-case nil
>       (outline-back-to-heading invisible-ok)
>     (error (error "Before first headline at position %d in buffer %s"
> 		  (point) (current-buffer)))))
>
> I think that `org-map-subtree` should call `org-back-to-heading` with
> the `invisible-ok` argument set to `'t`

That's correct.

> However, this is changes behavior, and people may be relying upon this behaviour in their scripts... though it would
> be rather strange to have your code behave different based on whether subheadings are visible or not. So perhaps
> we should just make this change.

This is not a big issue because you can test within the function if the
headline is collapsed or not.

Anyway, just to be safe, I've changed it in master.

Thank you.

Regards,

-- 
Nicolas Goaziou