GNU bug report logs - #76700
31.0.50; find-func.el: incorrect use of minor mode

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 76700 in the body.
You can then email your comments to 76700 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Madhu <enometh <at> meer.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; find-func.el: incorrect use of minor mode
Date: Mon, 03 Mar 2025 08:36:32 +0530 (IST)

[Message part 1 (text/plain, inline)]

I brought up this issue on emacs-devel but the discussion did not
proceed beyond
https://lists.gnu.org/archive/html/emacs-devel/2025-02/msg01054.html

for decades find-func.el had used `find-function-setup-keys' to set up
global bindings to functions in find-func.el.  Since these are global
bindings these did not require a minor mode.

The minor-mode was introduced in c3e989ca9d7, "New minor mode
find-function-mode replaces find-function-setup-keys" which introduced
a keymap via new features.

This was objected to on the emacs-devel list by Eshel in
+message-id:<m1ed4wfaym.fsf <at> dazzs-mbp.home> (sorry lists.gnu.org
namazu apparently websearch by mid does not work or provide 
a link to the html mailing list) on "Fri, 04 Oct 2024 11:56:17 +0200"

and the commit 1094c3f914 "find-function-mode: Define keys at a low
precedence level" was introduced to restore "backward compatibility".


However this is an incorrect use of define-minor-mode, The use of
define-minor-mode is to explicitly use a keymap to shadow global
bindings and using it to modify global bindings is a basic category
error a thinko and should not be promoted in the code base.

as the functions in find-func.el are always available for binding, a
minor mode does not make sense, if only to mutate global bindings.

global bindings set by the user when the mode is in effect are undone
when the mode is removed. Emacs should not be doing this.

I believe The original function find-function-setup-keys' introduced
by rms explicitly avoided using a minor mode precisely for these reasons. Introducing a minor mode is a conceptual regression.

If a local keymap is not desired, the minor mode should be removed and
the original function undeprecated.

The attached patch however restores the local keymap but sets up global
bindings via the original find-function-setup-keys.

[f.patch (text/x-patch, inline)]

From 1279279580f7f15e63da9624cdefa4c842f8b24f Mon Sep 17 00:00:00 2001
From: Madhu <enometh <at> net.meer>
Date: Mon, 24 Feb 2025 07:48:54 +0530
Subject: [PATCH] lisp/emacs-lisp/find-func.el: restore local keymap

* lisp/emacs-lisp/find-func.el: find-function-mode-map. (restored from
commit 1094c3f9147), (find-function-setup-keys), restored from commit
c3e989ca9, but modified to operate on global-keymap to maintain
compatibility).

 commit message 1094c9147 uses "low precedence level" instead of "global"
---
 lisp/emacs-lisp/find-func.el | 60 +++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/lisp/emacs-lisp/find-func.el b/lisp/emacs-lisp/find-func.el
index 6b7b5083620..b08b8c41aa2 100644
--- a/lisp/emacs-lisp/find-func.el
+++ b/lisp/emacs-lisp/find-func.el
@@ -812,36 +812,48 @@ find-variable-at-point
     (when (and symb (not (equal symb 0)))
       (find-variable-other-window symb))))
 
+(defvar-keymap find-function-mode-map
+  "C-x F"   #'find-function
+  "C-x 4 F" #'find-function-other-window
+  "C-x 5 F" #'find-function-other-frame
+
+  "C-x K"   #'find-function-on-key
+  "C-x 4 K" #'find-function-on-key-other-window
+  "C-x 5 K" #'find-function-on-key-other-frame
+
+  "C-x V"   #'find-variable
+  "C-x 4 V" #'find-variable-other-window
+  "C-x 5 V" #'find-variable-other-frame
+
+  "C-x L"   #'find-library
+  "C-x 4 L" #'find-library-other-window
+  "C-x 5 L" #'find-library-other-frame)
+
 ;;;###autoload
 (define-minor-mode find-function-mode
   "Enable some key bindings for the `find-function' family of functions."
-  :group 'find-function :version "31.1" :global t :lighter nil
-  ;; For compatibility with the historical behavior of the old
-  ;; `find-function-setup-keys', define our bindings at the precedence
-  ;; level of the global map.
-  :keymap nil
-  (pcase-dolist (`(,map ,key ,cmd)
-                 `((,ctl-x-map   "F" find-function)
-                   (,ctl-x-4-map "F" find-function-other-window)
-                   (,ctl-x-5-map "F" find-function-other-frame)
-                   (,ctl-x-map   "K" find-function-on-key)
-                   (,ctl-x-4-map "K" find-function-on-key-other-window)
-                   (,ctl-x-5-map "K" find-function-on-key-other-frame)
-                   (,ctl-x-map   "V" find-variable)
-                   (,ctl-x-4-map "V" find-variable-other-window)
-                   (,ctl-x-5-map "V" find-variable-other-frame)
-                   (,ctl-x-map   "L" find-library)
-                   (,ctl-x-4-map "L" find-library-other-window)
-                   (,ctl-x-5-map "L" find-library-other-frame)))
-    (if find-function-mode
-        (keymap-set map key cmd)
-      (keymap-unset map key t))))
+  :global t :lighter nil   ; compat. with old `find-function-setup-keys'
+  :group 'find-function :version "31.1")
 
 ;;;###autoload
 (defun find-function-setup-keys ()
-  "Turn on `find-function-mode', which see."
-  (find-function-mode 1))
-(make-obsolete 'find-function-setup-keys 'find-function-mode "31.1")
+  "Define some key bindings for the `find-function' family of functions.
+and also define the keys globally"
+  (find-function-mode 1)
+  (define-key ctl-x-map "F" 'find-function)
+  (define-key ctl-x-4-map "F" 'find-function-other-window)
+  (define-key ctl-x-5-map "F" 'find-function-other-frame)
+  (define-key ctl-x-map "K" 'find-function-on-key)
+  (define-key ctl-x-4-map "K" 'find-function-on-key-other-window)
+  (define-key ctl-x-5-map "K" 'find-function-on-key-other-frame)
+  (define-key ctl-x-map "V" 'find-variable)
+  (define-key ctl-x-4-map "V" 'find-variable-other-window)
+  (define-key ctl-x-5-map "V" 'find-variable-other-frame)
+  (define-key ctl-x-map "L" 'find-library)
+  (define-key ctl-x-4-map "L" 'find-library-other-window)
+  (define-key ctl-x-5-map "L" 'find-library-other-frame))
+
+;;(make-obsolete 'find-function-setup-keys 'find-function-mode "31.1")
 
 (provide 'find-func)
 
-- 
2.46.0.27.gfa3b914457

Message #8 received at 76700 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Madhu <enometh <at> meer.net>
Cc: 76700 <at> debbugs.gnu.org
Subject: Re: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode
Date: Mon, 03 Mar 2025 11:29:05 +0800

Hello,

On Mon 03 Mar 2025 at 08:36am +0530, Madhu wrote:

> I brought up this issue on emacs-devel but the discussion did not
> proceed beyond
> https://lists.gnu.org/archive/html/emacs-devel/2025-02/msg01054.html

I'm still intending to reply to you.  Please be patient.

-- 
Sean Whitton

Message #11 received at 76700 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Madhu <enometh <at> meer.net>, Eshel Yaron <me <at> eshelyaron.com>
Cc: 76700 <at> debbugs.gnu.org
Subject: Re: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode
Date: Sun, 09 Mar 2025 14:07:18 +0800

Hello Madhu, Eshel,

A couple of preliminaries:

(1) Madhu, you make various claims about how you think a minor mode must
    work, but I don't believe you are correct.
    There aren't hard rules in this area, as you seem to think.

(2) Your comments don't cover the actual reasons why I made this change.
    These were, primarily,
    (i)     so that the bindings can be toggled on and off, not just
            turned on
    (ii)    so that the bindings can be turned on and off using the
            customization interface, instead of by manually calling a
            particular function; and
    (iii)   for most people, standard minor mode bindings (i.e. the
            original version of the patch before Eshel commented) are
            more useful.

I think, though, that everyone can have what they want.  I propose to
add a new defcustom which can be used to switch between my original
version of the patch and the one after Eshel's requested changes.

Eshel -- I would like the default to be my original version of the
patch, so you would need to toggle a defcustom.  This is because I think
your workflow is probably the more unusual one.

Let me know how this sounds.

-- 
Sean Whitton

Message #14 received at 76700 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 76700 <at> debbugs.gnu.org, Madhu <enometh <at> meer.net>
Subject: Re: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode
Date: Sun, 09 Mar 2025 07:50:43 +0100

Hi Sean,

Sean Whitton <spwhitton <at> spwhitton.name> writes:

> I think, though, that everyone can have what they want.  I propose to
> add a new defcustom which can be used to switch between my original
> version of the patch and the one after Eshel's requested changes.
>
> Eshel -- I would like the default to be my original version of the
> patch, so you would need to toggle a defcustom.  This is because I think
> your workflow is probably the more unusual one.
>
> Let me know how this sounds.

Thanks for the heads-up, no objections here.  I'd be happy to review the
patch if you want someone to take a look before you install it.


Best,

Eshel

Message #17 received at 76700 <at> debbugs.gnu.org (full text, mbox):

From: Madhu <enometh <at> meer.net>
To: spwhitton <at> spwhitton.name
Cc: 76700 <at> debbugs.gnu.org, me <at> eshelyaron.com, rms <at> gnu.org
Subject: Re: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode
Date: Sun, 09 Mar 2025 15:41:52 +0530 (IST)

*  Sean Whitton <87zfhuhh49.fsf <at> melete.silentflame.com>
Wrote on Sun, 09 Mar 2025 14:07:18 +0800

> A couple of preliminaries:
> (1) Madhu, you make various claims about how you think a minor mode must
>     work, but I don't believe you are correct.
>     There aren't hard rules in this area, as you seem to think.

Sorry Sean, I've tried to explain how the modes are supposed to work,
I believe you were making a mistake out of ignorance of how emacs
works but if you insist on doubling down on your position...

> (2) Your comments don't cover the actual reasons why I made this change.
>     These were, primarily,
>     (i)     so that the bindings can be toggled on and off, not just
>             turned on
>     (ii)    so that the bindings can be turned on and off using the
>             customization interface, instead of by manually calling a
>             particular function; and

You are using the wrong tool for the job.

>     (iii)   for most people, standard minor mode bindings (i.e. the
>             original version of the patch before Eshel commented) are
>             more useful.

It violates the principle of emacs modes which is a conceptual
regression.

I'm cc'ing RMS who authored the original code, perhaps he can review
the earlier messages and confirm this is a wrong use for minor modes
and why he didn't use a minor mode in the first place.


> I think, though, that everyone can have what they want.  I propose to
> add a new defcustom which can be used to switch between my original
> version of the patch and the one after Eshel's requested changes.

Please remember If you have a minor mode the bindings should be on
keymap. the minor mode should not be mucking with global
bindings. Maintainers should be expected to stick to this principle.

> Eshel -- I would like the default to be my original version of the
> patch, so you would need to toggle a defcustom.  This is because I think
> your workflow is probably the more unusual one.

What's wrong with the patch I supplied?  Eshel and his users (me
included) can continue using the original function which is backward
compatible to turn on global bindings. turning find-func mode on and
off will turn the minor mode keymap on and off, as minor modes are
suppose to work.

Message #22 received at 76700-done <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 76700-done <at> debbugs.gnu.org
Subject: Re: bug#76700: 31.0.50; find-func.el: incorrect use of minor mode
Date: Sun, 09 Mar 2025 21:50:49 +0800

Version: 31.1

Hello,

On Sun 09 Mar 2025 at 07:50am +01, Eshel Yaron wrote:

> Hi Sean,
>
> Sean Whitton <spwhitton <at> spwhitton.name> writes:
>
>> I think, though, that everyone can have what they want.  I propose to
>> add a new defcustom which can be used to switch between my original
>> version of the patch and the one after Eshel's requested changes.
>>
>> Eshel -- I would like the default to be my original version of the
>> patch, so you would need to toggle a defcustom.  This is because I think
>> your workflow is probably the more unusual one.
>>
>> Let me know how this sounds.
>
> Thanks for the heads-up, no objections here.  I'd be happy to review the
> patch if you want someone to take a look before you install it.

Okay, implemented and installed that.  I tested it a fair bit so I won't
ask you to, but do let me know if you see any problems.

Please (setopt find-function-mode-lower-precedence t).

-- 
Sean Whitton

Message #25 received at 76700 <at> debbugs.gnu.org (full text, mbox):

From: Madhu <enometh <at> meer.net>
To: 76700 <at> debbugs.gnu.org, help-debbugs <at> gnu.org
Subject: Re: bug#76700: closed
Date: Wed, 12 Mar 2025 05:27:11 +0530 (IST)

*  help-debbugs <at> gnu.org (GNU bug Tracking System) <handler.76700.D76700.174152826314354.notifdone <at> debbugs.gnu.org>
Wrote on Sun, 09 Mar 2025 13:52:02 +0000

> Your bug report
> #76700: 31.0.50; find-func.el: incorrect use of minor mode
> which was filed against the emacs package, has been closed.

Could you please track the continuing changes to the code "fixed" (but
not really fixed ) by the commit which closed the bug here?

This this bug report was closed prematurely before the issues were
resolved, and a worse "solution" was installed

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.