helm-complex-command-history and interactivity

[michael-heerdegen]

Hi,

the current implementation of the helm-complex-command-history uses just the sexp type that comes with actions that are more or less the same as eval. In the case of helm-complex-command-history, we need (a bit) more: the called command must know that it was called interactively. With the current implementation, any called-interactively-p test (inside the command's defun) fails, and this-command is not updated accordingly, etc. The so repeated command behaves as it was called from Lisp, but it should behave as it was envoked normally, i.e., interactively.

In vanilla repeat-complex-command, there is a simple hack (search for "trick" in its defun) that solves that problem. Can we use this as well?

[thierryvolpiatto]

Hi Michael,

michael-heerdegen notifications@github.com writes:

Hi,

the current implementation of the helm-complex-command-history uses
just the sexp type that comes with actions that are more or less the
same as eval. In the case of helm-complex-command-history, we need (a
bit) more: the called command must know that it was called
interactively. With the current implementation, any
called-interactively-p test (inside the command's defun) fails, and
this-command is not updated accordingly, etc. The so repeated command
behaves as it was called from Lisp, but it should behave as it was
envoked normally, i.e., interactively.

In vanilla repeat-complex-command, there is a simple hack (search for
"trick" in its defun) that solves that problem. Can we use this as
well?

Could you provide a small recipe showing the current behavior, what it
is (not) doing, and what you expect instead. (I use rarely this or not
at all)

Thanks.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Just try the simple example from the bug report of the according vanilla bug 14136:

https://lists.gnu.org/archive/html/emacs-bug-tracker/2013-08/msg00161.html

All commands that do a called-interactively-p test are problematic.

There was some dired command where this problem often bites me, but I don't remeber now.

Another example: I use this code to search e.g. for cl.el stuff in my libs:

(require 'cl-lib)

(defvar lib-stuff-find-current-regexp)

(defvar lib-stuff-find-temp-map
  (let ((map (make-sparse-keymap)))
    (define-key map [?\ ] #'lib-stuff-find-redo)
    map))

(defun lib-stuff-find (regexp)
  "Search for symbols whoose symbol file is matched by regexp."
  (interactive "sRegexp: ")
  (setq lib-stuff-find-current-regexp regexp)
  (let ((file)
        (reporter (make-progress-reporter "Searching "
                                          (point-min) (point-max) (point))))
    (forward-symbol 1)
    (while (not (or (eobp)
            (and (not (nth 4 (syntax-ppss))) ;inside a comment
             (not (nth 3 (syntax-ppss))) ;inside a string
             (setq file (symbol-file (symbol-at-point)))
             (setq file (and file (file-name-sans-extension
                                               (file-name-nondirectory file))))
             (string-match regexp file))))
      (progress-reporter-update reporter (point))
      (forward-symbol 1))
    (if (eobp)
    (progn (message "Nothing found") nil)
      (forward-symbol -1)
      (set-transient-map lib-stuff-find-temp-map t)
      (message "%s: %s" (file-name-sans-extension file) (symbol-at-point))
      (when (called-interactively-p 'any)
    (sit-for 2)
    (with-temp-message
        (substitute-command-keys
         "\\<lib-stuff-find-temp-map>\\[lib-stuff-find-redo] to repeat search")
      (sit-for 2)))
      (symbol-at-point))))

(defun lib-stuff-find-redo ()
  (interactive)
  (forward-symbol 1)
  (lib-stuff-find lib-stuff-find-current-regexp))

(Sorry, github doesn't indent correctly.)

When I use this, then make some edits, and want to search further with the very same regexp via helm-complex-command-history, the command lib-stuff-find doesn't consider itself as having been called interactively and doesn't set the temporary overlay map that allows hitting SPC to repeat.

[michael-heerdegen]

And yes, the problem is a bit exotic and triggered not very often, but it's annoying every time it happens, and you can't completely avoid it.

[michael-heerdegen]

BTW, 500db39 is not kosher:

• If you bind helm-complex-command-history to a key (I do), it is not added to command-history, and the cdr will skip something else.
• Wouldn't delq not make even more sense than cdr? I.e., shouldn't we remove all occurrences of helm-complex-command-history? Hiding just the current occurrance seems somewhat inconsistent. This is no performance issue, since we already mapcar over the whole history.

(Or should we even avoid adding helm-complex-command-history to command-history?)

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Just try the simple example from the bug report of the according vanilla bug 14136:

https://lists.gnu.org/archive/html/emacs-bug-tracker/2013-08/msg00161.html

It is working now for this one.

All commands that do a called-interactively-p test are problematic.

Don't use it in your functions?

(defun lib-stuff-find ;(regexp)
(regexp &optional arg)
"Search for symbols whoose symbol file is matched by regexp."
;(interactive "sRegexp: ")
(interactive "sRegexp: \np")
(setq lib-stuff-find-current-regexp regexp)
(let ((file)
(reporter (make-progress-reporter "Searching "
(point-min) (point-max) (point))))
(forward-symbol 1)
(while (not (or (eobp)
(and (not (nth 4 (syntax-ppss))) ;inside a comment
(not (nth 3 (syntax-ppss))) ;inside a string
(setq file (symbol-file (symbol-at-point)))
(setq file (and file (file-name-sans-extension
(file-name-nondirectory file))))
(string-match regexp file))))
(progress-reporter-update reporter (point))
(forward-symbol 1))
(if (eobp)
(progn (message "Nothing found") nil)
(forward-symbol -1)
(set-transient-map lib-stuff-find-temp-map t)
(message "%s: %s" (file-name-sans-extension file) (symbol-at-point))
(when arg ;(called-interactively-p 'any)
(sit-for 2)
(with-temp-message
(substitute-command-keys
"<lib-stuff-find-temp-map>[lib-stuff-find-redo] to repeat search")
(sit-for 2)))
(symbol-at-point))))

When I use this, then make some edits, and want to search further with
the very same regexp via helm-complex-command-history, the command
lib-stuff-find doesn't consider itself as having been called
interactively and doesn't set the temporary overlay map that allows
hitting SPC to repeat.

Yes this is a problem, not fixed already, I am unable for now to setup a
working function for `called-interactively-p-functions'.
I am not sure though, that when done it will fix your problem.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

https://lists.gnu.org/archive/html/emacs-bug-tracker/2013-08/msg00161.html

It is working now for this one.

Thanks. Hhm, but the message (of the word count) is instantly erased,
so it's only visible afterwards in Messages.

All commands that do a called-interactively-p test are problematic.

Don't use it in your functions?

Yes, it's possible to avoid to trigger that bug, no doubt. Not nice,
anyhow, to be constrained in programming.

BTW, I also don't like the vanilla fix much.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Thierry Volpiatto notifications@github.com writes:

https://lists.gnu.org/archive/html/emacs-bug-tracker/2013-08/msg00161.html

It is working now for this one.

Thanks. Hhm, but the message (of the word count) is instantly erased,
so it's only visible afterwards in Messages.

Hmm, here it is working as expected, can you figure out why the message is
erased ?

All commands that do a called-interactively-p test are problematic.

Don't use it in your functions?

Yes, it's possible to avoid to trigger that bug, no doubt. Not nice,
anyhow, to be constrained in programming.

Agreed.

BTW, I also don't like the vanilla fix much.

Same here.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

Thanks. Hhm, but the message (of the word count) is instantly
erased, so it's only visible afterwards in Messages.

Hmm, here it is working as expected, can you figure out why the
message is erased ?

Correction: count-words is working as expected.

What I tested was count-words-region. And for this, the message is not
erased, it's just not messaged. Which is expected, because your patch
doesn't yet take care of this case.

[michael-heerdegen]

Looking at the definition of `called-interactively-p', which analyses
backtrace frames, I don't see any other approach to fix this other than
the way it was fixed in Emacs. I don't know any way to call the command
from Lisp so that the backtrace frames look the same as when it was
called interactively.

I think the situation would be nicer when the vanilla code would be
better factored, and the relevant part

(unwind-protect
    (progn
      ;; Trick called-interactively-p into thinking that `newcmd' is
      ;; an interactive call (bug#14136).
      (add-hook 'called-interactively-p-functions
                #'repeat-complex-command--called-interactively-skip)
      (eval newcmd))
  (remove-hook 'called-interactively-p-functions
               #'repeat-complex-command--called-interactively-skip))

would be exposed as a function.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Looking at the definition of `called-interactively-p', which analyses
backtrace frames, I don't see any other approach to fix this other than
the way it was fixed in Emacs. I don't know any way to call the command
from Lisp so that the backtrace frames look the same as when it was
called interactively.

I have fixed this issue but it have several problems:

• This approach used by emacs is ugly.
• It do not work with lexical code compiled (our case)
• It is not backward compatible, i.e backtrace-frame have to accept 2
  args.

So I will not commit it yet.
I will create a branch to let you try it (Only 24.4).

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[thierryvolpiatto]

The branch is "complex-command-history"

[michael-heerdegen]

FYI: I've read your patch, but had no useful comments about it. I would also like to avoid doing this. But I would not take backwards compatibility too seriously, since older versions of Emacs have the bug even without using helm.

[thierryvolpiatto]

We could use now the compatibility function Stefan sent (and you fixed) for compatibility and use the real fix for emacs-24.5 once commited WDYT ?

[thierryvolpiatto]

See #508

[michael-heerdegen]

Yes, that's what I thought, too. It's much better than the other hack.

But this only fixes called-interactively-p calls. References to this-command and last-command will still not work, I think, right? BTW, did you test if this-command is set accordingly with Stefans patch for inclusion?

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Yes, that's what I thought, too. It's much better than the other hack.

But this only fixes called-interactively-p calls. References to
this-command and last-command will still not work, I think, right?

I don't know what references you are talking about.

BTW, did you test if this-command is set accordingly with Stefans
patch for inclusion?

How ?
In which use case do we need this-command ?

I don't see the point for complex-command-history.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

BTW, did you test if this-command is set accordingly with Stefans
patch for inclusion?

How ?
In which use case do we need this-command ?

I don't see the point for complex-command-history.

Some commands need to behave differently depending on the command that
had been invoked directly before themselves (yank-pop, for example).
They can e.g. test whether they have been called repeatedly (eq
this-command last-command) or whether the last command was a yanking
command etc.

When we repeat a command from complex-command-history, this-command and
last-command must be updated according to it, else, the directly
following command might not behave correctly when it refers to
last-command.

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Thierry Volpiatto notifications@github.com writes:

BTW, did you test if this-command is set accordingly with Stefans
patch for inclusion?

How ?
In which use case do we need this-command ?

I don't see the point for complex-command-history.

Some commands need to behave differently depending on the command that
had been invoked directly before themselves (yank-pop, for example).
They can e.g. test whether they have been called repeatedly (eq
this-command last-command) or whether the last command was a yanking
command etc.

When we repeat a command from complex-command-history, this-command and
last-command must be updated according to it, else, the directly
following command might not behave correctly when it refers to
last-command.

Isn't `call-interactively' taking care of this ?

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

Thierry Volpiatto notifications@github.com writes:

Isn't `call-interactively' taking care of this ?

Actually, nothing is taking care of this. See my last message in the
bug's thread.

[thierryvolpiatto]

I don't use funcall-interactively anymore, it is failing in many places and sometimes return
unpredictables results when args are numbers.

Now it is working for most things (emacs 24.4 only).
tested on:
count-words => Ok
count-words-region => Ok
lib-stuff-find => Ok (Able to repeat with SPC)
(pp-eval-expression (quote (eval 122 t))) => Ok (Don't prompt with nothing)

[michael-heerdegen]

Ok, thanks for the info (and thanks for your work).

BTW, why is dont-compile necessary? I would expect that with compiling
(even with lexical binding on) the backtrace frames are different, but
why can't we look at them (just courious)?

What about the obsoleteness of dont-compile? AFAICT, we could just
replace the dont-compile expression with its macro expansion (eval +
quote).

[thierryvolpiatto]

michael-heerdegen notifications@github.com writes:

Ok, thanks for the info (and thanks for your work).

BTW, why is dont-compile necessary? I would expect that with compiling
(even with lexical binding on) the backtrace frames are different, but
why can't we look at them (just courious)?

We can look at them in both cases, but they are different, so
dont-compile ensure we will have always the same number of BT to reach
interactivity (i + 2).

What about the obsoleteness of dont-compile? AFAICT, we could just
replace the dont-compile expression with its macro expansion (eval +
quote).

Well, for now it is ok, obsolete warnings are disabled with local var,
we may inline it when they will remove it.

BTW feel free to close the bug if all is working fine for you.

Thanks.

Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

[michael-heerdegen]

BTW feel free to close the bug if all is working fine for you.

Thanks. I'll make some final tests tomorrow, and then hopefully close
it.

[michael-heerdegen]

Ok, looks good so far, thanks.