Bad-ptr / persp-mode.el

named perspectives(set of buffers/window configs) for emacs
400 stars 44 forks source link

Perspective + magit + helm troubles #51

Open algernon opened 7 years ago

algernon commented 7 years ago

I'm using Spacemacs, and make heavy use of perspective, but if I have multiple layouts, and do commit with magit in one of them, trying to switch buffers with helm-mini will fail with an error. See syl20bnr/spacemacs#7270 for the details.

For the time being, I have the following code, which I run manually whenever the error happens:

(with-eval-after-load "persp-mode"

(defun persp-remove-killed-buffers ()
    (interactive)
    (mapc #'(lambda (p)
              (when p
                (setf (persp-buffers p)
                      (delete-if-not #'buffer-live-p
                                     (persp-buffers p)))))
          (persp-persps)))
)

This appears to solve the issue, and I have not noticed any side effects.

Bad-ptr commented 7 years ago

Can not reproduce. So I need a detailed instruction of how to reproduce this. Are you using emacs daemon/emacsclients(and how many)/frames, spacemacs master or develop, etc? What I did:

Downloaded emacs-24.5 sources from http://mirror.tochlab.net/pub/gnu/emacs/emacs-24.5.tar.xz and builded emacs-24.5 Cloned spacemacs Started emacs, package-installed magit Created two layouts Do some modification on files in .emacs.d(which was cloned from spacemacs) magit-status, stage, commit C-x b without an error

Also I don't know anything about how to configure and use spacemacs and may be I missed something.

fardog commented 7 years ago

@Bad-ptr not the original reporter, but here's what i'm doing:

oh, edit: I'm using emacs 25.1 and the latest spacemacs 0.200.1

Bad-ptr commented 7 years ago

Still can not reproduce :( https://drive.google.com/open?id=0B4579Xy2gwv8VEpOMjlNdkZJMTg Interesting. Are you getting this bug on macOS? @fardog @algernon

algernon commented 7 years ago

I'm on Debian, Emacs 24. Will be able to dive deeper and hopefully provide better repro steps over the weekend. Unfortunately, I'm swamped until then, and can't play with my editor to cut down the test case into easy steps.

jcazevedo commented 7 years ago

I'm capable of reproducing this with a single emacs client, but the issue only manifests itself after I commit via magit (which creates the COMMIT_EDITMSG buffer). Here are the steps I'm doing to reproduce it:

deb0ch commented 7 years ago

I have an issue with the same repro steps, but different error.

It happens every time I commit using magit in a perspective. It will prevent me from listing buffers using helm-mini or (with-persp-buffer-list () (helm-mini)) as well as saving the layout (or all layouts).

When trying to list buffers, *Messages* shows:

apply: Wrong type argument: stringp, nil

When trying to save layout (or layouts), *Messages* shows:

string-prefix-p: Wrong type argument: stringp, nil

EDIT: forgot to mention, I'm on emacs 24.5.1 on Ubuntu 16.04 and Spacemacs 0.200.2

Bad-ptr commented 7 years ago

OK, don't know what it can be, but here is something you can try: 1) Redefine persp--set-frame-buffer-predicate-buffer-list-cache and persp--get-frame-buffer-predicate-buffer-list-cache like this:

(defsubst persp--set-frame-buffer-predicate-buffer-list-cache (buflist)
  (setq persp-frame-buffer-predicate-buffer-list-cache buflist))
(defmacro persp--get-frame-buffer-predicate-buffer-list-cache (buflist)
  (persp--set-frame-buffer-predicate-buffer-list-cache buflist))

2) Redefine the persp-frame-save-state:

(defun* persp-frame-save-state (&optional (frame (selected-frame)) set-persp-special-last-buffer)
  (when (and frame
             (not (persp-is-frame-daemons-frame frame))
             (not (frame-parameter frame 'persp-ignore-wconf))
             (not (frame-parameter frame 'persp-ignore-wconf-once)))
    (let ((persp (get-frame-persp frame)))
      (with-selected-frame frame
        (when set-persp-special-last-buffer
          (persp-special-last-buffer-make-current))
        (if persp
            (progn
              (persp-filter-out-bad-buffers persp)
              (setf (persp-window-conf persp) (funcall persp-window-state-get-function frame)))
          (setq persp-nil-wconf (funcall persp-window-state-get-function frame)))))))

3) Redefine the persp-buffer-filtered-out-p

(defun persp-buffer-filtered-out-p (buff-or-name &rest filters)
  (setq filters (if filters
                    (cons
                     persp-common-buffer-filter-functions
                     filters)
                  persp-common-buffer-filter-functions))
  (block pbfop
    (let ((buf (get-buffer buff-or-name))
          filter f)
      (when (buffer-live-p buf)
        (while (setq filter (pop filters))
          (when
              (if (functionp filter)
                  (funcall filter buf)
                (while (setq f (pop filter))
                  (when (funcall f buf)
                    (return-from pbfop t))))
            (return-from pbfop t))))
      nil)))
mnetzwur commented 7 years ago

I don't use magit, but I am having the same issue with markdown-mode's live preview. If I create a live preview from a markdown-mode buffer, I get the same error as reported above.

Bad-ptr's solution did not work. It produces the following error:

persp--set-frame-buffer-predicate-buffer-list-cache: Symbol’s function definition is void: \,

Algernon's function fixes it for me as well but I figured I'd post my slight modification of the work-around which does not require any manual interaction. Basically, I advise the helm-mini function to always remove killed buffers before doing anything else:

(with-eval-after-load "persp-mode"
  (defun persp-remove-killed-buffers (orig-fun &rest args)
    (mapc #'(lambda (p)
              (when p
                (setf (persp-buffers p)
                      (delete-if-not #'buffer-live-p
                                     (persp-buffers p)))))
          (persp-persps))
    (apply orig-fun args))
  (advice-add 'helm-mini :around #'persp-remove-killed-buffers)
  )

This might be overkill, but it works and I haven't noticed any downsides.

Bad-ptr commented 7 years ago

persp--set-frame-buffer-predicate-buffer-list-cache: Symbol’s function definition is void: \,

Fixed.)

Bad-ptr commented 7 years ago

the same issue with markdown-mode's live preview

I can reproduce it!)

Bad-ptr commented 7 years ago

I think I just fixed it. It will be great if someone will try to reproduce it with a version from develop branch 83d28676cc28ee175f3625041a68eb4c47be7605 https://github.com/Bad-ptr/persp-mode.el/tree/develop

Explanation: persp-mode set buffer-local variable persp-buffer-in-persps for each buffer. The value of this variable is a list of names of perspectives the buffer belongs to. Markdown mode (and probably magit too) is changing the major mode of some buffers and as a result the persp-buffer-in-persps value for these buffers is set to nil(it seems like changing major-mode of a buffer will reset all buffer-local variables).

Bad-ptr commented 7 years ago

Must be fixed now in master branch.

jcazevedo commented 7 years ago

I just tried with version 20161025.507 from MELPA and it seems to be fixed. Thanks!

Bad-ptr commented 7 years ago

Hmm... looks like everyone satisfied with the fix. )

syl20bnr commented 5 years ago

I've got this same error and symptoms using Emacs 27.0.50 with all packages up to date and last develop version of Spacemacs. If I open a magit buffer or a markdown buffer I've got the stringp, nil error.

Bad-ptr commented 5 years ago

This is strange. But you may try to reproduce it from emacs -Q, load&activate persp-mode, load&activate markdown-mode with live preview.

Or at least toggle-debug-on-error and provide a backtrace :)

syl20bnr commented 5 years ago

I'm currently debugging it, more info to come.

syl20bnr commented 5 years ago

Seems that I have a #<killed buffer> in the buffer list which ends up being passed as nil down the line to Helm:

(persp-buffer-list-restricted) option: 0
(persp-buffer-list-restricted) bl: (persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* #<killed buffer> README.md<.emacs.d>)
buffer: (persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* nil README.md<.emacs.d>)
syl20bnr commented 5 years ago

I can see there is some code to handle the killed buffers case, investigating if this code still works. Maybe the nil comes from somewhere else.

syl20bnr commented 5 years ago

Interesting, this docstring says:

(defun persp-kill-buffer-query-function ()
  "This must be the last hook in the `kill-buffer-query-functions'.
Otherwise if next function in the list returns nil -- the buffer will not be
killed, but just removed from a perspective(s)."

The value for kill-buffer-query-functions is:

kill-buffer-query-functions is a variable defined in ‘C source code’.
Its value is
(persp-kill-buffer-query-function process-kill-buffer-query-function)

Which could explain the issue ?

Bad-ptr commented 5 years ago

Which could explain the issue ?

The issue is that in the persp-kill-buffer-query-function the buffer can be removed from a perspective(s) (with the idea that it will be killed) but next function in the list returning nil will keep buffer alive. It's ok(it must not lead to error), but this buffer can "hide from sight of a user".

I think it is not related to your bug.

syl20bnr commented 5 years ago

It seems that the #<killed buffer> is the culprit. In the logs above the buffer: value corresponds to buffer-name-sorted variable in the function persp-restrict-ido-buffers.

The following filter function seems to fix it for me:

(defun toto (buffer-or-name)
  (not (persp-get-buffer-or-null buffer-or-name)))

(add-to-list #'persp-buffer-list-restricted-filter-functions #'toto)
syl20bnr commented 5 years ago

To be more clear.

With the following filtering function (that does nothing):

(defun toto (buffer-or-name) nil)

We get the bug with the following output:

(persp-buffer-list-restricted) option: 0
(persp-buffer-list-restricted) bl: (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* #<killed buffer> README.md<.emacs.d>)
(persp-buffer-list-restricted) returned bl: (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* #<killed buffer> README.md<.emacs.d>)
(persp-restrict-ido-buffers) buffer-names-sorted (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* nil README.md<.emacs.d>)

With this filtering function:

(defun toto (buffer-or-name)
  (not (persp-get-buffer-or-null buffer-or-name)))

No error and the output is:

(persp-buffer-list-restricted) option: 0
(persp-buffer-list-restricted) bl: (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* #<killed buffer> README.md<.emacs.d>)
(persp-buffer-list-restricted) returned bl: (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* README.md<.emacs.d>)
(persp-restrict-ido-buffers) buffer-names-sorted (magit: .emacs.d keybindings.el core-debug.el .spacemacs persp-mode.el *Help* helm-for-files.el helm.el funcs.el packages.el *scratch* *Backtrace* README.md<.emacs.d>)
Bad-ptr commented 5 years ago

well If I understand you did the following: 1) install Emacs 27.0.50 2) install spacemacs develop 3) update packages 4) restart emacs 5) open markdown file. With live-preview-mode ? 6) try to switch buffer with helm and got the error

Am I correct or did I miss something?

syl20bnr commented 5 years ago

Yeah but the tricky part is that I was able to open some markdown files without the bug. Maybe the issue here led me to believe that markdown files would trigger the bug although it could be something else.

I would replace the step 5 by:

Open multiple perspectives using SPC p l selecting a markdown file as first file to open. Then try if SPC b b works.

Bad-ptr commented 5 years ago

Seems like I encountered this bug in emacs 27. Very strange. Looks like sometimes buffers are killed without firing kill-buffer-hook/kill-buffer-query-functions.

Grekkor commented 4 years ago

Is there any fix available? I encounter this bug constantly...

Bad-ptr commented 4 years ago

actually it looks like I can not reproduce this issue with latest version of emacs 28

I encounter this bug constantly...

Is it happens with specific package/mode in use?

Bad-ptr commented 4 years ago

And don't forget you can try this code(i recently updated it to work with latest persp-mode) to see which buffer is bad.

Grekkor commented 4 years ago

It is very sporadic. I cannot tell what causes it but if it happens I get: wrong-type-argument stringp nil Thanks for the code snippet, I'll try it out and keep an eye on it.

Bad-ptr commented 4 years ago

// I need to document what this code do

The idea of the snippet is to check for killed buffers that were not removed from "perspectives" and to report bad buffer names to *Messages*. This can point to package/mode/user actions that trigger the bug. Also, after you get the error, you can manually do M-x persp-remove-killed-buffers RET to be able to switch buffers.