Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why disable after-make-frame-functions? #29

Closed
gcv opened this issue May 25, 2019 · 11 comments
Closed

Why disable after-make-frame-functions? #29

gcv opened this issue May 25, 2019 · 11 comments

Comments

@gcv
Copy link

gcv commented May 25, 2019

posframe--create-posframe explicitly binds after-make-frame-functions to nil. This currently breaks persp-mode (perspective-el), which uses frame parameters extensively. These parameters are initialized in a hook it adds to after-make-frame-functions, but are always missing from posframe frames.

It would be a fairly large change to persp-mode to become more resilient to missing parameters, and I haven't run into problems with anything other than posframe. Is there a reason why posframe needs to avoid running frame creation hooks?

@conao3
Copy link
Contributor

conao3 commented May 25, 2019

I use persp-mode, but I never felt it was broken. If there is any problem, please tell me the phenomenon and process. There is a possibility that I am not aware of the problem in the first place.

Also, posframe needs to set after-make-frame-functions to nil in order to create a pure frame as posframe's frame from a package like elscreen that wants the header to appear for all frames.

@gcv
Copy link
Author

gcv commented May 25, 2019

I was experimenting with ivy-posframe, and enabled it for Swiper like so:

        (setq ivy-display-functions-alist
              '((swiper . ivy-posframe-display-at-window-bottom-left)
                (t . ivy-posframe-display-at-window-center)))

        (ivy-posframe-enable)

Provided this code is in the Emacs startup script (and all dependencies load), then (1) make sure persp-mode is off, (2) make sure no posframes are open (calling posframe-delete-all should be sufficient), (3) turn on persp-mode, and (4) activate Swiper. This instantly fails with Wrong type argument: perspective, nil.

I can also reproduce this with just the code from the posframe README:

(posframe-show " *my-posframe-buffer*"
                 :string "This is a test"
                 :position (point))

This was tricky to track down. I knew the problem only showed up after I installed posframe and ivy-posframe, and it obviously had to do with persp-mode, but I had no idea where the crash took place. toggle-debug-on-error did nothing. Eventually, I figured out that this problem occurs in persp-mode's set-window-buffer after-advice (see https://github.com/nex3/perspective-el/blob/master/perspective.el#L782-L784). set-window-buffer gets called at the end of posframe--create-posframe and the advice code assumes the frame contains persp's data structures. That fails and crashes.

Unfortunately, this is only one of many places where persp-mode assumes that persp-init-frame has been called on all frames.

@conao3
Copy link
Contributor

conao3 commented May 25, 2019

I am interested in this issue because I am the maintainer of ivy-posframe.

I tried the first reproduction process. Since Swiper was started immediately after enabling persp-mode, the following error occurred when persp-auto-resume was started with a minibuffer open.
This problem can be solved by setting persp-auto-resume-time to a shorter interval, such as 0.1.

Debugger entered--Lisp error: (error "Can’t expand minibuffer to full frame")			     
  signal(error ("Can’t expand minibuffer to full frame"))					     
  error("Can't expand minibuffer to full frame")						     
  delete-other-windows(#<window 4 on  *Minibuf-1*>)						     
  persp-delete-other-windows()									     
  persp-restore-window-conf(#<frame  *Minibuf-1* 0x102085230>)					     
  mapc(persp-restore-window-conf (#<frame  0x1022cfdc0> #<frame  *Minibuf-1* 0x102085230>))	     
  persp-update-frames-window-confs(("default"))							     
  (progn (persp-update-frames-window-confs persp-names))					     
  (if (eq phash *persp-hash*) (progn (persp-update-frames-window-confs persp-names)))		     
  (when (eq phash *persp-hash*) (persp-update-frames-window-confs persp-names))			     
  (lambda (file phash persp-names) (when (eq phash *persp-hash*) (persp-update-frames-window-confs pe
  run-hook-with-args((lambda (file phash persp-names) (when (eq phash *persp-hash*) (persp-update-fra
  persps-from-savelist(((def-persp nil ((def-buffer "persp-mode.el" "/Users/conao/.emacs.d.debug/ivy-
  persp-load-state-from-file()									     
  #f(compiled-function () #<bytecode 0x40e55a2d>)()						     
  apply(#f(compiled-function () #<bytecode 0x40e55a2d>) nil)					     
  timer-event-handler([t 23784 48909 204131 nil #f(compiled-function () #<bytecode 0x40e55a2d>) nil n
  read-from-minibuffer("Swiper: " nil (keymap (keymap (3 keymap (6 . swiper-toggle-face-matching)) (6
  ivy-read("Swiper: " (#(" ;; This buffer is for text that is not saved, and for Lisp evaluation." 0 
  swiper--ivy((#(" ;; This buffer is for text that is not saved, and for Lisp evaluation." 0 1 (displ
  swiper()											     
  funcall-interactively(swiper)									     
  call-interactively(swiper nil nil)								     
  command-execute(swiper)                                                                            

However, the error you reported did not occur. Please give me more details or try my below init.el.
My Emacs is emacs-26.2 on macOS.

There is full init.el for debug.

(prog1 "Change user-emacs-directory"
  ;; enable debug
  (setq debug-on-error  t
        init-file-debug t)

  ;; you can run like 'emacs -q -l ~/hoge/init.el'
  (when load-file-name
    (setq user-emacs-directory
          (expand-file-name (file-name-directory load-file-name)))))

(prog1 "Load use-package.el"
  (setq package-archives '(("melpa" . "https://melpa.org/packages/")
                           ("gnu"   . "https://elpa.gnu.org/packages/")
                           ("org"   . "https://orgmode.org/elpa/")))
  (package-initialize)

  (unless (package-installed-p 'use-package)
    (package-refresh-contents)
    (package-install 'use-package))
  (require 'use-package))

(use-package use-package
  :custom ((use-package-expand-minimally t)
           (use-package-hook-name-suffix "")))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(use-package ivy
  :ensure t
  :custom ((ivy-mode t)
           (counsel-mode t))
  :bind (("C-s" . swiper))
  :config
  (prog1 "ivy-requirements"
    (use-package swiper :ensure t)
    (use-package counsel :ensure t))
  (use-package ivy-posframe
    :ensure t
    :custom ((ivy-display-functions-alist
              '((swiper . ivy-posframe-display-at-window-bottom-left)
                (t      . ivy-posframe-display-at-window-center))))
    :config
    (let ((inhibit-message t))
      (ivy-posframe-enable))))

(use-package persp-mode
    :ensure t
    :custom ((persp-keymap-prefix    "�p")
             (persp-nil-name         "default")
	     (persp-auto-resume-time 0.1)
             ;; (persp-mode t)                  ; enable after debug process
             )
    :hook ((emacs-startup-hook . toggle-frame-maximized))
    :config
    (prog1 "https://github.com/Bad-ptr/persp-mode.el#ivy"
      (with-eval-after-load "ivy"
        (add-hook 'ivy-ignore-buffers
                  #'(lambda (b)
                      (when persp-mode
                        (let ((persp (get-current-persp)))
                          (if persp
                              (not (persp-contain-buffer-p b persp))
                            nil)))))

        (setq ivy-sort-functions-alist
              (append ivy-sort-functions-alist
                      '((persp-kill-buffer   . nil)
                        (persp-remove-buffer . nil)
                        (persp-add-buffer    . nil)
                        (persp-switch        . nil)
                        (persp-window-switch . nil)
                        (persp-frame-switch  . nil)))))))

NOTE: persp-keymap-prefix is return value of (kbd "C-c p")

@gcv
Copy link
Author

gcv commented May 25, 2019

Thank you for looking into it.

Looking at your init.el, I can already see why you could not reproduce the problem I found. You're using a fork of the original perspective mode, which undoubtedly behaves differently. Looking at it casually, I see that it does not use frame parameters to store state, and does not use advice to modify any Emacs behaviors.

The fork you use is https://github.com/Bad-ptr/persp-mode.el. The original which I use is https://github.com/nex3/perspective-el. The original is confusingly and unfortunately named: it is called both perspective and perspective-el, but provides persp-mode to Emacs (MELPA link: https://melpa.org/#/perspective). The fork you use is named better (MELPA link: https://melpa.org/#/persp-mode).

I prefer the original because I find its frame-specific perspective list behavior preferable to the unified perspectives of the fork. In addition, I prefer to avoid the workgroups.el dependency the fork uses to implement saving perspectives to disk. I wrote a from-scratch implementation of perspective save and restore for the original package, which is currently in PR (nex3/perspective-el#80).

@gcv
Copy link
Author

gcv commented May 25, 2019

Actually, I was wrong about one thing: the fork does use frame parameters, though rather differently from the original. It also does use after-make-frame-functions, though perhaps in a way more compatible with ivy-posframe.

@conao3
Copy link
Contributor

conao3 commented May 25, 2019

Thanks! I installed 'original' perspective.el, then occur error you reported.
But I don't know why the posframe line is actually there until I ask @tumashu, but he probably can't delete it.
I think you may send a PR to perspective.el.

@conao3
Copy link
Contributor

conao3 commented May 25, 2019

I sent a PR to perspective upstream. It should not unconditionally expect persp-curr or persp-last to return a meaningful value. As proof of that, we confirm it in some places.
https://github.com/nex3/perspective-el/blob/master/perspective.el#L293
https://github.com/nex3/perspective-el/blob/master/perspective.el#L658

Proper conditional branching should be done before use, which is a common programming practice.

However, looking at the perspective maintenance situation, I don't think this PR will merge right away. In the meantime, you can cherry-pick the related commit from my fork.

@tumashu
Copy link
Owner

tumashu commented May 25, 2019

i do not remeber the reason to add this line, maybe i think it make posfame more stable.

@gcv
Copy link
Author

gcv commented May 25, 2019

@tumashu: Makes sense. Thank you for looking.

@conao3: Thank you! I agree with your analysis. I worry, however, that the approach in your patch leaves open the possibility of someone forgetting the check the return value in new code. What about changing the functions which may return nil to return valid but empty values instead? Please see https://github.com/gcv/perspective-el/commit/dd67e90821ba94549395c290d4fb2cc8e2cbcddd — what do you think?

@conao3
Copy link
Contributor

conao3 commented May 25, 2019

Regarding this issue, may we decide our goal as PR to perspective? If so, please close this issue and send your comments to the PR.
And I think that opinion should be incorporated into my PR. Thanks!

@gcv
Copy link
Author

gcv commented May 25, 2019

Yes, of course.

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

No branches or pull requests

3 participants