justbur / emacs-which-key

Emacs package that displays available keybindings in popup
GNU General Public License v3.0
1.74k stars 87 forks source link

Preserve window configuration #321

Closed fredericgiquel closed 2 years ago

fredericgiquel commented 3 years ago

The idea is to save current window configuration before which-key window is shown and to restore it after which-key window is hidden.

Now, when replaying the second illustration of the issue :

Illustration

duncanburke commented 3 years ago

To work around this problem I've been using advice:

(defvar my-which-key-windows-pos nil)
(defun my-which-key-before-popup (&rest r)
  (unless my-which-key-windows-pos
    (--map (push (cons it (window-start it)) my-which-key-windows-pos)
           (window-list (selected-frame)))))
(defun my-which-key-after (&rest r)
  (--map (when (window-live-p (car it))
           (set-window-start (car it) (cdr it) t))
         my-which-key-windows-pos)
  (setq my-which-key-windows-pos nil))
(advice-add 'which-key--show-popup :before #'my-which-key-before-popup)
(advice-add 'which-key--hide-popup :after #'my-which-key-after)

I'm concerned that using set-window-configuration in 2042f11eb1c036f3e4dcf3ce447e978b9ee64357 is too much of a blunt instrument. That function will not only restore the positions of the windows, but remove any new windows that have been created and restore any windows that have been destroyed. Note how in my advice I'm very conservative: I save the positions of any windows that were open in the current frame and restore only the positions of those windows that are still alive. I put in that check after I ran into some problems with trying to call set-window-start on a window that was no longer alive. That I ran into such a problem implies that in normal use, it is possible for a window to be destroyed between when which-key is invoked and when it exits. Any fix to this problem shouldn't introduce unexpected behavior such as restoring a window that the user expects to be gone.

fredericgiquel commented 3 years ago

@duncanburke You're right when you say that set-window-configuration does unnecessary (and maybe even dangerous) things. But I haven't had any drawback using this solution for several weeks. Maybe the problem with window no longer alive is caused by the position of the advice (outside the (when (buffer-live-p which-key--buffer)) condition. Is there a way to reproduce the problem (to better understand it) ?

duncanburke commented 3 years ago

Alright, I've managed to replicate it:

That consistently results in a window existing when my-which-key-before-popup is first called that is no longer alive when my-which-key-after is called. That window's buffer is titled *which-key*.

justbur commented 3 years ago

It does seem like the core functions for reading from the minibuffer are actively restoring the window configuration after the mini buffer closes, but I'm not yet comfortable with how this works. I'm fine including this functionality, but I'd like to hide it behind a user option until I understand it better.

In other words, if you want to add this as an option (default off), I'll accept it and revisit the design later.

fredericgiquel commented 3 years ago

@duncanburke Thanks for the example.

I tried it with your advices and (debug-on-entry 'set-window-start). It enters the debugger before toggling docstrings (just after "C-h"). It means that the restoration of window positions is launched when the which-key buffer is still visible. And that logically leads to weird situations.

And I tried it with my PR and (debug-on-entry 'set-window-configuration). It enters the debugger after "C-g" when the which-key side-window is gone. It works the same way as when you do not toggle the docstrings.

So I think that the question is not how to perform the save/restoration (with set-window-configuration or with set-window-start) but when.

fredericgiquel commented 3 years ago

@justbur I made the restoration optional (and disabled by default).

Fell free to change the option name or modify its documentation string.

justbur commented 3 years ago

Thank you @fredericgiquel. Unfortunately, this now puts you over 15 lines, so I will need to wait for your copyright assignment to merge the change. Please let me know when you complete that.

fredericgiquel commented 2 years ago

@justbur The copyright assignment process is complete. I received the fully signed PDF.

Do I have to do anything else?

justbur commented 2 years ago

No, thank you @fredericgiquel