bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

Lars Ingebrigtsen
[hidden email] writes:

> Start emacs with the -Q flag, use the following code to create
> keybindings with define-minor-mode:
>
> (define-minor-mode pdf-continuous-scroll-mode
>   "Emulate continuous scroll with two synchronized buffers"
>   nil
>   " Continuous"
>   '(((kbd "j") . (lambda () (interactive) (print "pushed j")))
>     ((kbd "C-n") . (lambda () (interactive)(print "pushed C-n"))))
>   (print "toggled minor mode"))
>
> Now activate the just defined pdf-continuous-scroll-mode.
> The "C-n" keybinding does not work correctly while the "j"
> keybinding does (i.e. prints "pushed j").

Hm.  It seems like there's a difference between \C-n and (kbd "C-n")
here for some reason.  With this definition:

(define-minor-mode pdf-continuous-scroll-mode-3
  "Emulate continuous scroll with two synchronized buffers"
  nil
  " Continuous"
  '(((kbd "j") . (lambda () (interactive) (print "pushed j")))
    ("\C-n" . (lambda () (interactive)(print "pushed C-n"))))
  (print "toggled minor mode"))

we get the keymap:

(keymap
 (14 lambda nil
     (interactive)
     (print "pushed C-n"))
 (106 lambda nil
      (interactive)
      (print "pushed j")))

With `kbd', we get the following keymap, which surely has to be wrong:

(keymap
 (67 keymap
     (45 keymap
         (110 lambda nil
              (interactive)
              (print "pushed C-n"))))
 (106 lambda nil
      (interactive)
      (print "pushed j")))


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



Reply | Threaded
Open this post in threaded view
|

bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

Lars Ingebrigtsen
Ah, it just works for "j" by accident -- define-minor-mode does not
evaluate anything in the keymap form.

So there's no bug here in define-minor-mode, but the here is wrong.  It
should be:

(define-minor-mode pdf-continuous-scroll-mode-5
  "Emulate continuous scroll with two synchronized buffers"
  nil
  " Continuous"
  `((,(kbd "j") . (lambda () (interactive) (print "pushed j")))
    (,(kbd "C-n") . (lambda () (interactive)(print "pushed C-n"))))
  (print "toggled minor mode"))

So I'm closing this bug report.

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




Reply | Threaded
Open this post in threaded view
|

bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

Drew Adams
> Ah, it just works for "j" by accident -- define-minor-mode does not
> evaluate anything in the keymap form.

It should.  According to its doc string.

> So there's no bug here in define-minor-mode, but the
> here is wrong.  It should be:
>
> (define-minor-mode pdf-continuous-scroll-mode-5
>   "Emulate continuous scroll with two synchronized buffers"
>   nil
>   " Continuous"
>   `((,(kbd "j") . (lambda () (interactive) (print "pushed j")))
>     (,(kbd "C-n") . (lambda () (interactive)(print "pushed C-n"))))
>   (print "toggled minor mode"))
>
> So I'm closing this bug report.

I don't agree that there's no bug (IIUC).

I think there's either a doc bug (if you think
the current behavior is what we want - I don't)
or a behavior bug (if you think the doc's
described behavior is what we want - I do).

The doc string says that KEYMAP can be:

 an expression that returns either a keymap or
 a list of (KEY . BINDING) pairs where KEY and
 BINDING are suitable for `define-key'

The cons ((kbd "C-n") . 'foo) is exactly such a
(KEY . BINDING) pair - both KEY and BINDING are
suitable arguments for `define-key'.

Yes, it's true that the _result of evaluating_
(kbd ...) is ALSO an acceptable arg for
`define-key'.  But `define-key' doesn't _require_
its KEY arg to be, say, a string or vector.
`define-key' evaluates its arg.  And a list
(kbd ...) is an acceptable arg for `define-key'.

You can write

 (define-key map (kbd "C-n") 'foo)

Or you can write

 (define-key map "
" 'foo)

where that string with char ?\n (Control-J) is
the result of evaluating the sexp (kbd "C-n").

Or you can write

 (define-key map "\n" 'foo)

`define-minor-mode' should accept any expression
that `define-key' accepts for KEY.  In the case
of `C-n' that means (kbd "C-n"), "\n", and "
".



Reply | Threaded
Open this post in threaded view
|

bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

Andreas Schwab-2
On Sep 26 2020, Drew Adams wrote:

> The cons ((kbd "C-n") . 'foo) is exactly such a
> (KEY . BINDING) pair - both KEY and BINDING are
> suitable arguments for `define-key'.

Is it?

ELISP> (define-key global-map '(kbd "C-n") ''foo)
*** Eval error ***  Wrong type argument: arrayp, (kbd "C-n")

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."



Reply | Threaded
Open this post in threaded view
|

bug#43617: 27.1; Define-minor-mode keybindings not get precedence over global keymap

dalanicolai
Well it seems quite obvious now that I simply overlooked the fact that quoting a list results in its elements not getting evaluated. I would argue that, although there might be no real bug in the doc, the doc still somehow helped me to overlook this fact. I think backquoting is not very much a hassle, but it would be nice to get reminded about it for when using the (kbd ...) construct. Of course if the (kbd "j") would not have worked I would have been less confused and maybe had found the mistake myself, but because that one did work it appeared to me to be a bug. Anyway, I think a simple change/addition in the docstring and/or the examples in section 23.3.3 of the elisp manual could help make things clearer.

On Sat, 26 Sep 2020 at 18:05, Drew Adams <[hidden email]> wrote:
> > The cons ((kbd "C-n") . 'foo) is exactly such a
> > (KEY . BINDING) pair - both KEY and BINDING are
> > suitable arguments for `define-key'.
>
> Is it?
>
> ELISP> (define-key global-map '(kbd "C-n") ''foo)
> *** Eval error ***  Wrong type argument: arrayp, (kbd "C-n")

I get your point.  I guess maybe there are two ways
to read the doc string.

The most _useful_ behavior for users, IMO, is for
`define-minor-mode' to allow expressions in arg
KEYMAP (when it's such a list) that correspond to
what a user writes in `(define-key ...)'.

Is that particular list form of KEYMAP intended
mostly for programmatically supplying such a list,
or for users to write such a list?

If the former, why is it needed/helpful at all,
since code can just as easily create a keymap arg.
If the latter, it gives users an easy way to write
key bindings directly for `define-minor-mode'.

I hadn't even paid attention to the existence of
such a form for the KEYMAP arg.  But it looks like
it could be handy for users to write - IF the sexp
to write is simple and straightforward.

If users instead need to use backquote syntax or
jump through other hoops to write such a KEYMAP
sexp, then what's the point - what's the use case?

Maybe there _is_ a programmatic use case.  If so,
what is it?