nex3 / perspective-el

Perspectives for Emacs.
MIT License
880 stars 71 forks source link

persp-mode doesn't works well if enable-recursive-minibuffers is t #163

Closed TxGVNN closed 3 years ago

TxGVNN commented 3 years ago

Hi,

I discovered a issue when I use (setq enable-recursive-minibuffers t) and in recursive-minibuffer.

How to reproduce

emacs -Q -l ~/.emacs.d/elpa/perspective-20210821.259/perspective-autoloads.el  --eval '(setq enable-recursive-minibuffers t)' --eval '(persp-mode)' --eval '(persp-switch "test")' 

The M-x -> type M-x again -> persp-next -> persp-next. Oops. Something went wrong, I don't know how to describe. Feel something is keep in minibuffer and we can not escape. After that persp-next and persp-prev doesn't work anymore.

I tested on persp-mode package, but it doesn't affect.

gcv commented 3 years ago

The *Messages* buffer shows an error message:

delete-other-windows: Specified root is not an ancestor of specified window

delete-other-windows gets called from persp-reset-windows, which probably gets called from persp-activate.

I turned off recursive minibuffers years ago because I found them confusing. What should happen in the use case you're describing?

TxGVNN commented 3 years ago

delete-other-windows: Specified root is not an ancestor of specified window

The message, seems it is normal, but after that persp-mode can not work anymore. I can't switch workspace :(

I would like to use recursive minibuffers, it helps me in some case (maybe it is rare). Do you know how to fix this bug? It makes me so annoying. Or we can deny any task about persp-mode if the minibuffer in recursive mode

gcv commented 3 years ago

It's not a real fix, but try this patch to persp-switch:

diff --git a/perspective.el b/perspective.el
index ec62bec..a4edae2 100644
--- a/perspective.el
+++ b/perspective.el
@@ -665,6 +665,8 @@ perspective's local variables are set.
 If NORECORD is non-nil, do not update the
 `persp-last-switch-time' for the switched perspective."
   (interactive "i")
+  (when (> (minibuffer-depth) 0)
+    (user-error "Perspective operations in recursive minibuffers not supported"))
   (unless (persp-valid-name-p name)
     (setq name (persp-prompt (and (persp-last) (persp-name (persp-last))))))
   (if (and (persp-curr) (equal name (persp-current-name))) name
gcv commented 3 years ago

@TxGVNN: Did that patch fix your problem?

TxGVNN commented 3 years ago

Thanks, your patch fix my problem. I used unless to skip recursive minibuffer instead.

gcv commented 3 years ago

Okay, I pushed my patch to master. https://github.com/nex3/perspective-el/commit/acad4fb2cfe27feb0ecbe07e51c364bfa5ea4f47

gcv commented 2 years ago

@TxGVNN: Can you take a look at https://github.com/nex3/perspective-el/commit/6e87eeb6160345390406a82af0db61782bfb9913 ? It should be a nicer fix for your recursive minibuffer use case.

TxGVNN commented 2 years ago

@gcv yeah, I just tried it, looks perfect. I will experience it longer and feedback if something went wrong. Thank you very much

TxGVNN commented 2 years ago

After few days to use new version. Seems it still doesn't totally fix my issue. Because I integrate perspective with project-switch. I still see this bug exist, although it appears rare.

  ;; switch persp with project
  (advice-add #'project-current :filter-return #'project-current-with-persp)
  (defun project-current-with-persp (pr)
    "Override project-current(MAYBE-PROMPT DIRECTORY)."
    (if-let* ((bound-and-true-p persp-mode)
              (cdr pr))
        (persp-switch (cdr pr))) pr)

My workaround: https://github.com/TxGVNN/giaelpa/commit/7dff9588902557981ad2eda4790fe4b55aee199d

@@ -665,6 +665,7 @@ perspective's local variables are set.
 If NORECORD is non-nil, do not update the
 `persp-last-switch-time' for the switched perspective."
   (interactive "i")
+ (unless (> (minibuffer-depth) 0)
   (unless (persp-valid-name-p name)
     (setq name (persp-prompt (and (persp-last) (persp-name (persp-last))))))
   (if (and (persp-curr) (equal name (persp-current-name))) name
@@ -679,7 +680,7 @@ If NORECORD is non-nil, do not update the
       (unless norecord
         (setf (persp-last-switch-time persp) (current-time))
         (run-hooks 'persp-switch-hook))
-      name)))
+      name))))