Implement autoshutdown when killing last buffer (#305)

[ilohmar]

This should close issue #217, also cf. #270.

• eglot.el: New defcustom `eglot-autoshutdown' (default nil).
  (eglot--managed-mode-onoff): Shutdown if so configured and no managed
  buffers left.

[ilohmar]

This is not working nicely yet, b/c of the order w.r.t. the didClose notification... I will think about the cleanest way to handle that and re-open the PR then; sorry.

[nemethf]

I don't understand why would anyone want to keep eglot-autoshutdown in nil. Is it because if users close a project and open another one of same kind, then they have to wait a lot in case of some servers? I think it would be helpful if the documentation of the variable contained guidance on why/when the variable should be set.

[joaotavora]

I don't understand why would anyone want to keep eglot-autoshutdown in nil

Good question but I think your answer is a reasonable case.

Anyway, if we introduce this, I think we should default it to t. And, yes, improve documentation.

[ilohmar]

Well, whenever I add a feature that changes some long-standing behavior, I start w/ it defaulting to off. Personally, I turn it on, of course, or I would not have tried to add this in the first place.

Once I get this working smoothly, I will gladly set the default to t and add a sentence mentioning the case for nil.

[joaotavora]

Well, whenever I add a feature that changes some long-standing behavior, I start w/ it defaulting to off.

This is the correct choice in 99% of cases. But in this one I think @nemethf has a good point.

[MaskRay]

lsp-mode in the past had the auto-close behavior. In August 2018, it introduced (defcustom lsp-keep-workspace-alive t and behaved like eglot. I think the auto-close behavior is handy if the startup time is small. However, if it takes a while, keeping it alive may be better. I am recently playing with Julia and the language server takes tens of seconds to start (no kidding, slow startup is a known problem in the Julia community. I should use PackageCompile.jl to precompile the package but it does not work currently).

I vote for keeping the server alive as the default.

[joaotavora]

@MaskRay OK, 2 for alive, 1 for autoshutdown, I abstain to be honest. If no one else voltes let's merge this with the current default behaviour but anyway @ilohmar has to fix problems in the meantime. @ilohmar what problemsn are those specifically?

[ilohmar]

eglot--managed-mode-on-off is on kill-buffer-hook before sending the didClose notification. So when the server is shut down by killing a buffer, the didClose is still sent, the hook fails, and the buffer is not killed. This did not come up in my tests because I did not work in a clean situation.

I would rather to not just add a workaround (like swapping the order in the hook), but to think thoroughly about a good way to deal with these issues. Sth about the -onoff proxy feels off to me.

Hope to get to it soon!

[ilohmar]

Actually, after studying some of the code, the cleanest fix for now is to make sure that didClose is sent before calling --managed-mode-onoff. I will add a negative hook depth to the didClose call (a positive depth for --managed-mode-onoff would put it after the global hooks, which is very messy, IMO).

@joaotavora, I think you counted my default setting as a "vote" for the keep-alive case, that's really quite the opposite of my opinion --- as I tried to explain above, I only kept the old behavior as I did not want to discuss the default at that point. :) But since we're discussing anyway, I vote to autoshutdown, and will put that into the code as well. The "slow-startup" argument seems based on the assumption that people keep their Emacs instance running, but often kill buffers "just so", and I'd say that's not typical Emacs usage. When I kill a buffer, I am done with it for the foreseeable future, that's when I want the server to be shut down.

I would still like to simplify the onoff mechanism and integrate the cache/book-keeping into the regular minor mode function (which should make it less fragile to keep a consistent state), but that's a different PR...

[joaotavora]

I only kept the old behavior as I did not want to discuss the default at that point. :)

Which makes sense, let's first get this working and then discuss the default. I'll try to evaluate your technical arguments later in the day.

[ilohmar]

Continued at #309.