Helm vs. Transient

[tarsius]

/cc @thierryvolpiatto @daedsidog @karthink

This discusses the issue raised at karthink/gptel#656, also discussed in the second half of #358.

The sequence in which Helm and Transient display and delete windows is problematic when used together. This can be observed by making them both log to the same buffer:

(setq transient--debug t)
(setq helm-debug t)
(setq helm-debug-buffer "*Messages*")

1. Transient invokes a suffix command, which does not exit the transient menu. Transient cannot know whether that command will use the minibuffer.
2. The suffix command decides to use the minibuffer, completing-read hands the job to Helm.
3. Helm saves the current window configuration, so that it can later be restored.
4. Helm actually uses the minibuffer.
5. Transient notices that the minibuffer is being used.
6. Because transient-show-during-minibuffer-read is nil, Transient deletes the window displaying the transient menu.
7. The user exits the minibuffer.
8. Helm tries to restore the window configuration stored in (3). It cannot actually do so because the window previously used to show the transient window no longer exists. In place of that it creates a new window, displaying some recently used buffer.
9. Transient displays its menu window again. Given the default configuration, it tries hard not to reuse some existing window, so the window created in (8) remains.
10. The user exits the transient menu. The window created in (8) and preserved in (9) continues to exist.

Transient does not save and restore the existing window configuration like Helm does because it is an interface to invoke arbitrary commands and the purpose of such commands could be to permanently change the window configuration. Instead it uses its own window to display its menu and takes care to prevent other things from reusing that window and carefully deletes just its own window when done.

Helm could theoretically use the same approach, but this is more difficult and because it is customizable, users may shoot themselves in the food when customizing transient-display-buffer-function and related options (the original issue in #358 is an example of that).

But we can handle this incompatibility by making Transient delete its window before Helm saves the window configuration. Normally Transient relies on a function on post-command-hook to detect uses of the minibuffer. When a command uses the minibuffer, then that hook is called twice on behalf of that command, once when the minibuffer is entered and then again when the command is actually done. These two calls can be told apart. Helm on the other hand knows that it is going to use the minibuffer, and it can detect when Transient is in use and act accordingly.

This is how I think that should be handled in Helm:

(defcustom helm-save-configuration-functions
  '(set-window-configuration
    ;; Replacing `current-window-configuration':
    . helm-current-window-configuration)
  "..."
  :group 'helm
  :type 'sexp)

(defun helm-current-window-configuration ()
  "Like `current-window-configuration' but deal with Transient incompatibility."
  (when (and (fboundp 'transient--preserve-window-p)
             (fboundp 'transient--delete-window)
             (not (transient--preserve-window-p)))
    ;; Noop if `transient--window' isn't a live window.
    (transient--delete-window))
  (current-window-configuration))

I only created transient--preserve-window-p today, so you'll have to update Transient before that can work.

[thierryvolpiatto]

Hello Jonas, Jonas Bernoulli ***@***.***> writes:

But we can handle this incompatibility by making Transient delete its window before Helm saves the window configuration. Normally Transient relies on a function on post-command-hook to detect uses of the minibuffer. When a command uses the minibuffer, then that hook is called twice on behalf of that command, once when the minibuffer is entered and then again when the command is actually done. These two calls can be told apart. Helm on the other hand knows that it is going to use the minibuffer, and it can detect when Transient is in use and act accordingly. This is how I think that should be handled in Helm: (defcustom helm-save-configuration-functions '(set-window-configuration ;; Replacing `current-window-configuration': . helm-current-window-configuration) "..." :group 'helm :type 'sexp) (defun helm-current-window-configuration () "Like `current-window-configuration' but deal with Transient incompatibility." (when (and (fboundp 'transient--preserve-window-p) (fboundp 'transient--delete-window) (not (transient--preserve-window-p))) ;; Noop if `transient--window' isn't a live window. (transient--delete-window)) (current-window-configuration)) I only created transient--preserve-window-p today, so you'll have to update Transient before that can work.

I know nothing about transient and even how to use it, perhaps you can show me how to reproduce such bug (recipe)? Otherwise if the code above is fine for you I am ok to merge it in Helm (PR please). Thanks.

…

-- Thierry

[tarsius]

I know nothing about transient and even how to use it,

It's the library used to implement the menus in Magit.

The issue can be demonstrated using l - A (setting a value for -author in the menu used to show a Git logs).

(PR please)

emacs-helm/helm#2711

[thierryvolpiatto]

Jonas Bernoulli ***@***.***> writes:

I know nothing about transient and even how to use it, It's the library used to implement the menus in Magit. The issue can be demonstrated using l - A (setting a value for -author in the menu used to show a Git logs).

Hmm, it's a long time I didn't use magit...

…

-- Thierry