counsel-M-x should not set last-command

[Wilfred]

I've noticed that using counsel-M-x sets last-command. This means that I can't do (if (eq this-command last-command) ...) to see if my command is being called repeatedly.

[abo-abo]

counsel-M-x is supposed to mirror execute-extended-command, which sets last-command for the same reason that you mention.

[Wilfred]

That's not the behaviour I'm seeing:

(global-set-key (kbd "M-x") #'counsel-M-x)
(global-set-key (kbd "C-c M-x") #'execute-extended-command)

(defun wh/report-last-command ()
  (interactive)
  (message "last-command: %s" last-command))

With ivy-mode disabled, C-c M-x wh/report-last-command repeatedly shows the messages:

last-command: other-window
last-command: wh/report-last-command
last-command: wh/report-last-command

M-x wh/report-last-command repeatedly shows the messages:

last-command: self-insert-command
last-command: counsel-M-x
last-command: counsel-M-x

The first invocation looks like counsel-M-x has set last-command based on commands called in the minibuffer, as self-insert-command is because I typed the name of the command. If I move around in the counsel minibuffer (M-x C-n C-p) to select the wh/report-last-command function I get:

last-command: ivy-previous-line

[abo-abo]

I don't why results that you show happen. But I know that this bit is needed:

(let ((prefix-arg current-prefix-arg))
  (setq real-this-command
        (setq this-command (intern cmd)))
  (command-execute (intern cmd) 'record))

Here's my experience: counsel-M-x -> forward-char -> eval-expression (last-command) shows forward-char.

[Wilfred]

Yep, I'm seeing the same as you. eval-expression shows the correct value of last-command, but the command being invoked (wh/report-last-command in this case) does not.

[abo-abo]

The repeat functionality also works, and I've had no complaints about counsel-M-x for a long while.

I don't know why your function doesn't work. Patches for a fix are welcome, but let's make sure the standard functionality still works.

[Wilfred]

I suspect the problem is that we're setting this-command in a callback. When counsel-M-x returns, Emacs does (setq last-command this-command) we end up with last-command being set to counsel-M-x.

Here's one possible patch:

(defun counsel-M-x (&optional initial-input)
  "Ivy version of `execute-extended-command'.
Optional INITIAL-INPUT is the initial input in the minibuffer."
  (interactive)
  (unless initial-input
    (setq initial-input (cdr (assoc this-command
                                    ivy-initial-inputs-alist))))
  (let* ((cands obarray)
         (pred 'commandp)
         (sort t))
    (when (require 'smex nil 'noerror)
      (unless smex-initialized-p
        (smex-initialize))
      (smex-detect-new-commands)
      (smex-update)
      (setq cands smex-ido-cache)
      (setq pred nil)
      (setq sort nil))
    ;; Ensure that `counsel-M-x' is never stored in `last-command',
    ;; for consistency with `execute-extended-command'. The :action
    ;; is invoked after `counsel-M-x' returns, so that isn't sufficient.
    (setq this-command last-command)
    (setq real-this-command real-last-command)

    (ivy-read (counsel--M-x-prompt) cands
              :predicate pred
              :require-match t
              :history 'extended-command-history
              :action
              (lambda (cmd)
                (when (featurep 'smex)
                  (smex-rank (intern cmd)))
                (setq this-command (intern cmd))
                (setq real-this-command (intern cmd))
                (let ((prefix-arg current-prefix-arg))
                  (command-execute (intern cmd) 'record)))
              :sort sort
              :keymap counsel-describe-map
              :initial-input initial-input
              :caller 'counsel-M-x)))

Another option would be to save last-command and real-last-command in counsel-M-x and also set those in the :action callback. What do you think?

[basil-conto]

I'm planning to revert PR #893 to resolve issue #2874.

I've tested with and without the reversion using a snippet like that of @gexplorer in #2874 (comment) :

(defvar warning-minimum-log-level)
(setq warning-minimum-log-level :debug)

(defun counsel--foo (&optional msg)
  (interactive)
  (lwarn 'counsel :debug "%s:
        last %S %S
        this %S %S %S
        curr %S"
         (or msg "`counsel-foo'")
         last-command real-last-command
         this-command real-this-command this-original-command
         (bound-and-true-p current-minibuffer-command)))

(add-hook 'minibuffer-setup-hook
          (lambda () (counsel--foo "Minibuffer setup"))
          -10)

(global-set-key (kbd "<f5>") #'execute-extended-command)
(global-set-key (kbd "<f6>") #'counsel-M-x)
(global-set-key (kbd "<f7>") #'counsel--foo)

I removed the call to counsel-mode in targets/plain.el:

swiper/targets/plain.el

Line 24 in 2257a9d

(counsel-mode)

and in each test run examined the contents of the *Warnings* buffer after the following steps:

1. make plain
2. C-a C-e
3. <f5> counsel--foo RET
4. <f7>
5. C-a C-e
6. <f6> counsel--foo RET
7. <f7>

Here's the *Warnings* output with the current version of counsel-M-x:

Minibuffer setup:
     last move-end-of-line move-end-of-line
     this execute-extended-command execute-extended-command execute-extended-command
     curr execute-extended-command
`counsel-foo':
     last move-end-of-line self-insert-command
     this counsel--foo counsel--foo execute-extended-command
     curr counsel--foo
`counsel-foo':
     last counsel--foo counsel--foo
     this counsel--foo counsel--foo counsel--foo
     curr counsel--foo

Minibuffer setup:
     last move-end-of-line move-end-of-line
     this move-end-of-line move-end-of-line counsel-M-x
     curr counsel-M-x
`counsel-foo':
     last self-insert-command self-insert-command
     this counsel--foo counsel--foo ivy-done
     curr counsel--foo
`counsel-foo':
     last counsel--foo counsel--foo
     this counsel--foo counsel--foo counsel--foo
     curr counsel--foo

The output after reverting PR #893 is identical, save for the currently incorrect values of this-command and real-this-command during minibuffer-setup-hook.

Am I missing something?