Open mehw opened 2 years ago
Thanks! I'll need some time to review all that.
Sure, take your time. Thanks, let me know.
@mehw: I like your approach, and as always, I'm impressed by the thoroughness of your tests.
Unfortunately, I'm running into trouble with some basics. In particular, commit 4b0ef93dd34e4a595f412a83d95a1583ad70eb4 introduces a problem: find-file
instantly errors out.
It's reproducible on my system just by doing this:
emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'
Substitute paths as needed. I'm... not sure what's going on here:
Debugger entered--Lisp error: (wrong-type-argument stringp nil)
expand-file-name(nil)
vc-file-getprop(nil vc-working-revision)
vc-working-revision(nil Git)
vc-git-mode-line-string(nil)
apply(vc-git-mode-line-string nil)
vc-call-backend(Git mode-line-string nil)
vc-mode-line(nil Git)
vc-refresh-state()
run-hooks(find-file-hook)
after-find-file(nil t)
find-file-noselect-1(#<buffer perspective.el> "~/Code/perspective-el/perspective.el" nil nil "~/Code/perspective-el/perspective.el" (8689707890 16777220))
find-file-noselect("~/Code/perspective-el/perspective.el" nil nil nil)
find-file("~/Code/perspective-el/perspective.el")
eval((find-file "~/Code/perspective-el/perspective.el") t)
Unfortunately, I'm running into trouble with some basics. In particular, commit 4b0ef93 introduces a problem:
find-file
instantly errors out.It's reproducible on my system just by doing this:
emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'
@gcv: It's quite odd... thanks for bringing that out. I'm looking into it... I didn't manage to sort out the problem yet.
@gcv: I'm sorry for not posting for so long... I finally have time to resume working on the PR... Thank you so much for waiting patiently in these strange times... ;)
All good! Looking forward to reviewing your work.
@mehw: Do you think you'll have time to look into this?
If not, I've been using persp-feature-flag-prevent-killing-last-buffer-in-perspective
and it seems to work quite well. I can flip it to on by default, and quietly get rid of it in a few weeks if no one complains.
@gcv: Yes, sorry, a lot has happened... I was optimistic that I could get back on track uninterrupted, but sometimes problems await us just around the corner...
I look into it. I have to rebase the PR and make the point about the error you reported:
emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'
I was able to address the problem, but still it's not clear to me the real source of it.
@gcv: Hi, I rebased the patch set on master, all tests passed.
About the error triggered by the below command, vc-git--run-command-string
is involved, and apparently it's due to the value of current-buffer
. I believe to be on a good lead, though. Thanks for the patience ;)
emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'
Okay, sounds good, let me know when you want me to take another look. Thanks for working on this.
Hi @gcv, I've done some progress, and things are more clear.
I need your help to decide about the appropriate behavior.
The value of the current-buffer
is what was causing the problem discussed in this PR when it is changed not following the vanilla (aka persp-mode
disabled) expectations in a particular scenario, involving vc-git--run-command-string
, where killing any buffer would always change the current-buffer
to the selected window's buffer. It's easy fixable controlling how and when the current-buffer
is changed while in persp-mode
(see the Bottom line below), but there could be other occasions where an unexpected change of current-buffer
may give troubles...
Right now, I'm refining the ert tests about how and when the current-buffer
should be changed after any (in)direct call to persp-forget-buffer
and kill-buffer
(either one, persp-maybe-kill-buffer
mediated and not) while in persp-mode
.
In vanilla (aka persp-mode
disabled) mode, kill-buffer
, when the curren-buffer
is killed, would change it to the value of other-buffer
; while in a perspective, this would mean that other-buffer
could be outside of the perspective, selecting it from the buffer-list
. Intead, bury-buffer
, called with no buffer argument, would change a current-buffer
, when it's also the window-buffer
, to the value of the most suitable window-prev-buffers
or buffer-list
as fallback; while in a perspective, the chosen buffer could also be outside of the perspective when the buffer-list
is considered.
current-buffer
changes follow the vanilla expectations?current-buffer
become one of the current perspective's buffers only if A. it is killed, or B. it is forgot when at the same time it is in a current perspective's window?What do you suggest?
Thank you a lot ;)
Does the current-buffer
problem show up because some code involving vc-git-*
uses kill-buffer
internally on temporary buffers — which triggers all persp-related checks and special handling?
Yes, it does.
kill-buffer
should always and immediately succeed on temporary buffers. That was the point of the workaround I put in: https://github.com/nex3/perspective-el/blob/4e38680793585a907ae46b148697030c2b552a00/perspective.el#L969-L973
I'm not sure how current-buffer and window visibility is affected in the case of vc-git-*
, though. Temporary buffers aren't supposed to go into prev and next buffer lists for a window (or if they do, that's pretty surprising behavior IMO — users aren't supposed to see them).
Hi @gcv, I see you removed persp-feature-flag-prevent-killing-last-buffer-in-perspective
with commit https://github.com/nex3/perspective-el/commit/e994fb3067d343732f9fc0ae209cecd5a6192237.
I plan to formalize the remaining tests this week end. I have also to fix a couple of subtle bugs ;)
I just renamed it and turned it on by default. Let's get more people on that code path. Looking forward to your code changes.
Thank you for waiting so patiently. Often I fear that the lack of new commits of mine is mistaken as lack of commitment.
No rush and no worries! We're all volunteers here, and this is about Emacs. It's all in good fun.
@gcv: This PR possibly fixes #168 (decreased processing speed during a
persp-maybe-kill-buffer
duty cycle). It may relate also to #164 and #167.DISCLAIMER:
basic-persp-killing-buffers-benchmark
may crash Emacs. If it does, try to repeat the test.In brief:
persp-maybe-kill-buffer
removes buffers manipulating the frame's hash table directly, and sets a perspective's dirty flag to update the windows configuration when the perspective ispersp-activate
re-activated.The first commit of this PR is just a starting point I needed, a remainder from where to begin, a plain changelog update.
Switching perspective repeatedly is time consuming. In reference to
perps-maybe-kill-buffer
, the speed improvements gained with this PR are due to removing buffers manipulating the frame's hash table directly, rather than switching to a perspective first.The major difference between switching-perspective-then-removing-buffer and remove-buffer-from-hash-table is that the former also updates a perspective's windows configuration.
Not updating a windows configuration means that once a perspective is switched to via
persp-activate
(aka re-activated), removed buffers still present in the windows configuration will be pulled back into the perspective... Unless a dirty flag is used...Once set, a perspective's dirty flag allows to update a windows configuration only when it's time to do it, i.e. during a
persp-activate
cycle, buffers not belonging to the perspective (pulled back in by the windows configuration) will be forgotten by the perspective automatically.persp-maybe-kill-buffer: different approaches, from last PR's commit to old behavior
Each section below has a descriptive header of the
persp-maybe-kill-buffer
behavior. Then, pseudo code and benchmark results will follow (a buffer is killed to collect the timings ofkill-buffer
, thepersp-maybe-kill-buffer
duty cycle is executed only when enabled by thepersp-feature-flag-prevent-killing-last-buffer-in-perspective
flag).Benchmark creating
20 perspectives * (51 regular buffers + 51 temporary buffers)
. All buffers are shared between perspectives, including"*dummy*"
and" *foo*"
. Before killig a buffer, the buffer ispersp-set-buffer
to the current perspective, to remove it from other perspectives (allowing it to be killed for sure, but still be part of a perspective). The sequence is:persp-maybe-kill-buffer
;Performance flag enabled/disabled:
persp-feature-flag-directly-kill-ido-ignore-buffers
Enables/Disables
persp-maybe-kill-buffer
:persp-feature-flag-prevent-killing-last-buffer-in-perspective
verify buffer accessing the frame's hash table, remove from hash table and set dirty flag
verify buffer accessing the frame's hash table, then switch perspectives and remove buffer
verify buffer switching perspectives, then switch perspectives and remove buffer