Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Preserve window configuration #321

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Preserve window configuration #321

merged 2 commits into from
Nov 26, 2021

Conversation

fredericgiquel
Copy link
Contributor

@fredericgiquel fredericgiquel commented Aug 4, 2021

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
Copy link
Contributor

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 2042f11 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
Copy link
Contributor Author

@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
Copy link
Contributor

duncanburke commented Aug 17, 2021

Alright, I've managed to replicate it:

  • C-x, wait for which-key to show
  • toggle docstrings
  • C-g

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
Copy link
Owner

justbur commented Aug 17, 2021

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

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

@justbur
Copy link
Owner

justbur commented Aug 24, 2021

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
Copy link
Contributor Author

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

Do I have to do anything else?

@justbur justbur merged commit a8da871 into justbur:master Nov 26, 2021
@justbur
Copy link
Owner

justbur commented Nov 26, 2021

No, thank you @fredericgiquel

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants