search

alphapapa / plz.el

An HTTP library for Emacs
GNU General Public License v3.0
200 stars 11 forks source link

Don't yes-or-no-p query the user when killing plz process buffers #52

Closed mkcms closed 4 months ago

mkcms commented 4 months ago

The current behavior when interrupting a sync request by C-g or in other way, externally - e.g. with with-timeout, Emacs unexpectedly asks if I really want to kill the curl process buffer. E.g. when interrupting this long-running request with C-g:

(plz 'get "http://httpbin.org/delay/10")

Emacs asks if I want to really kill the curl buffer. This is unexpected, as the library itself should never be interactive. It even does this in --batch mode, which I caught here: https://github.com/mkcms/compiler-explorer.el/actions/runs/9200670124/job/25307608066#step:6:148

I have signed the FSF papers a couple of years ago and already contributed to Emacs, so this is good to go.

alphapapa commented 4 months ago

Hi Michał,

Ah, this is great! I just had this happen to me yesterday when I was testing something while pressing C-g. I hadn't thought about this variable.

What do you think about using defsubst or define-inline for plz--kill-buffer? It would seem reasonable to me, to avoid an extra function call.

Thanks for submitting this!

cc: @josephmturner

josephmturner commented 4 months ago

Thank you @mkcms! I wonder if it might be more appropriate to temporarily bind kill-buffer-query-functions to (cl-callf2 remq 'process-kill-buffer-query-function kill-buffer-query-functions) instead of nil. OTOH, remq would allow for greater flexibility. OTOH, kill-buffer-query-functions is not a defcustom, so customizability might not be important in this case.

mkcms commented 4 months ago

@josephmturner

I wonder if it might be more appropriate to temporarily bind kill-buffer-query-functions to (cl-callf2 remq 'process-kill-buffer-query-function kill-buffer-query-functions) instead of nil

There's no need, because we actually do want to unconditionally kill the buffer. So we don't want any query. If another user function, apart from process-kill-buffer-query-function remains in there, and actually prevents the buffer from being killed (or just also asks us for confirmation), that won't really fix the problem.

@alphapapa

What do you think about using defsubst or define-inline for plz--kill-buffer? It would seem reasonable to me, to avoid an extra function call.

Sure, I can change that if you want, but I get this warning:

Warning: Optimization failure for plz--kill-buffer: Handler: plz--kill-buffer--inliner
(wrong-type-argument stringp (process-buffer process))

I rarely use these - do you think it's worth it?

josephmturner commented 4 months ago

There's no need, because we actually do want to unconditionally kill the buffer. So we don't want any query. If another user function, apart from process-kill-buffer-query-function remains in there, and actually prevents the buffer from being killed (or just also asks us for confirmation), that won't really fix the problem.

That sounds reasonable to me. I was imagining a scenario where for some reason (debugging, maybe?) the user wants to never kill any process buffers. The user could add a different hook, and that would ensure the buffer stayed alive.

OTOH, if we only remq the default hook, other hooks might prompt for confirmation in an undesired way.

Whatever you guys decide is fine by me. My example user story is pretty far-fetched - I'm just trying to be thorough :)

alphapapa commented 4 months ago

According to the Elisp manual, if generate-new-buffer is called with the inhibit-buffer-hooks argument, kill-buffer-query-functions are not called. So it would probably be better if we just passed that argument when we call generate-new-buffer.

mkcms commented 4 months ago

@alphapapa

if generate-new-buffer is called with the inhibit-buffer-hooks argument, kill-buffer-query-functions are not called

Nice, that's even better. I've updated the PR to do that.

alphapapa commented 4 months ago

@mkcms Thank you!

josephmturner commented 4 months ago

Note that the current solution only works on Emacs 28+. We were not able to backport generate-new-buffer's optional INHIBIT-BUFFER-HOOKS argument in compat. See this comment for details.

alphapapa commented 4 months ago

@josephmturner Thanks. So do we need to bind kill-buffer-query-functions on Emacs 27 after all?

I can't reopen this PR, so we'll have to make a new issue.

josephmturner commented 4 months ago

@alphapapa Yes, I believe so. I think we could apply the same patch that @mkcms wrote, and then add a TODO to deprecate plz--kill-buffer when plz stops supporting Emacs 27.