C-h k, C-h f and keyboard macros: Patch.

C-h k, C-h f and keyboard macros: Patch.

[Luc Teirlinck]

Currently, `describe-key' (usually C-h k) and `describe-function'
(usually C-h f) produce confusing output when confronted with a
keyboard macro.  Bind any key to a keyboard macro and do C-h k.  The
output looks like:

    C-c 3 runs the command ott
       which is a keyboard macro.
    It is bound to C-c 3.
    [Missing arglist.  Please make a bug report.]

    Keyboard macro.

There is no missing arglist.  The following patch would replace the
confusing "[Missing arglist.  Please make a bug report.]" with a
description of the macro, provided by `kmacro-display'.

New C-h k output:

    C-c 3 runs the command ott
       which is a keyboard macro.
    It is bound to C-c 3.
    Macro: SPC one SPC two SPC three SPC

    Keyboard macro.

It also seems to work well for keys bound directly to the macro
definition:

    C-c d runs the command [24 100 126]
       which is a keyboard macro.
    It is bound to C-c d.
    Macro: C-x d ~

    Keyboard macro.

In other words, we get the "readable" and the "formal" definition,
yielding no duplication in this case.

An alternative to printing the `kmacro-display' output would be to
just erase the offending line and not replace it with anything, but I
find the `kmacro-display' output useful in situations where I do
C-h k.  The patch affects C-h f in the same way as C-h k.

Change log:

2003-02-10  Luc Teirlinck  <teirllm@mail.auburn.edu>

        * help-fns.el (describe-function-1): Change output for
          keyboard macros.

Patch:

===File ~/help-fns-diff=====================================
cd /usr/local/share/emacs/21.3.50/lisp/
diff -c /usr/local/share/emacs/21.3.50/lisp/help-fns.el /usr/local/share/emacs/21.3.50/lisp/help-fns.new.el
*** /usr/local/share/emacs/21.3.50/lisp/help-fns.el	Tue Feb  4 22:37:29 2003
--- /usr/local/share/emacs/21.3.50/lisp/help-fns.new.el	Mon Feb 10 16:22:57 2003
***************
*** 293,299 ****
      (when (commandp function)
        (let* ((remapped (remap-command function))
  	     (keys (where-is-internal
! 		   (or remapped function) overriding-local-map nil nil)))
  	(when remapped
  	  (princ "It is remapped to `")
  	  (princ (symbol-name remapped))
--- 293,299 ----
      (when (commandp function)
        (let* ((remapped (remap-command function))
  	     (keys (where-is-internal
! 		    (or remapped function) overriding-local-map nil nil)))
  	(when remapped
  	  (princ "It is remapped to `")
  	  (princ (symbol-name remapped))
***************
*** 323,328 ****
--- 323,331 ----
  						 function)))))
  		   usage)
  		 (car usage))
+ 		((or (stringp def)
+ 		     (vectorp def))
+ 		 (kmacro-display def))
  		(t "[Missing arglist.  Please make a bug report.]")))
  	(terpri))
        (let ((obsolete (and

Diff finished at Mon Feb 10 16:47:18
============================================================

Re: C-h k, C-h f and keyboard macros: Patch.

[Juanma Barranquero]

On 11 Feb 2003 14:10:42 +0100, storm@cua.dk (Kim F. Storm) wrote:

> GREAT IDEA!!

I also thing it's a great idea.

Trying it, though, has unveiled a bug with local keymaps and
`lookup-key':

emacs -q --no-site-file

M-x ielm

*** Welcome to IELM ***  Type (describe-mode) for help.
ELISP> (remap-command [home])
nil
ELISP> (cua-mode 1)
nil
ELISP> (remap-command [home])
*** Eval error ***  Key sequence contains invalid event
ELISP> 

If current_minor_maps (at keymap.c, line 1456) returns non-zero, the
loop is entered and the first call to Flookup_key at line 1463 fails.


                                                           /L/e/k/t/u

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Currently, `describe-key' (usually C-h k) and `describe-function'
> (usually C-h f) produce confusing output when confronted with a
> keyboard macro.

> New C-h k output:
> 
>     C-c 3 runs the command ott
>        which is a keyboard macro.
>     It is bound to C-c 3.
>     Macro: SPC one SPC two SPC three SPC
> 
>     Keyboard macro.
> 
> In other words, we get the "readable" and the "formal" definition,
> yielding no duplication in this case.

GREAT IDEA!!

> 
> An alternative to printing the `kmacro-display' output would be to
> just erase the offending line and not replace it with anything, but I
> find the `kmacro-display' output useful in situations where I do
> C-h k.  The patch affects C-h f in the same way as C-h k.

The problem with using kmacro-display is that it is intended for
interactive use, so it prints the macro definition using `message'.
It is better to use format-kbd-macro directly:

Index: help-fns.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/help-fns.el,v
retrieving revision 1.28
diff -c -r1.28 help-fns.el
*** help-fns.el	4 Feb 2003 11:23:06 -0000	1.28
--- help-fns.el	11 Feb 2003 12:10:17 -0000
***************
*** 323,328 ****
--- 323,331 ----
  						 function)))))
  		   usage)
  		 (car usage))
+  		((or (stringp def)
+  		     (vectorp def))
+ 		 (format "\nMacro: %s" (format-kbd-macro def)))
  		(t "[Missing arglist.  Please make a bug report.]")))
  	(terpri))
        (let ((obsolete (and

Re: C-h k, C-h f and keyboard macros: Patch.

[Juanma Barranquero]

On 11 Feb 2003 15:13:47 +0100, storm@cua.dk (Kim F. Storm) wrote:

> you are trying to run (key-binding [remap [home]] nil t)
> which correctly reports a "Key sequence contains invalid event"
> error (but only when there are any remap entries in one of the
> active keymaps -- as there are when you enable cua-mode).

Oh, I don't doubt you're right, but, from the point of view of a user,
it's weird that asking for a remaping says nil when there aren't remap
entries, and error when there are...

> The problem I can see is that remap-command expects a SYMBOL as the
> argument.  All callers in C checks for this, but from Lisp, there is
> no such check.  I'll add one.

Thanks.

> So 
> 
> > ELISP> (cua-mode 1)
> > ELISP> (remap-command [home])
> 
> should have failed with a "wrong type argument" error.

This will make Luc's patch fail for me...

> But how was this related to the C-h k macro patch??

After Luc's (or your's) patch, with cua-mode enabled, if I do:

[f3] "pepe" home [f4]     ; define macro that inserts "pepe" and goes to
                          ; the beginning of line
C-x C-k b M-z             ; assign it to M-z
C-h k M-z                 ; see it

I get "Key sequence contains invalid event", not the C-h k output.


                                                           /L/e/k/t/u

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

Juanma Barranquero <lektu@terra.es> writes:
> 
> Trying it, though, has unveiled a bug with local keymaps and
> `lookup-key':

> ELISP> (remap-command [home])
*** Eval error ***  Key sequence contains invalid event

The call `(remap-command COMMAND)' is equivalent to
the call `(key-binding [remap COMMAND] nil t)', so in your case,
you are trying to run (key-binding [remap [home]] nil t)
which correctly reports a "Key sequence contains invalid event"
error (but only when there are any remap entries in one of the
active keymaps -- as there are when you enable cua-mode).

This reveals that key-binding only validates as much of the key
sequence as is matched by the active keymaps, but I consider that a
minor issue.

The problem I can see is that remap-command expects a SYMBOL as the
argument.  All callers in C checks for this, but from Lisp, there is
no such check.  I'll add one.


So 

> ELISP> (cua-mode 1)
> ELISP> (remap-command [home])

should have failed with a "wrong type argument" error.


But how was this related to the C-h k macro patch??


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-h k, C-h f and keyboard macros: Patch.

[Juanma Barranquero]

On 11 Feb 2003 16:18:00 +0100, storm@cua.dk (Kim F. Storm) wrote:

> I see it now, thanks!

I'm glad, I was starting to believe it was my Emacs... :)

> Here is a version of the patch which fixes this problem:

Works great. Thanks!

                                                           /L/e/k/t/u


-- 
  "'Cowardly' is not an adverb, although it looks like one. It is an
adjective. It makes a statement about general temperament, rather than
a specific occasion. I don't think Emacs has a general temperament."
  "Mine does."
                      -- RMS and Eli Zaretskii, in emacs-devel@gnu.org

Re: C-h k, C-h f and keyboard macros: Patch.

[Juanma Barranquero]

On 11 Feb 2003 16:36:41 +0100, storm@cua.dk (Kim F. Storm) wrote:

> Thinking more about it, I would argue that since remap-command may in
> practice be applied to anything that is found in a keymap (as was the
> case here), it would be better to just return nil if the argument is
> not a symbol.

So, you're finally agreeing with my initial bug report :-P

> So with latest CVS head, the previous version of the patch is still ok:

Ok, thanks (again).


                                                           /L/e/k/t/u

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

Juanma Barranquero <lektu@terra.es> writes:

> After Luc's (or your's) patch, with cua-mode enabled, if I do:
> 
> [f3] "pepe" home [f4]     ; define macro that inserts "pepe" and goes to
>                           ; the beginning of line
> C-x C-k b M-z             ; assign it to M-z
> C-h k M-z                 ; see it
> 
> I get "Key sequence contains invalid event", not the C-h k output.

I see it now, thanks!

Here is a version of the patch which fixes this problem:

Index: help-fns.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/help-fns.el,v
retrieving revision 1.28
diff -c -r1.28 help-fns.el
*** help-fns.el	4 Feb 2003 11:23:06 -0000	1.28
--- help-fns.el	11 Feb 2003 14:16:50 -0000
***************
*** 291,297 ****
      (princ ".")
      (terpri)
      (when (commandp function)
!       (let* ((remapped (remap-command function))
  	     (keys (where-is-internal
  		   (or remapped function) overriding-local-map nil nil)))
  	(when remapped
--- 291,297 ----
      (princ ".")
      (terpri)
      (when (commandp function)
!       (let* ((remapped (and (symbolp function) (remap-command function)))
  	     (keys (where-is-internal
  		   (or remapped function) overriding-local-map nil nil)))
  	(when remapped
***************
*** 323,328 ****
--- 323,331 ----
  						 function)))))
  		   usage)
  		 (car usage))
+  		((or (stringp def)
+  		     (vectorp def))
+ 		 (format "\nMacro: %s" (format-kbd-macro def)))
  		(t "[Missing arglist.  Please make a bug report.]")))
  	(terpri))
        (let ((obsolete (and



-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

> 
> Here is a version of the patch which fixes this problem:
> 
> !       (let* ((remapped (and (symbolp function) (remap-command function)))

Thinking more about it, I would argue that since remap-command may in
practice be applied to anything that is found in a keymap (as was the
case here), it would be better to just return nil if the argument is
not a symbol.  I have just committed that change!

So with latest CVS head, the previous version of the patch is still ok:

Index: help-fns.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/help-fns.el,v
retrieving revision 1.28
diff -c -r1.28 help-fns.el
*** help-fns.el	4 Feb 2003 11:23:06 -0000	1.28
--- help-fns.el	11 Feb 2003 14:36:31 -0000
***************
*** 323,328 ****
--- 323,331 ----
  						 function)))))
  		   usage)
  		 (car usage))
+  		((or (stringp def)
+  		     (vectorp def))
+ 		 (format "\nMacro: %s" (format-kbd-macro def)))
  		(t "[Missing arglist.  Please make a bug report.]")))
  	(terpri))
        (let ((obsolete (and


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-h k, C-h f and keyboard macros: Patch.

[Luc Teirlinck]

Kim Storm wrote:

   The problem with using kmacro-display is that it is intended for
   interactive use, so it prints the macro definition using `message'.
   It is better to use format-kbd-macro directly:

Using format-kbd-macro directly is indeed a much better idea than
using kmacro-display.  `kmacro-display' also has the additional
problem of not being autoloaded.  (I should have thought of that
earlier.)  `format-kbd-macro' is autoloaded.

Sincerely,

Luc.

Re: C-h k, C-h f and keyboard macros: Patch.

[Stefan Monnier]

> The call `(remap-command COMMAND)' is equivalent to
> the call `(key-binding [remap COMMAND] nil t)', so in your case,
> you are trying to run (key-binding [remap [home]] nil t)
> which correctly reports a "Key sequence contains invalid event"
> error (but only when there are any remap entries in one of the
> active keymaps -- as there are when you enable cua-mode).

I think it shouldn't report an error but just return nil.
Checking that events are "meaningful" might be OK for define-key but
doesn't make any sense for lookup-key and friends.
If you look at the test used currently, it is already more lax in
lookup-key than in define-key (justified by the following comment:)

      /* Allow string since binding for `menu-bar-select-buffer'
	 includes the buffer name in the key sequence.  */

I suggest we just get rid of the checks in lookup-key.


	Stefan

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:

> 
> I suggest we just get rid of the checks in lookup-key.

That's ok with me.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: C-h k, C-h f and keyboard macros: Patch.

[Richard Stallman]

It looks good to me.

Re: C-h k, C-h f and keyboard macros: Patch.

[Richard Stallman]

The function name `remap-command' is misleading.
It has the form VERB-NOUN, which means that it does VERB to NOUN.
This would be a good name for a function that defines a mappimg.

The doc string suggests it is actually a function to inquire
about the consequences of current keymap contents.  That must
have a different name.

Would someone please rename it to `command-remapping'?

Re: C-h k, C-h f and keyboard macros: Patch.

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> The function name `remap-command' is misleading.
> It has the form VERB-NOUN, which means that it does VERB to NOUN.
> This would be a good name for a function that defines a mappimg.
> 
> The doc string suggests it is actually a function to inquire
> about the consequences of current keymap contents.  That must
> have a different name.
> 
> Would someone please rename it to `command-remapping'?

Done.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk