[RFC] Replace lambda functions added to org-mode-hook with named funcs

[RFC] Replace lambda functions added to org-mode-hook with named funcs

[Kaushal Modi]

Hello,

Yesterday, while helping someone out[0] with why their custom
functions added to org-mode-hook didn't work, I asked them to reveal
the value of org-mode-hook, and they presented this as the default
value of org-mode-hook once Org was loaded:

=====
'(org-mode-hook
   (quote
    (#[0 "\300\301\302\303\304$\207"
         [add-hook change-major-mode-hook org-show-block-all append local]
         5]
     #[0 "\300\301\302\303\304$\207"
         [add-hook change-major-mode-hook org-babel-show-result-all
append local]
         5]
     org-babel-result-hide-spec org-babel-hide-all-hashes)))
=====

Going down the rabbit hole, I discovered many places in Org source
where lambdas were added to org-mode-hook.

I propose to replace such lamba functions with named functions.
Here's an example of diff on maint branch, after making one such change:

=====
diff --git a/lisp/org.el b/lisp/org.el
index 2cc9b6a1c..9f28502d4 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -7429,10 +7429,10 @@ a block.  Return a non-nil value when toggling
is successful."
       (when (eq (overlay-get ov 'invisible) 'org-hide-block)
         (delete-overlay ov))))))))

-;; Remove overlays when changing major mode
-(add-hook 'org-mode-hook
-      (lambda () (add-hook 'change-major-mode-hook
-                   'org-show-block-all 'append 'local)))
+(defun org--unfold-all-blocks-on-major-mode-change ()
+  "Remove overlays when changing major mode."
+  (add-hook 'change-major-mode-hook #'org-show-block-all 'append 'local))
+(add-hook 'org-mode-hook #'org--unfold-all-blocks-on-major-mode-change)

 ;;; Org-goto
 =====

If there is no objection to this, I can fix this everywhere in maint,
and then merge that into master.

Comments?


--
Kaushal Modi

[0]: https://www.reddit.com/r/emacs/comments/9l1aji/org_mode_hooks_dont_work/e73awsc/

Re: [RFC] Replace lambda functions added to org-mode-hook with named funcs

[Nicolas Goaziou]

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Going down the rabbit hole, I discovered many places in Org source
> where lambdas were added to org-mode-hook.
>
> I propose to replace such lamba functions with named functions.
> Here's an example of diff on maint branch, after making one such change:
>
> =====
> diff --git a/lisp/org.el b/lisp/org.el
> index 2cc9b6a1c..9f28502d4 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -7429,10 +7429,10 @@ a block.  Return a non-nil value when toggling
> is successful."
>        (when (eq (overlay-get ov 'invisible) 'org-hide-block)
>          (delete-overlay ov))))))))
>
> -;; Remove overlays when changing major mode
> -(add-hook 'org-mode-hook
> -      (lambda () (add-hook 'change-major-mode-hook
> -                   'org-show-block-all 'append 'local)))
> +(defun org--unfold-all-blocks-on-major-mode-change ()
> +  "Remove overlays when changing major mode."
> +  (add-hook 'change-major-mode-hook #'org-show-block-all 'append 'local))
> +(add-hook 'org-mode-hook #'org--unfold-all-blocks-on-major-mode-change)

If that's a function added to `org-mode-hook', it is not useful to add
"on major mode change".

> If there is no objection to this, I can fix this everywhere in maint,
> and then merge that into master.

Please make changes in "master" instead, and merge them into "next"
then.

Regards,

-- 
Nicolas Goaziou

Re: [RFC] Replace lambda functions added to org-mode-hook with named funcs

[Kaushal Modi]

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

On Fri, Oct 5, 2018, 6:42 AM Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:

>
> If that's a function added to `org-mode-hook', it is not useful to add
> "on major mode change".
>

Yes, I'll remove that one hook addition.

> If there is no objection to this, I can fix this everywhere in maint,
> > and then merge that into master.
>
> Please make changes in "master" instead, and merge them into "next"
> then.
>

OK. I'll get to this sometime this week.

Thanks.

>

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

Re: [RFC] Replace lambda functions added to org-mode-hook with named funcs

[Phil Sainty]

On Fri, Oct 5, 2018, 6:42 AM Nicolas Goaziou <address@hidden> wrote:
> Kaushal Modi <address@hidden> writes:
> > I propose to replace such lamba functions with named functions.
> > Here's an example of diff on maint branch, after making one such change:
> >
> > -;; Remove overlays when changing major mode
> > -(add-hook 'org-mode-hook
> > -      (lambda () (add-hook 'change-major-mode-hook
> > -                   'org-show-block-all 'append 'local)))
> > +(defun org--unfold-all-blocks-on-major-mode-change ()
> > +  "Remove overlays when changing major mode."
> > +  (add-hook 'change-major-mode-hook #'org-show-block-all 'append 'local))
> > +(add-hook 'org-mode-hook #'org--unfold-all-blocks-on-major-mode-change)
> 
> If that's a function added to `org-mode-hook', it is not useful to
> add "on major mode change".

Certainly it's useful.  Or at least in general it's a common *pattern*
for a major mode to add a function to `change-major-mode-hook' so that
if the user changes from that major mode to some other major mode, the
function will be called and can put the buffer into a sensible state
before the replacement mode function is called.

The only curious thing about it to me is that this code is being run
via `org-mode-hook' rather than in the `org-mode' body; but maybe
there's some reason for that.


Regarding the general issue:

Grep shows me all of the following instances in the master branch
(commit d215c3a8c0b4c027), where a lambda is added to a hook variable
(a few of them in the form of commented suggestions to the user).

It's never a good idea; all of these should be changed to use named
functions, IMO.


-Phil


-*- grep -*-
./ob-core.el:1429:(add-hook 'org-mode-hook
./ob-haskell.el:66:  (add-hook 'inferior-haskell-hook
./ol-w3m.el:171:(add-hook
./ol-w3m.el:176:(add-hook
./org-agenda.el:2246:      (add-hook 'filter-buffer-substring-functions
./org-agenda.el:2935:	(add-hook
./org-attach.el:697:;; (add-hook
./org-compat.el:813:     (add-hook 'imenu-after-jump-hook
./org-compat.el:817:     (add-hook 'org-mode-hook
./org-compat.el:880:     (add-hook 'speedbar-visiting-tag-hook
./org-crypt.el:144:      (add-hook 'auto-save-hook
./org-crypt.el:267:  (add-hook
./org-ctags.el:196:(add-hook 'org-mode-hook
./org-ctags.el:59:;;    (add-hook 'org-mode-hook
./org-indent.el:188:	(add-hook 'filter-buffer-substring-functions
./org-mouse.el:1085:(add-hook 'org-agenda-mode-hook
./org-mouse.el:856:(add-hook 'org-mode-hook
./org-src.el:745:  (add-hook \\='org-src-mode-hook
./org.el:15697:	(add-hook 'after-save-hook
./org.el:18978:(add-hook 'occur-mode-find-occurrence-hook
./org.el:21221:(add-hook 'org-mode-hook     ;remove overlays when 
changing major mode
./org.el:2916:  (add-hook \\='org-capture-mode-hook

Re: [RFC] Replace lambda functions added to org-mode-hook with named funcs

[stardiviner]

I propose one vote for this too. This give user better understanding. Some times
I check out the variable org-mode-hook's value for troubleshooting, the
meaningful function names can help me read quickly.

Phil Sainty <psainty@orcon.net.nz> writes:

> On Fri, Oct 5, 2018, 6:42 AM Nicolas Goaziou <address@hidden> wrote:
>> Kaushal Modi <address@hidden> writes:
>> > I propose to replace such lamba functions with named functions.
>> > Here's an example of diff on maint branch, after making one such change:
>> >
>> > -;; Remove overlays when changing major mode
>> > -(add-hook 'org-mode-hook
>> > -      (lambda () (add-hook 'change-major-mode-hook
>> > -                   'org-show-block-all 'append 'local)))
>> > +(defun org--unfold-all-blocks-on-major-mode-change ()
>> > +  "Remove overlays when changing major mode."
>> > +  (add-hook 'change-major-mode-hook #'org-show-block-all 'append 'local))
>> > +(add-hook 'org-mode-hook #'org--unfold-all-blocks-on-major-mode-change)
>>
>> If that's a function added to `org-mode-hook', it is not useful to
>> add "on major mode change".
>
> Certainly it's useful.  Or at least in general it's a common *pattern*
> for a major mode to add a function to `change-major-mode-hook' so that
> if the user changes from that major mode to some other major mode, the
> function will be called and can put the buffer into a sensible state
> before the replacement mode function is called.
>
> The only curious thing about it to me is that this code is being run
> via `org-mode-hook' rather than in the `org-mode' body; but maybe
> there's some reason for that.
>
>
> Regarding the general issue:
>
> Grep shows me all of the following instances in the master branch
> (commit d215c3a8c0b4c027), where a lambda is added to a hook variable
> (a few of them in the form of commented suggestions to the user).
>
> It's never a good idea; all of these should be changed to use named
> functions, IMO.
>
>
> -Phil
>
>
> -*- grep -*-
> ./ob-core.el:1429:(add-hook 'org-mode-hook
> ./ob-haskell.el:66:  (add-hook 'inferior-haskell-hook
> ./ol-w3m.el:171:(add-hook
> ./ol-w3m.el:176:(add-hook
> ./org-agenda.el:2246:      (add-hook 'filter-buffer-substring-functions
> ./org-agenda.el:2935:	(add-hook
> ./org-attach.el:697:;; (add-hook
> ./org-compat.el:813:     (add-hook 'imenu-after-jump-hook
> ./org-compat.el:817:     (add-hook 'org-mode-hook
> ./org-compat.el:880:     (add-hook 'speedbar-visiting-tag-hook
> ./org-crypt.el:144:      (add-hook 'auto-save-hook
> ./org-crypt.el:267:  (add-hook
> ./org-ctags.el:196:(add-hook 'org-mode-hook
> ./org-ctags.el:59:;;    (add-hook 'org-mode-hook
> ./org-indent.el:188:	(add-hook 'filter-buffer-substring-functions
> ./org-mouse.el:1085:(add-hook 'org-agenda-mode-hook
> ./org-mouse.el:856:(add-hook 'org-mode-hook
> ./org-src.el:745:  (add-hook \\='org-src-mode-hook
> ./org.el:15697:	(add-hook 'after-save-hook
> ./org.el:18978:(add-hook 'occur-mode-find-occurrence-hook
> ./org.el:21221:(add-hook 'org-mode-hook     ;remove overlays when changing major
> mode
> ./org.el:2916:  (add-hook \\='org-capture-mode-hook


-- 
[ stardiviner ]
       I try to make every word tell the meaning what I want to express.

       Blog: https://stardiviner.github.io/
       IRC(freenode): stardiviner, Matrix: stardiviner
       GPG: F09F650D7D674819892591401B5DF1C95AE89AC3