[bug] org-cycle changed behavior in orgstruct-mode

[bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Hi,

In org 8.3 orgstruct-mode and org-cycle is no longer working as expected.

In my init file I have,

     ;; Local Variables:
     ;; outline-regexp: ";;\\*+\\|\\`"
     ;; orgstruct-heading-prefix-regexp: ";;\\*+\\|\\`"
     ;; eval: (when after-init-time (orgstruct-mode) (org-global-cycle 3))
     ;; End:

With org 8.3+, when I use org-cycle on a headline, e.g. ";;* Org", the
display is no longer consistent with outline-toggle-children (I think the
"outline"-prefix might be a recent addition).

In Org 8.3+

    No tabs:
        ;;* Org

    One tab:
        ;;* Org
        ...

    Two tabs:
        ;;* Org
        ;;** Subhead1
             forms1
        ;;** Subhead2
             forms2


In Org 8.2.10 behavior is consistent with outline-toggle-children:


    No tabs:
        ;;* Org

    One tab:
        ;;* Org
        ;;** Subhead1
        ;;** Subhead2

    Two tabs:
        ;;* Org
        ;;** Subhead1
             forms1
        ;;** Subhead2
             forms2

I don't know how to debug this further.  Hints appreciated.

Ramus

-- 
Lasciate ogni speranza, voi che leggete questo.

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Hello,

Rasmus <rasmus@gmx.us> writes:

> In org 8.3 orgstruct-mode and org-cycle is no longer working as
> expected.

I assume you mean development version (i.e., Org 8.4), not Org 8.3
(maint).

> With org 8.3+, when I use org-cycle on a headline, e.g. ";;* Org", the
> display is no longer consistent with outline-toggle-children (I think the
> "outline"-prefix might be a recent addition).
>
> In Org 8.3+
>
>     No tabs:
>         ;;* Org
>
>     One tab:
>         ;;* Org
>         ...
>
>     Two tabs:
>         ;;* Org
>         ;;** Subhead1
>              forms1
>         ;;** Subhead2
>              forms2
>
>
> In Org 8.2.10 behavior is consistent with outline-toggle-children:
>
>
>     No tabs:
>         ;;* Org
>
>     One tab:
>         ;;* Org
>         ;;** Subhead1
>         ;;** Subhead2
>
>     Two tabs:
>         ;;* Org
>         ;;** Subhead1
>              forms1
>         ;;** Subhead2
>              forms2
>
> I don't know how to debug this further.  Hints appreciated.

Could you comment out

  (define-key org-mode-map [remap show-children] 'org-show-children)

in org.el?

I added this line, but I think it is a mistake.


Regards,

-- 
Nicolas Goaziou

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Hi,

>> In org 8.3 orgstruct-mode and org-cycle is no longer working as
>> expected.
>
> I assume you mean development version (i.e., Org 8.4), not Org 8.3
> (maint).

I pull from master, but (org-version) gives "Org-mode version 8.3.1
(release_8.3.1-47-g3e7aa3 @ /usr/share/emacs/site-lisp/org/)".

>> ..
>
> Could you comment out
>
>   (define-key org-mode-map [remap show-children] 'org-show-children)
>
> in org.el?
>
> I added this line, but I think it is a mistake.

I tried to recompile with that line commented out but it doesn't make a
difference.  I also tried to remove that defun from orgstruct-setup, but
that also didn't make change anything.

If there is a way to get the buffer-string for what is actually displayed
in the window I could use git bisect to find the bad commit, but I don't
know if that is possible.

Rasmus

-- 
. . . It begins of course with The Internet.  A Net of Peers

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> I tried to recompile with that line commented out but it doesn't make a
> difference.  I also tried to remove that defun from orgstruct-setup, but
> that also didn't make change anything.

Hmm. Odd.
>
> If there is a way to get the buffer-string for what is actually displayed
> in the window I could use git bisect to find the bad commit, but I don't
> know if that is possible.

See `org-copy-visible'.

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Rasmus <rasmus@gmx.us> writes:
>
>> I tried to recompile with that line commented out but it doesn't make a
>> difference.  I also tried to remove that defun from orgstruct-setup, but
>> that also didn't make change anything.
>
> Hmm. Odd.
>>
>> If there is a way to get the buffer-string for what is actually displayed
>> in the window I could use git bisect to find the bad commit, but I don't
>> know if that is possible.
>
> See `org-copy-visible'.

Great.  The bad commit seems to be:

    commit 898cfbcac0560d1d742d939a62c5a8253fe9b66f
    Author: Nicolas Goaziou <mail@nicolasgoaziou.fr>
    Date:   Mon Aug 10 13:34:07 2015 +0200

        Implement faster `show-children' function

        * lisp/org.el (org-show-children): New function.
        (org-cycle-internal-local):
        (org-set-visibility-according-to-property):
        (org-content):
        (org-move-subtree-down):
        (orgstruct-setup):
        (org-show-set-visibility):
        * contrib/lisp/org-toc.el (org-toc-cycle-subtree):
        (org-toc-restore-config):
        * lisp/org-agenda.el (org-agenda-show-1):
        * lisp/org-feed.el (org-feed-update): Use new function.

        * etc/ORG-NEWS: Document new function.

        Suggested-by: Samuel Wales <samologist@gmail.com>
        <http://permalink.gmane.org/gmane.emacs.orgmode/99616>

I guess I will have to dissect it more to figure out what broke.

Here's my test.el

    (add-to-list 'load-path "~/src/code/org-mode/lisp")
    (require 'org)
    (let ((ol-rx ";;\\*+\\|\\`")
          (kill noninteractive))
      (with-temp-buffer
        (insert ";;* test
    ;;** subhead
    form1
    ")
        (set 'outline-regexp ol-rx)
        (set 'orgstruct-heading-prefix-regexp ol-rx)
        (orgstruct-mode)
        (org-global-cycle 3)
        (goto-char (point-min))
        (orgstruct-hijacker-org-cycle-4 nil)
        (let ((str (org-copy-visible (point-min) (point-max))))
          (if (string-match-p ";;\\* test\n+$" str)
              (progn (message "failed")
                     (and kill (kill-emacs 1)))
            (message "success")
            (and kill (kill-emacs 0)))
          str)))

And my test.sh

    #!/bin/sh
    emacs --batch --load test.el

Rasmus

-- 
Slowly unravels in a ball of yarn and the devil collects it

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> I tried to recompile with that line commented out but it doesn't make a
> difference.  I also tried to remove that defun from orgstruct-setup, but
> that also didn't make change anything.

What if you replace the defun with `show-children' in `orgstruct-setup'
instead of simply removing the line? You may want to still comment out
the line above though.

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> Great.  The bad commit seems to be:
>
>     commit 898cfbcac0560d1d742d939a62c5a8253fe9b66f
>     Author: Nicolas Goaziou <mail@nicolasgoaziou.fr>
>     Date:   Mon Aug 10 13:34:07 2015 +0200
>
>         Implement faster `show-children' function
>
>         * lisp/org.el (org-show-children): New function.
>         (org-cycle-internal-local):
>         (org-set-visibility-according-to-property):
>         (org-content):
>         (org-move-subtree-down):
>         (orgstruct-setup):
>         (org-show-set-visibility):
>         * contrib/lisp/org-toc.el (org-toc-cycle-subtree):
>         (org-toc-restore-config):
>         * lisp/org-agenda.el (org-agenda-show-1):
>         * lisp/org-feed.el (org-feed-update): Use new function.
>
>         * etc/ORG-NEWS: Document new function.
>
>         Suggested-by: Samuel Wales <samologist@gmail.com>
>         <http://permalink.gmane.org/gmane.emacs.orgmode/99616>
>
> I guess I will have to dissect it more to figure out what broke.

`org-cycle-internal-local' calls `org-show-children', which is the Org
specific implementation of `show-children'.

I don't think orgstruct details should leak into `org-show-children', so
I suggest to plug orgstruct stuff, i.e., make sure `show-children' is
called instead of `org-show-children', elsewhere. 

One possibility is to use a hook in `org-cycle', not sure which one
though. `orgstruct-mode' could also assume its kludge status and use
something like `add-function'.

WDYT?

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Rasmus <rasmus@gmx.us> writes:
>
>> Great.  The bad commit seems to be:
>>
>>     commit 898cfbcac0560d1d742d939a62c5a8253fe9b66f
>>     Author: Nicolas Goaziou <mail@nicolasgoaziou.fr>
>>     Date:   Mon Aug 10 13:34:07 2015 +0200
>>
>>         Implement faster `show-children' function
>>
>>         * lisp/org.el (org-show-children): New function.
>>         (org-cycle-internal-local):
>>         (org-set-visibility-according-to-property):
>>         (org-content):
>>         (org-move-subtree-down):
>>         (orgstruct-setup):
>>         (org-show-set-visibility):
>>         * contrib/lisp/org-toc.el (org-toc-cycle-subtree):
>>         (org-toc-restore-config):
>>         * lisp/org-agenda.el (org-agenda-show-1):
>>         * lisp/org-feed.el (org-feed-update): Use new function.
>>
>>         * etc/ORG-NEWS: Document new function.
>>
>>         Suggested-by: Samuel Wales <samologist@gmail.com>
>>         <http://permalink.gmane.org/gmane.emacs.orgmode/99616>
>>
>> I guess I will have to dissect it more to figure out what broke.
>
> `org-cycle-internal-local' calls `org-show-children', which is the Org
> specific implementation of `show-children'.

OK

> I don't think orgstruct details should leak into `org-show-children', so
> I suggest to plug orgstruct stuff, i.e., make sure `show-children' is
> called instead of `org-show-children', elsewhere. 

Sounds right.

> One possibility is to use a hook in `org-cycle', not sure which one
> though. `orgstruct-mode' could also assume its kludge status and use
> something like `add-function'.

This is a possibility.  Isn't nadvice.el is too recent though?  [Do you
check NEWS files to see when something is added?].  Anyway, we can add an
advice around it and redefine org-show-children or something....  Sounds
ugly.

Would it be enough to just change the keymap of TAB-like keys?

I would love to rip orgstruct out of org.el and make it less
hacky/"kludgy".  The implementation remains mostly opaque to me.

Rasmus

-- 
The Kids call him Billy the Saint

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Rasmus <rasmus@gmx.us> writes:
>
>> I tried to recompile with that line commented out but it doesn't make a
>> difference.  I also tried to remove that defun from orgstruct-setup, but
>> that also didn't make change anything.
>
> What if you replace the defun with `show-children' in `orgstruct-setup'
> instead of simply removing the line? You may want to still comment out
> the line above though.

I tried to add show-children, and NOT have org-show-children in the setup
but alas it doesn't change the situation.  But anyway it's called through
`org-cycle', in my use-case at least.  This in turn rely on at least
org-cycle-internal-local which in turn uses org-show-children.

So maybe the easiest way to "fix" this is to advice orgstruct to redirect
call?  Maybe add-function :around... We can rely on nadvice in master,
right?

Rasmus

-- 
Need more coffee. . .

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> I tried to add show-children, and NOT have org-show-children in the setup
> but alas it doesn't change the situation.  But anyway it's called through
> `org-cycle', in my use-case at least.  This in turn rely on at least
> org-cycle-internal-local which in turn uses org-show-children.

Then `org-cycle-internal-local' could use something like:

  (if (derived-mode-p 'org-mode) #'org-show-children #'show-children)

This would require a comment in the code. This is a small leak but as
long as we don't have a clear (non broken) design for orgstruct, I think
we cannot do much.

> So maybe the easiest way to "fix" this is to advice orgstruct to redirect
> call?  Maybe add-function :around... We can rely on nadvice in master,
> right?

Actually, we can't. "nadvice.el" was introduced in 24.4. AFAIK master
still supports release 24.3.

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

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

Hi,

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

>> I tried to add show-children, and NOT have org-show-children in the setup
>> but alas it doesn't change the situation.  But anyway it's called through
>> `org-cycle', in my use-case at least.  This in turn rely on at least
>> org-cycle-internal-local which in turn uses org-show-children.
>
> Then `org-cycle-internal-local' could use something like:
>
>   (if (derived-mode-p 'org-mode) #'org-show-children #'show-children)
>
> This would require a comment in the code. This is a small leak but as
> long as we don't have a clear (non broken) design for orgstruct, I think
> we cannot do much.

Do you mean to change org-cycle-internal-local as in this patch or to make
a hook around it?  Maybe the exception should be if orgstruct is detected
rather than if org is not detected.

>> So maybe the easiest way to "fix" this is to advice orgstruct to redirect
>> call?  Maybe add-function :around... We can rely on nadvice in master,
>> right?
>
> Actually, we can't. "nadvice.el" was introduced in 24.4. AFAIK master
> still supports release 24.3.

OK.  If we can pollute org-cycle-internal-local it doesn't matter.
Otherwise I can write an "old-style" advice.

Rasmus

-- 
History is what should never happen again

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org.el-Only-call-org-show-children-from-org-mode.patch --]
[-- Type: text/x-diff, Size: 1163 bytes --]

From 1dcad907938450d0c718aa6f79a1a363ed6d72cd Mon Sep 17 00:00:00 2001
From: Rasmus <rasmus@gmx.us>
Date: Sun, 30 Aug 2015 20:16:28 +0200
Subject: [PATCH] org.el: Only call org-show-children from org-mode

* org.el (org-cycle-internal-local): Only call org-show-children in
  org-mode.

orgstruct-mode does not behave nicely when using org-show-children as
opposed to show-children.
---
 lisp/org.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 82c42c2..49e4e53 100755
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6902,7 +6902,11 @@ in special contexts.
       (if (org-at-item-p)
 	  (org-list-set-item-visibility (point-at-bol) struct 'children)
 	(org-show-entry)
-	(org-with-limited-levels (org-show-children))
+	(org-with-limited-levels
+	 ;; `orgstruct-mode' does not work well with
+	 ;; `org-show-children'.  Thus, use `show-children' unless in
+	 ;; org-mode.
+	 (if (derived-mode-p 'org-mode) (org-show-children) (show-children)))
 	;; FIXME: This slows down the func way too much.
 	;; How keep drawers hidden in subtree anyway?
 	;; (when (memq 'org-cycle-hide-drawers org-cycle-hook)
-- 
2.5.1

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> Do you mean to change org-cycle-internal-local as in this patch or to make
> a hook around it?

The former. However, a hook is OK too, but I couldn't find an acceptable
candidate.

Funnily, `org-end-of-subtree' does the exact opposite: it hard-codes
behaviour and switch to the slow, outline.el, function when in
a different mode. Other functions do not even have a fast path (e.g.
`org-forward-heading-same-level').

We could make these functions faster, but we first need to decide how to
proceed when it happens. After all, maybe the most simple path is good
ole advice. Since `orgstruct-mode' already hijacks bindings, the next
step would be to do the same for functions.

WDYT?

> Maybe the exception should be if orgstruct is detected rather than if
> org is not detected.

Not really, because you can have orgstruct on even in an Org buffer.

> +	(org-with-limited-levels
> +	 ;; `orgstruct-mode' does not work well with
> +	 ;; `org-show-children'.  Thus, use `show-children' unless in
> +	 ;; org-mode.

Nitpick: "unless in Org mode" or "unless major mode is `org-mode'".

Thank you for the patch.

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Hi,

>> Do you mean to change org-cycle-internal-local as in this patch or to make
>> a hook around it?
>
> The former. However, a hook is OK too, but I couldn't find an acceptable
> candidate.
>
> Funnily, `org-end-of-subtree' does the exact opposite: it hard-codes
> behaviour and switch to the slow, outline.el, function when in
> a different mode. Other functions do not even have a fast path (e.g.
> `org-forward-heading-same-level').
>
> We could make these functions faster, but we first need to decide how to
> proceed when it happens.

But are the slowdowns associated with outline.el functions anything that
will realistically be felt in normal usage?

>  After all, maybe the most simple path is good ole advice. Since
> `orgstruct-mode' already hijacks bindings, the next step would be to do
> the same for functions.
>
> WDYT?

IMO the biggest advantage of orgstruct is that it has more sensible
keybindings than outline.  Of course it's also nice with lists and tables
(assuming they are not in orgstruct++; don't remember)...  Adding advice
to orgstruct, which is a mess anyway, won't make it any worse than it
is...

I'm all for advising if that's easier or if we want to add workarounds in
more places.  For my problem, the proposed one line patch fixes it,
though.

>> Maybe the exception should be if orgstruct is detected rather than if
>> org is not detected.
>
> Not really, because you can have orgstruct on even in an Org buffer.

OK.

-- 
. . . It begins of course with The Internet.  A Net of Peers

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Nicolas Goaziou]

Rasmus <rasmus@gmx.us> writes:

> But are the slowdowns associated with outline.el functions anything that
> will realistically be felt in normal usage?

They are noticeable under certain circumstances. I implemented
`org-show-children' because `show-children' was /reported/ as slow.

Generally speaking, outline doesn't cope well with dense sub-trees. As
an example, consider `org-forward-heading-same-level'. Using outline, we
need to move to next headline, check its level, compare with original,
rinse and repeat, possibly many times, until we find a match. However,
we could simply do

  (re-search-forward some-appropriate-regexp nil t)

and be done with it since we can turn level into a regexp.

> IMO the biggest advantage of orgstruct is that it has more sensible
> keybindings than outline.  Of course it's also nice with lists and tables
> (assuming they are not in orgstruct++; don't remember)...

Table is orgtbl-mode, not orgstruct-mode. 

List handling is a real concern however, because we cannot use the
parser (there is no context to know about) but still have to analyze
lists.

> Adding advice to orgstruct, which is a mess anyway, won't make it any
> worse than it is...

Agreed.

> I'm all for advising if that's easier or if we want to add workarounds in
> more places.  For my problem, the proposed one line patch fixes it,
> though.

Indeed. But it only solves `org-show-children'. I have the feeling that
we will encounter a similar issue with another base function like this
one later, tho.

Regards,

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Hi,

>> But are the slowdowns associated with outline.el functions anything that
>> will realistically be felt in normal usage?
>
> They are noticeable under certain circumstances. I implemented
> `org-show-children' because `show-children' was /reported/ as slow.

OK.

> Generally speaking, outline doesn't cope well with dense sub-trees. As
> an example, consider `org-forward-heading-same-level'. Using outline, we
> need to move to next headline, check its level, compare with original,
> rinse and repeat, possibly many times, until we find a match. However,
> we could simply do
>
>   (re-search-forward some-appropriate-regexp nil t)
>
> and be done with it since we can turn level into a regexp.

Indeed.  That seems like a better implementation in any case.

>> IMO the biggest advantage of orgstruct is that it has more sensible
>> keybindings than outline.  Of course it's also nice with lists and tables
>> (assuming they are not in orgstruct++; don't remember)...
>
> Table is orgtbl-mode, not orgstruct-mode. 
>
> List handling is a real concern however, because we cannot use the
> parser (there is no context to know about) but still have to analyze
> lists.

In an ideal world it should also "ignore" comment-start in the sense that
I it should help me with lists after, e.g. ";;".  I wonder if any of
Thorsten's packages on this already solved this issue.

>> Adding advice to orgstruct, which is a mess anyway, won't make it any
>> worse than it is...
>
> Agreed.

OK, so I'll experiment with an "old advice" around orgstruct.

BTW: We have 20 defadvice in lisp/.  That seems like enough reason (to me)
to bump Org 9 to 24.4.

Rasmus

-- 
And when I’m finished thinking, I have to die a lot

Re: [bug] org-cycle changed behavior in orgstruct-mode

[Rasmus]

Hi,

I have been busy.

So was the consensus to fix this using "old-style" advice or can I use
nadvice syntax?  As Nicolas mentioned, the latter bumps the requirement of
master to 24.4 instead of 24.3, but it's much nicer to write, read and
debug.

BTW: I think recently AUCTeX made changes that require 25, but I don't
     read that list carefully.

Rasmus

-- 
History is what should never happen again