bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Drew Adams]

For :keymap, the doc string says "Same as the KEYMAP argument."
Similarly the doc in (elisp) `Defining Minor Modes'.  This does not
appear to be correct.

For positional arg KEYMAP, the doc string says:

  If non-nil, it should be a variable name (whose value is a keymap),
  or an expression that returns either a keymap or a list of
  arguments for `easy-mmode-define-keymap'.
  ...

But the value of :keymap apparently cannot be a variable name whose
value is a keymap or an expression that evaluates to a keymap.  The
:keymap value you pass is apparently not evaluated - unlike other
keyword values such as :group.

So, for example, the `define-minor-mode' fr `follow-mode' uses this:

  :keymap follow-mode-map

and not this:

  :keymap 'follow-mode-map

and not this:

  (let ((mainmap (make-sparse-keymap))
        (map (make-sparse-keymap)))
    (define-key map "\C-v"	'follow-scroll-up)
    ...
    (define-key mainmap follow-mode-prefix map)
    (define-key mainmap [remap end-of-buffer] 'follow-end-of-buffer)
    ...
    mainmap)

In fact, it is not clear from the doc whether and which keyword values
are evaluated.  And shouldn't they all be evaluated or else none be so?


In GNU Emacs 24.5.1 (i686-pc-mingw32)
 of 2015-04-11 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/usr --host=i686-pc-mingw32'

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Drew Adams]

Consider this code.  Evaluate the defvar, and then try to evaluate
each of the define-minor-mode sexps, in turn.  The only difference
among the mode-defining sexps is whether the :group and :keymap
values are quoted or unquoted symbols.

(defvar foo-map  (let ((map  (make-sparse-keymap)))
		   (define-key map "q" 'forward-char)
		   map)
  "...")

(define-minor-mode foo-mode "..."
  :global t :group 'convenience :init-value nil :keymap 'foo-map)

Debugger entered--Lisp error: (error "Invalid keymap foo-map")
  signal(error ("Invalid keymap foo-map"))
  error("Invalid keymap %S" foo-map)

(define-minor-mode foo-mode "..."
  :global t :group convenience :init-value nil :keymap foo-map)

Debugger entered--Lisp error: (void-variable convenience)
  (custom-declare-variable (quote foo-mode) (quote nil) "Non-nil if Foo mode is enabled.\nSee the `foo-mode' command\nfor a description of this minor mode." :set (function custom-set-minor-mode) :initialize (quote custom-initialize-default) :group convenience :type (quote boolean))

(define-minor-mode foo-mode "..."
  :global t :group convenience :init-value nil :keymap 'foo-map)

Same error as previous.

(define-minor-mode foo-mode "..."
  :global t :group 'convenience :init-value nil :keymap foo-map)

Whew!  Success, finally.  But it does not correspond to the doc.
And the behavior is not consistent.

------

How does Emacs's own Lisp code deal with this?  In different ways.

Here is allout.el, for example.  It DOES use a quoted map-variable
(and it jumps through a few hoops).

(defvar allout-mode-map 'allout-mode-map
  "Keybindings place-holder for (allout) outline minor mode.
Do NOT set the value of this variable.  Instead, customize
`allout-command-prefix', `allout-prefixed-keybindings', and
`allout-unprefixed-keybindings'.")

(defvar allout-mode-map-value nil
  "Keymap for allout outline minor mode.
Do NOT set the value of this variable.  Instead, customize
`allout-command-prefix', `allout-prefixed-keybindings', and
`allout-unprefixed-keybindings'.")

;;;_    = make allout-mode-map-value an alias for allout-mode-map:
;; this needs to be revised when the value is changed, sigh.
(defalias 'allout-mode-map allout-mode-map-value)

(defun allout-institute-keymap (map)
  "Associate allout-mode bindings with allout as a minor mode."
  ;; Architecture:
  ;; allout-mode-map var is a keymap by virtue of being a defalias for
  ;; allout-mode-map-value, which has the actual keymap value.
  ;; allout-mode-map's symbol value is just 'allout-mode-map, so it can be
  ;; used in minor-mode-map-alist to indirect to the actual
  ;; allout-mode-map-var value, which can be adjusted and reassigned.
  ;; allout-mode-map-value for keymap reference in various places:
  (setq allout-mode-map-value map)
  ;; the function value keymap of allout-mode-map is used in
  ;; minor-mode-map-alist - update it:
  (fset allout-mode-map allout-mode-map-value))

(define-minor-mode allout-mode "..."
  :lighter " Allout" :keymap 'allout-mode-map
  ...)

Next up: autoarg-mode.  This uses an UNquoted map variable.

(defvar autoarg-mode-map
  (let ((map (make-sparse-keymap)))
    ...
    (define-key map " " 'autoarg-terminate)
    map)
  "Keymap for Autoarg mode.")

(define-minor-mode autoarg-mode "..."
  nil " Aarg" autoarg-mode-map :global t :group 'keyboard)

Those are only the first two grep hits for `define-minor-mode'.

Isn't this more than a doc bug?  Why should the :group value be
evaluated but not the :keymap value?

At any rate, none of the current behavior in this regard is
documented.  Users need to experiment to find out what the
real story is.

Am I missing something?  `define-minor-mode' has been around
since at least Emacs 22.  Has this behavior inconsistency and
missing doc never been noticed before?

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[npostavs]

tags 25505 notabug
quit

Drew Adams <drew.adams@oracle.com> writes:

> For :keymap, the doc string says "Same as the KEYMAP argument."
> Similarly the doc in (elisp) `Defining Minor Modes'.  This does not
> appear to be correct.
>
> For positional arg KEYMAP, the doc string says:
>
>   If non-nil, it should be a variable name (whose value is a keymap),
>   or an expression that returns either a keymap or a list of
>   arguments for `easy-mmode-define-keymap'.
>   ...
>
> But the value of :keymap apparently cannot be a variable name whose
> value is a keymap or an expression that evaluates to a keymap.  The
> :keymap value you pass is apparently not evaluated - unlike other
> keyword values such as :group.

The doc says "variable name" as opposed to "expression", I think it's
clear that expression will be evaluated, and something which is not an
expression will not be evaluated.  I guess we could add "it should be an
unquoted variable name..."

>
> So, for example, the `define-minor-mode' fr `follow-mode' uses this:
>
>   :keymap follow-mode-map

This is correct, follow-mode-map is the name of the variable.

> and not this:
>
>   :keymap 'follow-mode-map

This is not correct, it's an expression which evaluates to the name of
the variable.

>
> and not this:
>
>   (let ((mainmap (make-sparse-keymap))
>         (map (make-sparse-keymap)))
>     (define-key map "\C-v"	'follow-scroll-up)
>     ...
>     (define-key mainmap follow-mode-prefix map)
>     (define-key mainmap [remap end-of-buffer] 'follow-end-of-buffer)
>     ...
>     mainmap)

This is correct, it's an expression which evaluates to a keymap.

> In fact, it is not clear from the doc whether and which keyword values
> are evaluated.  And shouldn't they all be evaluated or else none be so?

I don't see why, it's a macro, some arguments are evaluated, some
aren't.

> Here is allout.el, for example.  It DOES use a quoted map-variable
> (and it jumps through a few hoops).

I don't think it should be doing that, it seems to be using some kind of
needlessly complicated trickery.

> Next up: autoarg-mode.  This uses an UNquoted map variable.

That's correct.

> Those are only the first two grep hits for `define-minor-mode'.

I checked a few more at random, I found no more incorrect cases.

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Drew Adams]

> tags 25505 notabug
> quit
> 
> The doc says "variable name" as opposed to "expression", I think it's
> clear that expression will be evaluated, and something which is not an
> expression will not be evaluated.  I guess we could add "it should be an
> unquoted variable name..."

I don't think the text is clear at all.  As you acknowledge, a macro
can, but need not, evaluate any of its arguments.  This macro evaluates
some and not others (like `setq', `defvar', etc.).  Its doc should be
explicit (aka clear) about which args are evaluated.  It is not clear now.

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Michael Heerdegen]

npostavs@users.sourceforge.net writes:

> The doc says "variable name" as opposed to "expression", I think it's
> clear that expression will be evaluated, and something which is not an
> expression will not be evaluated.  I guess we could add "it should be
> an unquoted variable name..."

I agree with Drew that the paragraph about the KEYMAP arg could be
improved.  Currently we have

| Optional KEYMAP is the default keymap bound to the mode keymap.  If
| non-nil, it should be a variable name (whose value is a keymap), or
| ...

It's not clear that this is about specifying the _name_ of a keymap (a
symbol), in contrast to specifying a keymap (value).

I also agree that the doc could make clearer if and when the arguments
are evaluated.


Michael.

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Noam Postavsky]

On Wed, Feb 15, 2017 at 9:49 AM, Michael Heerdegen
<michael_heerdegen@web.de> wrote:
>
> | Optional KEYMAP is the default keymap bound to the mode keymap.  If
> | non-nil, it should be a variable name (whose value is a keymap), or
> | ...
>
> It's not clear that this is about specifying the _name_ of a keymap (a
> symbol), in contrast to specifying a keymap (value).
>
> I also agree that the doc could make clearer if and when the arguments
> are evaluated.

Ok, so what changes do you have in mind?

bug#25505: 24.5; doc of `define-minor-mode': incorrect for :keymap

[Stefan Kangas]

tags 25505 fixed
close 25505 28.1
thanks

npostavs@users.sourceforge.net writes:

> tags 25505 notabug
> quit
>
> Drew Adams <drew.adams@oracle.com> writes:
>
>> For :keymap, the doc string says "Same as the KEYMAP argument."
>> Similarly the doc in (elisp) `Defining Minor Modes'.  This does not
>> appear to be correct.
>>
>> For positional arg KEYMAP, the doc string says:
>>
>>   If non-nil, it should be a variable name (whose value is a keymap),
>>   or an expression that returns either a keymap or a list of
>>   arguments for `easy-mmode-define-keymap'.
>>   ...
>>
>> But the value of :keymap apparently cannot be a variable name whose
>> value is a keymap or an expression that evaluates to a keymap.  The
>> :keymap value you pass is apparently not evaluated - unlike other
>> keyword values such as :group.
>
> The doc says "variable name" as opposed to "expression", I think it's
> clear that expression will be evaluated, and something which is not an
> expression will not be evaluated.  I guess we could add "it should be an
> unquoted variable name..."

(Noam asked for suggestions here years ago, but there have been none.)

Having read this bug report, I think "unquoted variable name" fixes the
confusion here, so I've made that change on master (commit 293b8c71b2).
This change will be a part of the upcoming Emacs 28.1.

I'm consequently closing this bug report.