bug#5364: 23.1.91; execute-extended-command should do like FFAP

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

OK, how would you execute this command that you see sitting in front of you?
(list-load-path-shadows)

I ended up doing <escape> x l i s t p <backspace>
- <escape> / <return>

That's because I am unable to just put the cursor on it and do M-x, as
for some reason execute-extended-command won't prompt me for it even now
these days. I recall the idea was rejected.

Would you put the cursor after it, and hit C-x C-e?
Well sorry. That will fire up the non interactive version, etc.
In GNU Emacs 23.1.91.1

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> OK, how would you execute this command that you see sitting in front of you?
> (list-load-path-shadows)
>
> I ended up doing <escape> x l i s t p <backspace>
> - <escape> / <return>
>
> That's because I am unable to just put the cursor on it and do M-x, as
> for some reason execute-extended-command won't prompt me for it even now
> these days. I recall the idea was rejected.

The idea of adding a command to pull a string from the original buffer
to the minibuffer was not rejected.  The only problem is that we have
to find a good keybinding.  One proposal was to use `M-.', so when the
cursor is on the string `list-load-path-shadows' you can type

  M-x M-. RET

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

>>>>> "JL" == Juri Linkov <juri@jurta.org> writes:
>> OK, how would you execute this command that you see sitting in front of you?
>> (list-load-path-shadows)
>> for some reason execute-extended-command won't prompt me for it even now
>> these days. I recall the idea was rejected.

JL> The idea of adding a command to pull a string from the original buffer
JL> to the minibuffer was not rejected.  The only problem is that we have
JL> to find a good keybinding.  One proposal was to use `M-.', so when the
JL> cursor is on the string `list-load-path-shadows' you can type

JL>   M-x M-. RET

JL> -- 
JL> Juri Linkov
JL> http://www.jurta.org/emacs/

Put the cursor on emacs-version and hit C-h v. One sees:
Describe variable (default emacs-version):
Now put the cursor upon (list-load-path-shadows) and hit C-h f. One sees:
Describe function (default list-load-path-shadows):
Now put the cursor upon (list-load-path-shadows) and hit M-x. One sees just:
M-x

One should see
Execute function (default list-load-path-shadows):

Currently one can hit TAB to get all the completions, so why not prompt
with the most likely?

JL> One proposal was to use...

That's nice but first implement my idea please.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> Put the cursor on emacs-version and hit C-h v. One sees:
> Describe variable (default emacs-version):
> Now put the cursor upon (list-load-path-shadows) and hit C-h f. One sees:
> Describe function (default list-load-path-shadows):
> Now put the cursor upon (list-load-path-shadows) and hit M-x. One sees just:
> M-x
>
> One should see
> Execute function (default list-load-path-shadows):
>
> Currently one can hit TAB to get all the completions, so why not prompt
> with the most likely?

I think we should close bug#5364 (bug#355) and bug#5214 with one change:

1. Remove a long list of default values from `M-x M-n M-n'.

2. Provide only one default value for M-x when a string under point
is a command name.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

JL> I think we should close bug#5364 (bug#355) and bug#5214 with one change:
It's a deal.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

> Put the cursor on emacs-version and hit C-h v. One sees:
> Describe variable (default emacs-version):
> Now put the cursor upon (list-load-path-shadows) and hit C-h f. One sees:
> Describe function (default list-load-path-shadows):
> Now put the cursor upon (list-load-path-shadows) and hit M-x. One sees just:
> M-x

Note that M-x is very different from the other two, since what it takes
is a command to execute, where the others only take a function/variable
to look up.  It's very common to want to look up the var/fun at point,
but it's a lot less common to want to run the command at point
(many/most commands operate at point, so for most uses of M-x point is
an argument to the command rather than a way to specify which command to
run.  Another way to say it is that it's rather uncommon to want to
apply a command to its name).

As for how I would do what you want: M-b C-M-SPC M-w followed by M-x C-y RET


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

Stefan: Yes but adding our way doesn't hinder your way, It merely adds a
tip which you can ignore. But telling us to use your way,
SM> M-b C-M-SPC M-w followed by M-x C-y RET
is just continuing the stone age punishment for no reason. And if we
never ran into the need over and over, we wouldn't have reported it.
So there.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

> Stefan: Yes but adding our way doesn't hinder your way, It merely adds a
> tip which you can ignore.

An ignorable int is usually OK, but not when it's wrong in 99% of
the cases.  So before accepting such a change I need to be convinced
that it wouldn't bump into false positives too often.

> But telling us to use your way,
> SM> M-b C-M-SPC M-w followed by M-x C-y RET
> is just continuing the stone age punishment for no reason.

I'm not sure if such generally applicable, orthogonal solutions qualify
as "stone age".

> And if we never ran into the need over and over, we wouldn't have
> reported it.  So there.

That's the point I don't understand: how come you bump into it over and
over again?  I can't think of a situation where this would happen more
than once in a blue moon.
I use M-x very often and I can't think of a case where I could have used
such a hint.

So maybe, if you describe the cases where this repeatedly shows up for
you, I can come up with a way to reconcile our difference.


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
SM> if you describe the cases where this repeatedly shows up for
SM> you, I can come up with a way to reconcile our difference.

I sent tons in. And will keep on sending them... however as I am poorly
organized, they end up all over the bug system. Anyway, how about: the
burden is on you to show why it is so bad... what ever it is we were
just discussing... as right now I discovered C-x i (insert-file)
doesn't even know that I want to insert e.g., /etc/motd even though my
cursor is right next to it and I have FFAP turned on. Anyway, that's all
that I'm still good for these days, alerting you fellows to such
shortcomings. Yes, I did want to insert a file right next to its name today.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

SM> if you describe the cases where this repeatedly shows up for
SM> you, I can come up with a way to reconcile our difference.
> I sent tons in. And will keep on sending them... however as I am poorly

Single examples of "text at point" don't help me understand why this
happens *repeatedly*.  If you could describe where those chunks of text
come from and why you end up using them in M-x, maybe that would help.

> organized, they end up all over the bug system. Anyway, how about: the
> burden is on you to show why it is so bad... what ever it is we were

It's bad to have a default in the prompt which is almost always not the
one you want.  People will soon send bug reports about "M-x chooses dumb
defaults".

> just discussing... as right now I discovered C-x i (insert-file)
> doesn't even know that I want to insert e.g., /etc/motd even though my
> cursor is right next to it and I have FFAP turned on. Anyway, that's all
> that I'm still good for these days, alerting you fellows to such
> shortcomings. Yes, I did want to insert a file right next to its name today.

In the pretest code, we've solved this problem by making M-n bring up
the "file at point" if you have ffap loaded (IIUC).  This should work
with all file-reading commands, contrary to FFAP itself which only
works for the commands it redefined.
So after C-x i, try M-n.


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> Single examples of "text at point" don't help me understand why this
> happens *repeatedly*.  If you could describe where those chunks of text
> come from and why you end up using them in M-x, maybe that would help.

After this bug report, I started to notice that most often I want to
put a command name at point to the M-x minibuffer is when I'm looking
at a new package and trying out its commands.

> It's bad to have a default in the prompt which is almost always not the
> one you want.  People will soon send bug reports about "M-x chooses dumb
> defaults".

I agree that a default should not be in the prompt of M-x.

> In the pretest code, we've solved this problem by making M-n bring up
> the "file at point" if you have ffap loaded (IIUC).  This should work
> with all file-reading commands, contrary to FFAP itself which only
> works for the commands it redefined.
> So after C-x i, try M-n.

We could do the same for M-x to let M-n to bring up the command at point.
This single default value will replace the current list of useless
default values reported in bug#5214.

The following patch closes both bug#5364 and bug#5214.  For bug#5364
it adds the command at point when typing `M-x M-n' (but not to the
prompt of M-x).  For bug#5214 it removes a list of confusing random
values for `M-x M-n M-n'.

I know that relying on the `minibuffer-history-variable' being equal
to `extended-command-history' is not a clean solution, but it works,
and currently I see no other way to achieve the same result.

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2010-01-13 08:35:10 +0000
+++ lisp/simple.el	2010-01-14 21:07:12 +0000
@@ -1375,9 +1375,13 @@ (defun minibuffer-default-add-completion
 	(all (all-completions ""
 			      minibuffer-completion-table
 			      minibuffer-completion-predicate)))
-    (if (listp def)
-	(append def all)
-      (cons def (delete def all)))))
+    (if (eq minibuffer-history-variable 'extended-command-history)
+	(with-current-buffer (window-buffer (minibuffer-selected-window))
+	  (and (function-called-at-point)
+	       (format "%S" (function-called-at-point))))
+      (if (listp def)
+	  (append def all)
+	(cons def (delete def all))))))
 
 (defun goto-history-element (nabs)
   "Puts element of the minibuffer history in the minibuffer.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

>> It's bad to have a default in the prompt which is almost always not the
>> one you want.  People will soon send bug reports about "M-x chooses dumb
>> defaults".
> I agree that a default should not be in the prompt of M-x.

I could live with that.

> I know that relying on the `minibuffer-history-variable' being equal
> to `extended-command-history' is not a clean solution, but it works,
> and currently I see no other way to achieve the same result.

The behavior is OK, but the implementation sucks too much.  I think we
can do better via minibuffer-with-setup-hook.  That might require to
turn M-x's interactive spec into Elisp (moving the whole of M-x to Elisp
would be good as well, but it's a much bigger effort).


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> The behavior is OK, but the implementation sucks too much.  I think we
> can do better via minibuffer-with-setup-hook.  That might require to
> turn M-x's interactive spec into Elisp (moving the whole of M-x to Elisp
> would be good as well, but it's a much bigger effort).

This patch moves interactive spec into Elisp and also implements
the following task from comments in execute-extended-command:

  /* This isn't strictly correct if execute-extended-command
     is bound to anything else.  Perhaps it should use
     this_command_keys?  */

It uses `(key-description (this-single-command-keys))' to do this.

=== modified file 'src/keyboard.c'
--- src/keyboard.c	2010-01-13 08:35:10 +0000
+++ src/keyboard.c	2010-01-15 01:09:18 +0000
@@ -10512,7 +10512,30 @@
 
 \f
 DEFUN ("execute-extended-command", Fexecute_extended_command, Sexecute_extended_command,
-       1, 1, "P",
+       2, 2,
+       "(list current-prefix-arg \
+    (minibuffer-with-setup-hook \
+        (lambda () \
+          (set (make-local-variable 'minibuffer-default-add-function) \
+               (lambda () \
+                 (with-current-buffer (window-buffer \
+                                       (minibuffer-selected-window)) \
+                   (and (commandp (function-called-at-point)) \
+                        (format \"%S\" (function-called-at-point))))))) \
+      (completing-read (concat \
+                        (cond \
+                         ((eq current-prefix-arg '-) \"- \") \
+                         ((and (consp current-prefix-arg) \
+                               (eq (car current-prefix-arg) 4)) \"C-u \") \
+                         ((and (consp current-prefix-arg) \
+                               (integerp (car current-prefix-arg))) \
+                          (format \"%d \" (car current-prefix-arg))) \
+                         ((integerp current-prefix-arg) \
+                          (format \"%d \" current-prefix-arg))) \
+                        (key-description (this-single-command-keys)) \
+                        \" \") \
+                       obarray 'commandp t nil \
+                       'extended-command-history)))",
        doc: /* Read function name, then read its arguments and call it.
 
 To pass a numeric argument to the command you are invoking with, specify
@@ -10520,11 +10543,9 @@ (at your option) any later version.
 
 Noninteractively, the argument PREFIXARG is the prefix argument to
 give to the command you invoke, if it asks for an argument.  */)
-     (prefixarg)
-     Lisp_Object prefixarg;
+     (prefixarg, function)
+     Lisp_Object prefixarg, function;
 {
-  Lisp_Object function;
-  char buf[40];
   int saved_last_point_position;
   Lisp_Object saved_keys, saved_last_point_position_buffer;
   Lisp_Object bindings, value;
@@ -10543,32 +10564,8 @@ (at your option) any later version.
 			XVECTOR (this_command_keys)->contents);
   saved_last_point_position_buffer = last_point_position_buffer;
   saved_last_point_position = last_point_position;
-  buf[0] = 0;
   GCPRO3 (saved_keys, prefixarg, saved_last_point_position_buffer);
 
-  if (EQ (prefixarg, Qminus))
-    strcpy (buf, "- ");
-  else if (CONSP (prefixarg) && XINT (XCAR (prefixarg)) == 4)
-    strcpy (buf, "C-u ");
-  else if (CONSP (prefixarg) && INTEGERP (XCAR (prefixarg)))
-    sprintf (buf, "%ld ", (long) XINT (XCAR (prefixarg)));
-  else if (INTEGERP (prefixarg))
-    sprintf (buf, "%ld ", (long) XINT (prefixarg));
-
-  /* This isn't strictly correct if execute-extended-command
-     is bound to anything else.  Perhaps it should use
-     this_command_keys?  */
-  strcat (buf, "M-x ");
-
-  /* Prompt with buf, and then read a string, completing from and
-     restricting to the set of all defined commands.  Don't provide
-     any initial input.  Save the command read on the extended-command
-     history list. */
-  function = Fcompleting_read (build_string (buf),
-			       Vobarray, Qcommandp,
-			       Qt, Qnil, Qextended_command_history, Qnil,
-			       Qnil);
-
 #ifdef HAVE_WINDOW_SYSTEM
   if (hstarted) start_hourglass ();
 #endif

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

> -       1, 1, "P",
> +       2, 2,
> +       "(list current-prefix-arg \
> +    (minibuffer-with-setup-hook \
> +        (lambda () \
> +          (set (make-local-variable 'minibuffer-default-add-function) \
> +               (lambda () \
> +                 (with-current-buffer (window-buffer \
> +                                       (minibuffer-selected-window)) \
> +                   (and (commandp (function-called-at-point)) \
> +                        (format \"%S\" (function-called-at-point))))))) \
> +      (completing-read (concat \
> +                        (cond \
> +                         ((eq current-prefix-arg '-) \"- \") \
> +                         ((and (consp current-prefix-arg) \
> +                               (eq (car current-prefix-arg) 4)) \"C-u \") \
> +                         ((and (consp current-prefix-arg) \
> +                               (integerp (car current-prefix-arg))) \
> +                          (format \"%d \" (car current-prefix-arg))) \
> +                         ((integerp current-prefix-arg) \
> +                          (format \"%d \" current-prefix-arg))) \
> +                        (key-description (this-single-command-keys)) \
> +                        \" \") \
> +                       obarray 'commandp t nil \
> +                       'extended-command-history)))",
>         doc: /* Read function name, then read its arguments and call it.

The Elisp code looks OK, except it should be in the C file.  Please move
it to simple.el where we can edit it with edebug, font-lock, eldoc, ...


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The Elisp code looks OK, except it should be in the C file.  Please move
> it to simple.el where we can edit it with edebug, font-lock, eldoc, ...

BTW, this is post 23.2, I think.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Stefan Monnier]

>> The Elisp code looks OK, except it should be in the C file.  Please move
>> it to simple.el where we can edit it with edebug, font-lock, eldoc, ...
> BTW, this is post 23.2, I think.

Yes, indeed,


        Stefan

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> The Elisp code looks OK, except it should be in the C file.  Please move
> it to simple.el where we can edit it with edebug, font-lock, eldoc, ...

A patch for post-23.2:

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2010-01-13 08:35:10 +0000
+++ lisp/simple.el	2010-01-15 09:14:30 +0000
@@ -1210,6 +1210,29 @@ (defun repeat-complex-command (arg)
       (if command-history
 	  (error "Argument %d is beyond length of command history" arg)
 	(error "There are no previous complex commands to repeat")))))
+
+(defun read-extended-command ()
+  "Read command name to invoke in `execute-extended-command'."
+  (minibuffer-with-setup-hook
+      (lambda ()
+	(set (make-local-variable 'minibuffer-default-add-function)
+	     (lambda ()
+	       (with-current-buffer (window-buffer (minibuffer-selected-window))
+		 (and (commandp (function-called-at-point))
+		      (format "%S" (function-called-at-point)))))))
+    (completing-read
+     (concat (cond
+	      ((eq current-prefix-arg '-) "- ")
+	      ((and (consp current-prefix-arg)
+		    (eq (car current-prefix-arg) 4)) "C-u ")
+	      ((and (consp current-prefix-arg)
+		    (integerp (car current-prefix-arg)))
+	       (format "%d " (car current-prefix-arg)))
+	      ((integerp current-prefix-arg)
+	       (format "%d " current-prefix-arg)))
+	     (key-description (this-single-command-keys))
+	     " ")
+     obarray 'commandp t nil 'extended-command-history)))
 \f
 (defvar minibuffer-history nil
   "Default minibuffer history list.

=== modified file 'src/keyboard.c'
--- src/keyboard.c	2010-01-13 08:35:10 +0000
+++ src/keyboard.c	2010-01-15 09:17:15 +0000
@@ -10512,19 +10512,17 @@ (at your option) any later version.
 
 \f
 DEFUN ("execute-extended-command", Fexecute_extended_command, Sexecute_extended_command,
-       1, 1, "P",
-       doc: /* Read function name, then read its arguments and call it.
+       2, 2, "(list current-prefix-arg (read-extended-command))",
+       doc: /* Read arguments, then read FUNCTION name and call it.
 
 To pass a numeric argument to the command you are invoking with, specify
 the numeric argument to this command.
 
 Noninteractively, the argument PREFIXARG is the prefix argument to
 give to the command you invoke, if it asks for an argument.  */)
-     (prefixarg)
-     Lisp_Object prefixarg;
+     (prefixarg, function)
+     Lisp_Object prefixarg, function;
 {
-  Lisp_Object function;
-  char buf[40];
   int saved_last_point_position;
   Lisp_Object saved_keys, saved_last_point_position_buffer;
   Lisp_Object bindings, value;
@@ -10543,32 +10541,8 @@ (at your option) any later version.
 			XVECTOR (this_command_keys)->contents);
   saved_last_point_position_buffer = last_point_position_buffer;
   saved_last_point_position = last_point_position;
-  buf[0] = 0;
   GCPRO3 (saved_keys, prefixarg, saved_last_point_position_buffer);
 
-  if (EQ (prefixarg, Qminus))
-    strcpy (buf, "- ");
-  else if (CONSP (prefixarg) && XINT (XCAR (prefixarg)) == 4)
-    strcpy (buf, "C-u ");
-  else if (CONSP (prefixarg) && INTEGERP (XCAR (prefixarg)))
-    sprintf (buf, "%ld ", (long) XINT (XCAR (prefixarg)));
-  else if (INTEGERP (prefixarg))
-    sprintf (buf, "%ld ", (long) XINT (prefixarg));
-
-  /* This isn't strictly correct if execute-extended-command
-     is bound to anything else.  Perhaps it should use
-     this_command_keys?  */
-  strcat (buf, "M-x ");
-
-  /* Prompt with buf, and then read a string, completing from and
-     restricting to the set of all defined commands.  Don't provide
-     any initial input.  Save the command read on the extended-command
-     history list. */
-  function = Fcompleting_read (build_string (buf),
-			       Vobarray, Qcommandp,
-			       Qt, Qnil, Qextended_command_history, Qnil,
-			       Qnil);
-
 #ifdef HAVE_WINDOW_SYSTEM
   if (hstarted) start_hourglass ();
 #endif

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> This patch moves interactive spec into Elisp and also implements
> the following task from comments in execute-extended-command:
>
>   /* This isn't strictly correct if execute-extended-command
>      is bound to anything else.  Perhaps it should use
>      this_command_keys?  */
>
> It uses `(key-description (this-single-command-keys))' to do this.

Actually, it occured to me that displaying a key other than "M-x"
in the prompt of `execute-extended-command' is too confusing.

For instance, in bindings.el `execute-extended-command' is bound to the
[menu] key.  When I accidentally type the <menu> key, the first reaction
is "What does this mean?!" because it's very strange to see the prompt
"<menu> " waiting for a command.  It looks like a beginning of a key
sequence for the main menu.  OTOH, a well-known prompt "M-x" means
it reads an extended command.  I've added a comment that explains
why "M-x" is better than anything else.

Also we should keep the current function signature of
`execute-extended-command' unchanged and not to add a new arg
for two reasons:

1. There are places in code that call `execute-extended-command'
with one argument.

2. Calling `read-extended-command' to read a command name should not be
in the `interactive' spec because it needs to remember the hourglass
status before reading a command name (using `hourglass_started'),
and restore the hourglass (using `start_hourglass') after that.

So I installed a patch that calls `read-extended-command'
in the middle of `execute-extended-command'.

Of course, moving the whole of `execute-extended-command' to Elisp
would be better but the main obstacle is the hourglass functions
that have no Elisp interface.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Jan D.]

On 2010-01-12 21:54, Juri Linkov wrote:
>> OK, how would you execute this command that you see sitting in front of you?
>> (list-load-path-shadows)
>>
>> I ended up doing<escape>  x l i s t p<backspace>
>> -<escape>  /<return>
>>
>> That's because I am unable to just put the cursor on it and do M-x, as
>> for some reason execute-extended-command won't prompt me for it even now
>> these days. I recall the idea was rejected.
>
> The idea of adding a command to pull a string from the original buffer
> to the minibuffer was not rejected.  The only problem is that we have
> to find a good keybinding.  One proposal was to use `M-.', so when the
> cursor is on the string `list-load-path-shadows' you can type
>
>    M-x M-. RET
>

Hmm, why don't to the same as C-h f does, offer the function as a 
default?  I think that works quite well.

	Jan D.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

And M-x occur doesn't help us out by prompting with the words under the
cursor too.

Compare M-x rgrep.

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[Juri Linkov]

> And M-x occur doesn't help us out by prompting with the words under the
> cursor too.

For historical reasons, `M-x occur' provides the last history item as
the default.  There was much resistance when we tried to change this :)

However, you can type `M-x occur M-n' to put the word under the cursor
to the minibuffer.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#5364: 23.1.91; execute-extended-command should do like FFAP

[jidanni]

Do M-x apropos something
See any commands you want to try?
Just put the cursor on it and hit M-x RET
Bzzzt. No go.

OK, I will try the new M-n stuff next Debian emacs-snapshot. Thanks.