emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [RFC] Replace lambda functions added to org-mode-hook with named funcs
@ 2018-10-04 15:10 Kaushal Modi
  2018-10-05 10:42 ` Nicolas Goaziou
  0 siblings, 1 reply; 5+ messages in thread
From: Kaushal Modi @ 2018-10-04 15:10 UTC (permalink / raw)
  To: emacs-org list

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/

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [RFC] Replace lambda functions added to org-mode-hook with named funcs
@ 2019-10-01  5:00 Phil Sainty
  2019-10-06  9:54 ` stardiviner
  0 siblings, 1 reply; 5+ messages in thread
From: Phil Sainty @ 2019-10-01  5:00 UTC (permalink / raw)
  To: emacs-orgmode; +Cc: Nicolas Goaziou, Kaushal Modi

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-06 10:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04 15:10 [RFC] Replace lambda functions added to org-mode-hook with named funcs Kaushal Modi
2018-10-05 10:42 ` Nicolas Goaziou
2018-10-06 13:42   ` Kaushal Modi
  -- strict thread matches above, loose matches on Subject: below --
2019-10-01  5:00 Phil Sainty
2019-10-06  9:54 ` stardiviner

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).