bug#45393: 27.1; Make remove-hook (interactive

bug#45393: 27.1; Make remove-hook (interactive

[Thibault Polge]

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

This is a follow-up to a previous discussion on emacs-devel[1].

The attached patch makes `remove-hook` interactive.  A common use case
for remove-hook is to fix your own mistakes when programming Emacs:
renaming a hook function for something more expressive, moving a
function to a different hook or from global to local, and so on.  For
these cases, it seems more natural to use the function interactively
than to have to write throwaway lisp one-liners.

A limitation of this approach is that since completion requires a text
representation of the function to remove, if two hooks have the same
representation under `princ` it will be impossible to distinguish
between them.  In this case, which is probably *extremely* rare (and
only concerns anonymous functions), only the first one will be
removed.

The copyright assignment paperwork was completed on Dec, 11.

[1] See: https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg00860.html


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-remove-hook-interactive.patch --]
[-- Type: text/x-patch, Size: 2145 bytes --]

From 8175c3f1e92a53a6b9bb9e78cfb7d2cb481bf583 Mon Sep 17 00:00:00 2001
From: Thibault Polge <thibault@thb.lt>
Date: Wed, 23 Dec 2020 14:19:15 +0100
Subject: [PATCH] Make `remove-hook` interactive

* lisp/subr.el: modify remove-hook to make it interactive.
---
 lisp/subr.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 1fb0f9ab7e..2c8bc6a191 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1742,7 +1742,30 @@ FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
 list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
 
 The optional third argument, LOCAL, if non-nil, says to modify
-the hook's buffer-local value rather than its default value."
+the hook's buffer-local value rather than its default value.
+
+Interactively, prompt for the various arguments (skipping local
+unless HOOK has both local and global functions).  If multiple
+functions have the same representation under `princ', the first
+one will be removed."
+  (interactive
+   (let* ((hook (intern (completing-read "Hook variable: " obarray #'boundp t)))
+          (local
+           (and
+            (local-variable-p hook)
+            (symbol-value hook)
+            (or (not (default-value hook)) ; No need to prompt if there's nothing global
+                (y-or-n-p (format "%s has a buffer-local binding, use that? " hook)))))
+          (fn-alist (mapcar
+                     (lambda (x) (cons (with-output-to-string (prin1 x)) x))
+                     (if local (symbol-value hook) (default-value hook))))
+          (function (alist-get (completing-read
+                                (format "%s hook to remove:"
+                                        (if local "Buffer-local" "Global"))
+                                fn-alist
+                                nil t)
+                               fn-alist nil nil 'string=)))
+     (list hook function local)))
   (or (boundp hook) (set hook nil))
   (or (default-boundp hook) (set-default hook nil))
   ;; Do nothing if LOCAL is t but this hook has no local binding.
-- 
2.29.2


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

Best regards,
Thibault

bug#45393: 27.1; Make remove-hook (interactive

[Drew Adams]

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

1. I think this is a fine idea.

For one thing, it'll help with the gotcha of
someone misguidedly using a lambda form as a
hook function (which requires using the same
form for removal).  Instead of having to type
the lambda form exactly for `remove-hook', it
can be retrieved from the history.

2. Please add a space char at the end of the
"Function:" prompt.

3. The doc string should maybe start with a
description of the interactive use.  I think
that's typical.  And that description should
describe each arg (instead of the interactive
description just referring to the Lisp
description).

On the other hand, 90% of the uses will not
be interactive, so maybe that general rule
shouldn't be followed here?

4. I suggest doing the same for `add-hook',
and letting `remove-hook' use the last hook
added interactively as default when reading
the hook.

Attached is some quick-and-dirty code that
does that, to show what I mean.  If the idea
is accepted then we can work out a patch.

When `add-hook' is used interactively (only),
the hook var and function are recorded in
vars `last-add-hook-var' and `hook-history'.

[-- Attachment #2: throw-add-remove-hook.el --]
[-- Type: application/octet-stream, Size: 6081 bytes --]

(defvar last-add-hook-var nil
  "Last hook variable used with `add-hook' interactively.")
(defvar hook-history ()
  "History of hook variables used as minibuffer input.")

(defun add-hook (hook function &optional append local msgp)
  "Add to the value of HOOK the function FUNCTION.
FUNCTION is not added if already present.
FUNCTION is added (if necessary) at the beginning of the hook list
unless the optional argument APPEND is non-nil, in which case
FUNCTION is added at the end.

The optional fourth argument, LOCAL, if non-nil, says to modify
the hook's buffer-local value rather than its global value.
This makes the hook buffer-local, and it makes t a member of the
buffer-local value.  That acts as a flag to run the hook
functions of the global value as well as in the local value.

HOOK should be a symbol, and FUNCTION may be any valid function.  If
HOOK is void, it is first set to nil.  If HOOK's value is a single
function, it is changed to a list of functions.

Interactively, prompt for the various arguments (skipping local
unless HOOK has both local and global functions).  With a prefix
argument, append, else prepend."
  (interactive
   (let* ((hook (intern
		 (completing-read "Hook variable: "
				  obarray #'boundp t nil
				  'hook-history last-add-hook-var)))
          (local
           (and
            (local-variable-p hook)
            (symbol-value hook)
            (or (not (default-value hook)) ; No need to prompt if there's nothing global
                (y-or-n-p (format "%s has a buffer-local binding, use that? " hook)))))
	  (function (read-from-minibuffer "Function: " nil nil 'READ)))
     (list hook function current-prefix-arg local t)))
  (or (boundp hook) (set hook nil))
  (or (default-boundp hook) (set-default hook nil))
  (if local (unless (local-variable-if-set-p hook)
	      (set (make-local-variable hook) (list t)))
    ;; Detect the case where make-local-variable was used on a hook
    ;; and do what we used to do.
    (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
      (setq local t)))
  (let ((hook-value (if local (symbol-value hook) (default-value hook))))
    ;; If the hook value is a single function, turn it into a list.
    (when (or (not (listp hook-value)) (functionp hook-value))
      (setq hook-value (list hook-value)))
    ;; Do the actual addition if necessary
    (unless (member function hook-value)
      (when (stringp function)
	(setq function (purecopy function)))
      (setq hook-value
	    (if append
		(append hook-value (list function))
	      (cons function hook-value))))
    ;; Set the actual variable
    (if local
	(progn
	  ;; If HOOK isn't a permanent local,
	  ;; but FUNCTION wants to survive a change of modes,
	  ;; mark HOOK as partially permanent.
	  (and (symbolp function)
	       (get function 'permanent-local-hook)
	       (not (get hook 'permanent-local))
	       (put hook 'permanent-local 'permanent-local-hook))
	  (set hook hook-value))
      (set-default hook hook-value)))
  (when msgp (setq last-add-hook-var  hook)))


(defun remove-hook (hook function &optional local)
  "Remove from the value of HOOK the function FUNCTION.
HOOK should be a symbol, and FUNCTION may be any valid function.  If
FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
list of hooks to run in HOOK, then nothing is done.  See `add-hook'.

The optional third argument, LOCAL, if non-nil, says to modify
the hook's buffer-local value rather than its default value.

Interactively, prompt for the various arguments (skipping local
unless HOOK has both local and global functions).  If multiple
functions have the same representation under `princ', remove the
first one."
  (interactive
   (let* ((hook (intern
		 (completing-read "Hook variable: "
				  obarray #'boundp t nil 'hook-history
				  (and last-add-hook-var
				       (symbol-name last-add-hook-var)))))
          (local
           (and
            (local-variable-p hook)
            (symbol-value hook)
            (or (not (default-value hook)) ; No need to prompt if there's nothing global
                (y-or-n-p (format "%s has a buffer-local binding, use that? " hook)))))
          (fn-alist (mapcar
                     (lambda (x) (cons (with-output-to-string (prin1 x)) x))
                     (if local (symbol-value hook) (default-value hook))))
          (function (alist-get (completing-read
                                (format "%s hook to remove: "
                                        (if local "Buffer-local" "Global"))
                                fn-alist
                                nil t)
                               fn-alist nil nil 'string=)))
     (list hook function local)))
  (or (boundp hook) (set hook nil))
  (or (default-boundp hook) (set-default hook nil))
  ;; Do nothing if LOCAL is t but this hook has no local binding.
  (unless (and local (not (local-variable-p hook)))
    ;; Detect the case where make-local-variable was used on a hook
    ;; and do what we used to do.
    (when (and (local-variable-p hook)
	       (not (and (consp (symbol-value hook))
			 (memq t (symbol-value hook)))))
      (setq local t))
    (let ((hook-value (if local (symbol-value hook) (default-value hook))))
      ;; Remove the function, for both the list and the non-list cases.
      (if (or (not (listp hook-value)) (eq (car hook-value) 'lambda))
	  (if (equal hook-value function) (setq hook-value nil))
	(setq hook-value (delete function (copy-sequence hook-value))))
      ;; If the function is on the global hook, we need to shadow it locally
      ;;(when (and local (member function (default-value hook))
      ;;	       (not (member (cons 'not function) hook-value)))
      ;;  (push (cons 'not function) hook-value))
      ;; Set the actual variable
      (if (not local)
	  (set-default hook hook-value)
	(if (equal hook-value '(t))
	    (kill-local-variable hook)
	  (set hook hook-value))))))

bug#45393: 27.1; Make remove-hook (interactive

[Lars Ingebrigtsen]

Thibault Polge <thibault@thb.lt> writes:

> The attached patch makes `remove-hook` interactive.

Looks good; applied to Emacs 28 (after some minor white-space changes to
make the lines shorter than 80 characters).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45393: 27.1; Make remove-hook (interactive

[Juri Linkov]

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

>> The attached patch makes `remove-hook` interactive.
>
> Looks good; applied to Emacs 28 (after some minor white-space changes to
> make the lines shorter than 80 characters).

It's difficult to use this feature without a default value.
Here's the patch that adds it:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: remove-hook-default.patch --]
[-- Type: text/x-diff, Size: 696 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 1acc3c3250..ad0c812ed3 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1749,7 +1749,11 @@ remove-hook
 functions have the same representation under `princ', the first
 one will be removed."
   (interactive
-   (let* ((hook (intern (completing-read "Hook variable: " obarray #'boundp t)))
+   (let* ((default (and (symbolp (variable-at-point))
+                        (symbol-name (variable-at-point))))
+          (hook (intern (completing-read
+                         (format-prompt "Hook variable" default)
+                         obarray #'boundp t nil nil default)))
           (local
            (and
             (local-variable-p hook)

bug#45393: 27.1; Make remove-hook (interactive

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> It's difficult to use this feature without a default value.
> Here's the patch that adds it:

Sure; looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no