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

Add support for saving and restoring perspectives to disk #80

Merged
merged 14 commits into from
Oct 27, 2019
Merged

Add support for saving and restoring perspectives to disk #80

merged 14 commits into from
Oct 27, 2019

Conversation

gcv
Copy link
Collaborator

@gcv gcv commented Feb 25, 2019

This is an attempt to add support for durable perspectives. I often have many open, with many buffers and windows in each. Restarting Emacs with so much state (for upgrades or just instability reasons) is really painful.

This has no dependencies aside from perspective-el itself. It provides no integration with desktop-save-mode, though I imagine few persp-mode users rely on it since it doesn't really do the right thing.

It works with my Emacs setup, but definitely needs testing with other people's configurations.

@nickgarber
Copy link

Just a note to say I'm interested in this feature and am testing it out as of a few moments ago.
Happy to report on my experiences. :)

Really appreciate all the work that's gone into perspective, this feature included!

@nickgarber
Copy link

nickgarber commented Apr 3, 2019

I was getting errors from persp-state-load at read-whole-buffer-string.

It seemed like these records were accumulating and causing format issues on subsequent loads so I made two changes:

  1. replace read-from-whole-string with thing-at-point--read-from-whole-string per the recommendation in function Help
  2. delete the file prior to every save
          (use-package perspective
            :after (projectile)
            :init
            (persp-mode)
            :config
            (progn
              ;;(straight-use-package '(perspective :type git :host github :repo "gcv/perspective-el"))
              (defun persp-persist-state-to-file () ""
                     (progn
                       (delete-file "~/.vault/cfgs/emacs/apps/perspective/persist.el")
                       (persp-state-save "~/.vault/cfgs/emacs/apps/perspective/persist.el")))
              (add-hook 'kill-emacs-hook 'persp-persist-state-to-file)
              (defun persp-resume-state-from-file ()  "" (persp-state-load "~/.vault/cfgs/emacs/apps/perspective/persist.el"))
              (add-hook 'after-init 'persp-resume-state-from-file)))

With those adjustments this seems to be working for me!

@nickgarber
Copy link

nickgarber commented Apr 3, 2019

@gcv, I'm curious to know is desktop-save interoperability/support on your list for future changes?

@gcv
Copy link
Collaborator Author

gcv commented Apr 3, 2019

@nickgarber: Thank you for testing!

I think I'll add support for a custom persp-state-default-file variable which, if set, will provide a default for persp-state-save and persp-state-load. Still need to think about what to do about deleting the target file if it exists before saving — leaning towards prompting the user first unless a prefix argument is used or persp-state-default-file is set. That should make it possible to just add persp-state-save to kill-emacs-hook in your use case, but also avoid clobbering existing state files for people who might want to keep different persp state configurations.

I'm reluctant to use thing-at-point--read-from-whole-string, since (1) read-from-whole-string is an alias for it, and (2) it was only added in 25.1, and I'm not sure what versions of Emacs perspective-el officially supports.

About desktop-save-mode interoperability: I don't use it myself since it borks perspectives. I know it's supposed to help with font faces and variables, but I change those somewhat frequently so I don't find saving that helpful (different size screens, also I change through themes depending on ambient light). How do you use it, and how would you want it to integrate with persp-state?

@nickgarber
Copy link

nickgarber commented Apr 4, 2019

Your points in the first and second paragraph sound wholly sensible to me.
I've adjusted my test case to use the below:

         (use-package perspective
             :after (projectile)
             :init 
             (persp-mode)
             :custom
             (persp-state-default-file "~/.vault/cfgs/emacs/apps/perspective/persist.el" "")
             :config
             (progn
               ;;(straight-use-package '(perspective :type git :host github :repo "gcv/perspective-el"))
               (defun persp-persist-state-to-file () ""
                      (progn
                        (delete-file persp-state-default-file)
                        (persp-state-save persp-state-default-file)))
               (add-hook 'kill-emacs-hook 'persp-persist-state-to-file)
               (defun persp-resume-state-from-file ()  "" (persp-state-load persp-state-default-file))
               (add-hook 'after-init 'persp-resume-state-from-file)))

and (just a mention), I've also started testing this config alongside psession as:

           ;;; https:/thierryvolpiatto/psession
           (use-package psession                                                                            
             :init
             (require 'psession)
             (psession-mode 1)
             (psession-savehist-mode 1)
             (psession-autosave-mode 1))

and this all seems to be going well.

Perspective (+projectile) allows me to:

  • maintain discrete and coherent projects
  • lessen the cognitive load of switching between multiple projects over time

In particular, it's required to efficiently deliver on tasks that are prone to preemption.

When the desktop-save / session persistence functionality is added it just serves to extend the "over time" farther, across Emacs restarts.

Put simply, it means the mental safety-net can catch more preempted project work.

In the case of desktop-save-mode I believe I've also observed speed-ups when resuming vs when loading all of Emacs' init code, but thats anecdotal and secondary to the value described above.

Couple questions:

  • Q: Would it be possible to extend the work your doing to enable "hibernating" (and resuming) individual perspectives to disk?

  • Q: Any suggestions or tips to make the interoperability between psession and perspectives more useful or profound?

Thanks!

@gcv
Copy link
Collaborator Author

gcv commented Apr 5, 2019

I just pushed some changes to support persp-state-default-file, and added documentation to the README. For your use case, just make sure that variable is set, and you can then point kill-emacs-hook to persp-state-save directly (i.e., you don't need your custom wrapper function, and can just use (add-hook 'kill-emacs-hook #'persp-state-save)).

Please test. 😁

It's definitely possible to extend persp-state to support saving and restoring individual perspectives. It would be a nifty feature, though not one I've personally needed so far. persp--state-file-data and persp--state-frame-data would need to learn to filter by perspective, and interactive entry point saving and loading functions would have to be added.

For psession integration: as far as I can tell, the only things it restores that persp-state does not are (1) buffer-local variables, and (2) registers. History is well-handled with savehist-mode in my setup. On the downside, psession will likely mess up at least the main perspective, because it'll open all files first, and then persp-mode will start and stuff them in main. persp-state is pretty careful to avoid doing that. Variables and registers don't matter in my workflow, so I don't really need psession. If I did need it, I think I'd hack it to make sure (1) it didn't open any files which are already open, and (2) make sure it runs after persp-state-load, since by default it runs in emacs-startup-hook.

@tbtommyb
Copy link

Hi from Reddit! I'm trying this out. I have set the persp-state-default-file to ~/.emacs.d/perspectives/store.el. When I run persp-state-save I get:

let*: Args out of range: #s(perspective "dotfiles" (#<buffer store.el> #<buffer *Messages*> #<buffer *Buffer List*> #<killed buffer> #<buffer init.el> #<buffer *Bookmark List*> #<buffer *Warnings*> #<buffer *GNU Emacs*> #<buffer perspective.el> #<killed buffer> ...) nil nil nil nil #<window-configuration>), 8

Can you advise? Thanks!

@gcv
Copy link
Collaborator Author

gcv commented Jul 17, 2019

Thank you for testing. I'll need your help to track this down.

Please evaluate the following code (using ielm for example):

(persp--state-frame-data)

Does that crash with the same args out of range? If it does then run this:

(cl-loop for buffer in (persp-buffers (persp-curr))
                                                  if (persp--state-interesting-buffer-p buffer)
                                                  collect (buffer-name buffer))

If all that succeeds, run this:

(make-persp--state-complete
                                   :files (persp--state-file-data)
                                   :frames (persp--state-frame-data))

PS: What version of Emacs?

@gcv gcv mentioned this pull request Jul 22, 2019
@tbtommyb
Copy link

tbtommyb commented Aug 4, 2019

Hello, apologies for the delay. I changed my config over to use use-package and wanted to get everything working before turning to this.

With my new config I can't recreate the error. I can try and use my old config if you want to track it down?

When I use persp-state-save and persp-state-load interactively things work well - thanks! There are a couple of points I noticed.

  • my default Projectile action is to open dired in the project root. When I restore perspectives the dired buffers change to scratch. This is a bit annoying as scratch buffers seem to belong to the wrong project (i.e. C-x C-f from the scratch buffer takes me to a different project directory). This is a separate issue but it would be good if the restored perspectives included dired etc.
  • When I run the commands interactively the default file I have specified appears at the top of the minibuffer, which looks a little odd. Is it meant to do that?

Screenshot 2019-08-04 at 20 54 30

  • is there a way to control which perspective is active after loading? If I am in a perspective and load from file I would expect it to stay in the same perspective but with the restored arrangement. Or load the last active one on startup.

  • finally I'm a bit unsure about how to run persp-state-load on startup because perspective etc only loads when I first run C-c P P due to use-package. Obviously that's nothing to do with the PR but what would you recommend?

Overall this works really well and is basically exactly what I was looking for so thanks for your work.

@gcv
Copy link
Collaborator Author

gcv commented Aug 5, 2019

Lots of good feedback here.

  1. If you don't experience the problem any more, let's just let it go for now. I'm sure it'll come up again in some easier-to-reproduce situation.

  2. Regarding dired: I wrote the code to explicitly ignore all non-file-visiting buffers because they often have non-reproducible state (e.g., buffers responsible for shells or other running processes). Obviously dired is a major exception here, and I had a feeling it would come up eventually :). Other modes come to mind, like treemacs, though that one will be harder to deal with. I should add special-case support at least for dired buffers.

  3. I just call read-file-name with a default here. Judging by your screenshot, you use Ivy/Counsel — I can look into how they interact. Could you paste in your configuration for both Ivy and Counsel?

  4. The random perspective order happens because I just write a (readable) hash table to disk. I can see how that can be annoying, and will look into how much work it would be to retain their order. That would be necessary for compatibility with Generalizes sorting of perspectives and adds sort-by-creation-time. #87 anyway.

  5. Not sure, but I think adding :demand t to your use-package declaration will force use-package to eagerly load perspective. Or maybe you could just put a call to (persp-state-load "...") in your startup file and maybe that'll trigger the lazy load? Let me know if none of these work and I can try a little harder to think of a workaround. I use use-package myself but its lazy loading doesn't work quite right in my setup and I don't restart Emacs often enough to be annoyed and track it down.

I'm a bit swamped, but I'll try to find time to work on these improvements soon. Thanks for testing!

@tbtommyb
Copy link

tbtommyb commented Aug 6, 2019

No worries! I will try those things for lazy loading. Here's my ivy/counsel config

(use-package counsel
  :ensure t
  )


(use-package ivy
  :ensure t
  :after counsel
  :config
  (ivy-mode t)
  (setq ivy-use-virtual-buffers t)
  ;; fuzzy matching
  (setq ivy-re-builders-alist '((t . ivy--regex-fuzzy)))
  (setq ivy-initial-inputs-alist nil)
  :bind (("C-s" . swiper)
         ("M-x" . counsel-M-x)
         ("C-c k" . counsel-ag)
         ("C-x C-f" . counsel-find-file)
         )
  )

@gcv
Copy link
Collaborator Author

gcv commented Aug 9, 2019

@tbtommyb: I just updated this PR with a commit enabling saving and restoring dired-mode buffers and windows. Seems to work for me. Could you please test it?

I looked into the Ivy strange display thing. It seems to be an artifact of the way ivy-mode modifies read-file-name. I guess I can detect ivy and handle it as a special case, but that workaround seems like a future maintenance hassle waiting to happen.

@tbtommyb
Copy link

Yep, works nicely!

One other thing I've noticed is that I tend to leave lots of buffers open and also use flycheck. This means that restoring state can take quite a long time (15-20 seconds on a MBP). Maybe this is fine and inevitable because of how I do things. Personally I wouldn't expect perspective to reload anything more than the visible buffers but I guess it depends on how you use emacs. Anyway the functionality seems to be all there, thanks!

@azzamsa
Copy link

azzamsa commented Sep 7, 2019

@gcv sorry but why didn't we use desktop-mode functionality? ( I tried it but didn't work)

leave lots of buffers open and also use flycheck. This means that restoring state can take quite a long time (15-20 seconds on a MBP). @tbtommyb

@gcv desktop-mode had (setq desktop-restore-eager 5), so it will restore things lazily

desktop-restore-eager’s value is t

Documentation:
Number of buffers to restore immediately.
Remaining buffers are restored lazily (when Emacs is idle).
If value is t, all buffers are restored immediately.

@gcv
Copy link
Collaborator Author

gcv commented Sep 7, 2019

desktop-mode predates perspective by a fair bit, and I don't see any non-fragile way to extend it. I'm certainly open to extending persp-state-load to load lazily, but I'm worried about introducing unpleasant corner cases.

@azzamsa
Copy link

azzamsa commented Sep 7, 2019

I haven't try you patch, but are you aware of shell buffers restoration?.
desktop-mode can't handle it, so we need desktop+ to restore shell buffers.

is this patch can restore shell buffers?

and, I saw some unmerged pr, are waiting for nex3 review/suggestion, or just waiting for tests?

@gcv
Copy link
Collaborator Author

gcv commented Sep 7, 2019

At this point, nex3 does not have time to work on this project. @zenspider and I have just received commit access, although we have not yet worked out the patch review process between us.

As for restoring shell buffers: the patch does not restore them at the moment, though I can probably add support for eshell specifically. Have to think about that one. Supporting term.el or vterm buffers is unlikely.

@azzamsa
Copy link

azzamsa commented Sep 7, 2019

though I can probably add support for eshell specifically. Have to think about that one. Supporting term.el or vterm buffers is unlikely.

Is shell will be supported, I use shell-here all the time, really need this.

@zenspider
Copy link
Collaborator

@azzamsa maybe I don't understand... It looks to me like shell-here opens a shell up relative to the buffer you're in... This is not exactly what M-x shell already does? I have some extra stuff to rename all my shell buffers to be unique so I can have many open, but otherwise ANY process is relative to the buffer you are in (or, that buffer's current value of default-directory)

@gcv
Copy link
Collaborator Author

gcv commented Sep 9, 2019

@azzamsa: I'm reluctant to add support for shell-mode, because it will be unreliable. The whole point of saving shell buffer state is to restore the shell with the same current working directory, but shell-mode does not have good directory tracking: https://www.gnu.org/software/emacs/manual/html_node/emacs/Directory-Tracking.html#Directory-Tracking

The M-x dirs workaround fails if the cursor in the shell buffer is not at a shell prompt.

As I said, supporting eshell should not be a problem, as it does a good job of keeping default-directory up-to-date. Would you be able to switch to eshell for your use cases? In my opinion, shell-mode is the worst shell/terminal system Emacs has available. You could also try vterm, but it will not be supported with persp-state-save because it also has no directory tracking.

@azzamsa
Copy link

azzamsa commented Sep 9, 2019

@gcv okay great. https:/ffevotte/desktop-plus maybe this project can give you some glimpse.

@gcv
Copy link
Collaborator Author

gcv commented Sep 9, 2019

@azzamsa: Thanks for the reference link. I looked at it. desktop-plus indeed relies on default-directory being set correctly, which makes sense but is not good. After a little experimentation, I managed to get shell-mode to set default-directory to truly bizarre values like /../. 😱

@gcv gcv mentioned this pull request Sep 9, 2019
@azzamsa
Copy link

azzamsa commented Sep 9, 2019

managed to get shell-mode to set default-directory to truly bizarre values like /../. 😱

Haha I hope that experiment didn't take much of your productive time. Just leave shell-mode support if that so difficult to deal with.

perspective.el Outdated Show resolved Hide resolved
perspective.el Outdated Show resolved Hide resolved
perspective.el Outdated Show resolved Hide resolved
@gcv
Copy link
Collaborator Author

gcv commented Oct 2, 2019

Heads up for anyone following this PR: I rebased the underlying branch on top of the current mainline master and force-pushed. Reason: (1) some unrelated commits accidentally made it to this feature branch and I wanted to remove them, and (2) mainline recently acquired the ability to write automated tests, and I wanted to write tests for this feature before merging it in.

@gcv gcv requested a review from zenspider October 2, 2019 23:30
Copy link
Collaborator

@zenspider zenspider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a lot. As such, I don't think my review is really comprehensive enough.

Dunno if this should be a comment review or a request changes or what.

perspective.el Show resolved Hide resolved
perspective.el Show resolved Hide resolved
perspective.el Show resolved Hide resolved
persps-in-frame)))))
(make-persp--state-frame
:persps persps-in-frame
:order persp-names-in-order)))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sames... maybe I'm just agitating for a TODO: refactor / clean? comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was documented here: https:/gcv/perspective-el/blob/4a0fe25/perspective.el#L1000-L1027

I'm certainly open to suggestions, but I don't see any way to simplify the code. It just takes Emacs' representation of window layouts, does a bit of transformation on unpersistable buffers, puts the result in a struct, and writes the struct to disk.

perspective.el Show resolved Hide resolved
perspective.el Show resolved Hide resolved
perspective.el Show resolved Hide resolved
perspective.el Show resolved Hide resolved
test/test-perspective.el Show resolved Hide resolved
@zenspider
Copy link
Collaborator

zenspider commented Oct 25, 2019 via email

@zenspider
Copy link
Collaborator

Shit. Looks like my merges conflict with this. Sorry. I should have gotten this merged first since it is bigger / harder.

Once you rebase, please merge. We need this complexity in master and behind us.

@gcv
Copy link
Collaborator Author

gcv commented Oct 27, 2019

No big deal. It was a very minor conflict.

@gcv gcv merged commit 0a36c1d into nex3:master Oct 27, 2019
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

Successfully merging this pull request may close these issues.

6 participants