GNU bug report logs -
#51993
29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files
Previous Next
Reported by: Jim Porter <jporterbugs <at> gmail.com>
Date: Sat, 20 Nov 2021 04:30:01 UTC
Severity: normal
Tags: patch
Found in version 29.0.50
Done: Jim Porter <jporterbugs <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51993 in the body.
You can then email your comments to 51993 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sat, 20 Nov 2021 04:30:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Jim Porter <jporterbugs <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sat, 20 Nov 2021 04:30:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
When killing an emacsclient terminal via C-x C-c, it should prompt to
save the files initially passed to emacsclient. To see this in action:
$ emacs -Q --daemon
$ emacsclient -a "" -c foo.txt
$ emacsclient -a "" -c bar.txt
;; In the first client frame:
foobar ;; Insert some text
C-x C-c
;; Emacs prompts "Save file /path/to/foo.txt?..."
Now try the above, but call `(server-stop-automatically 'delete-frame)'
first (or replace `delete-frame' with `kill-terminal'; it doesn't
matter). In this case, Emacs doesn't prompt to save the file. However,
the docstring/comments in `server-save-buffers-kill-terminal' say that
it should: "Offer to save each buffer, then kill the current client. ...
Only files from emacsclient file list."
Attached is a patch to restore this behavior when stopping the server
automatically. This puts all of the logic into
`server-save-buffers-kill-terminal', which allows
`server-stop-automatically--handle-delete-frame' to be simpler. I've
also added some more detailed comments explaining the logic here, since
there are some pretty subtle aspects to it.
There's a further subtlety that I should probably mention here though:
when killing a nowait frame, it would kill Emacs entirely if that were
the last frame (even in Emacs 28, and probably earlier). The only way
(that I can think of) that this could come up would be to run:
$ emacs -Q --eval '(start-server)'
$ emacsclient -n
C-x 5 0 ;; in the non-client frame
C-x C-c ;; in the emacsclient frame
However, when doing this with a regular (non-nowait) client, the last
step would report the error "Attempt to delete the sole visible or
iconified frame". Even more oddly, it would work the *second* time you
tried to kill the client terminal, since `server-delete-client' would
set the `client' frame-parameter to nil before deleting it; on the
second attempt, Emacs thinks the frame is a non-client frame (even
though it is).
I've fixed this in the second patch by following the nowait behavior: if
you kill a client and *all* the existing frames belong to that client,
it kills Emacs entirely. I'm not sure this will come up often in
practice, but it's a fairly simple change.
Some tests would be nice to prevent this from regressing, but I'm not
sure how to write a test that starts up a daemon and connects clients to
it...
[0001-Ensure-killing-an-emacsclient-terminal-prompts-to-sa.patch (text/plain, attachment)]
[0002-Don-t-explicitly-delete-client-frames-when-killing-E.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sat, 20 Nov 2021 07:14:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 19 Nov 2021 20:29:43 -0800
>
> When killing an emacsclient terminal via C-x C-c, it should prompt to
> save the files initially passed to emacsclient. To see this in action:
>
> $ emacs -Q --daemon
> $ emacsclient -a "" -c foo.txt
> $ emacsclient -a "" -c bar.txt
>
> ;; In the first client frame:
> foobar ;; Insert some text
> C-x C-c
> ;; Emacs prompts "Save file /path/to/foo.txt?..."
>
> Now try the above, but call `(server-stop-automatically 'delete-frame)'
> first (or replace `delete-frame' with `kill-terminal'; it doesn't
> matter). In this case, Emacs doesn't prompt to save the file. However,
> the docstring/comments in `server-save-buffers-kill-terminal' say that
> it should: "Offer to save each buffer, then kill the current client. ...
> Only files from emacsclient file list."
Gregory, any comments?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 09:49:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>> When killing an emacsclient terminal via C-x C-c, it should prompt to
>> save the files initially passed to emacsclient. To see this in action:
>>
>> $ emacs -Q --daemon
>> $ emacsclient -a "" -c foo.txt
>> $ emacsclient -a "" -c bar.txt
>>
>> ;; In the first client frame:
>> foobar ;; Insert some text
>> C-x C-c
>> ;; Emacs prompts "Save file /path/to/foo.txt?..."
>>
>> Now try the above, but call `(server-stop-automatically 'delete-frame)'
>> first (or replace `delete-frame' with `kill-terminal'; it doesn't
>> matter). In this case, Emacs doesn't prompt to save the file. However,
>> the docstring/comments in `server-save-buffers-kill-terminal' say that
>> it should: "Offer to save each buffer, then kill the current client.
>> ... Only files from emacsclient file list."
>
> Gregory, any comments?
>
This is not a bug, this is the intented behavior of that feature, which
was discussed on emacs-devel and in bug#51377.
But in commit 997ca88ef44 the word "last" disappeared in the explanation
of the meaning of the symbol 'kill-terminal': "when the last frame is
being closed" became "when the terminal is killed". Hence the confusion.
I attached a patch which preserves the intended behavior of that feature,
and adds a fourth possible behavior, the one Jim now wants.
[Improve-and-extend-server-stop-automatically.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 18:26:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 51993 <at> debbugs.gnu.org (full text, mbox):
(Cc'ing Lars since he merged the previous patch, and I want to be sure
everyone's on the same page here.)
On 11/23/2021 1:48 AM, Gregory Heytings wrote:
> This is not a bug, this is the intented behavior of that feature, which
> was discussed on emacs-devel and in bug#51377.
I started that discussion[1] (and participated throughout it), and I
don't think we actually agreed that this was the intended behavior. The
closest I see is this:
On 10/24/2021 11:08 AM, Jim Porter wrote[2]:
> I don't think this is true in general. The docstring for
> `server-save-buffers-kill-terminal' says: "If emacsclient was started
> with a list of filenames to edit, then only these files will be asked to
> be saved." As a result, some files with unsaved changes may still exist,
> so we'd want to prompt about those *before* the last frame is closed.
However, I should stress that the case I brought up above is just a
counterexample to show a problem with a previous implementation
strategy, not a full specification. That's part of why I posted patches
in bug#51377 in the hopes that an implementation would explain the
behavior I intend more precisely than prose.
The current behavior on Emacs 29 certainly isn't what I personally
intended when bringing the idea up on emacs-devel.
On 11/23/2021 1:48 AM, Gregory Heytings wrote:
> I attached a patch which preserves the intended behavior of that
> feature, and adds a fourth possible behavior, the one Jim now wants.
I'm concerned that we're now up to 4 different behaviors, when I think
two of them are just the result of a miscommunication between the two of
us. The way I've interpreted our prior discussion is that you would
prefer the daemon to be killed invisibly if there's no reason to keep it
alive; this is the `empty' option in your patches. On the other hand,
I'd prefer to the daemon to be killed "loudly" when there are no
non-daemon frames left, including being prompted to save buffers, kill
processes, etc in all the "usual" cases; this is `delete-last-frame' in
your patch, plus a couple of other tweaks I have pending. (Note: Just to
be clear, this isn't a specification; it's only a brief summary.)
I think your message to me in bug#51377 encapsulates this well:
On 10/24/2021 2:37 PM, Gregory Heytings wrote[3]:
> I see. We have different mental models, I guess. From my viewpoint
> the Emacs server should stay there until it's not necessary, and I'd be
> surprised to be queried about what to do with buffers opened of
> processes started in a frame I already closed when I want to close
> another frame. But of course I do not object to have both behaviors.
However, in your next two messages in the bug, you added:
On 10/26/2021 3:37 AM, Gregory Heytings wrote[4]:
> If it's now also necessary to kill the daemon when you close the
> last Emacs frame with the window manager close button (I did not see
> this requirement in your original post)...
On 10/26/2021 4:59 AM, Gregory Heytings wrote[5]:
> It just occurred to me that it's very easy to add a third behavior,
> namely the one you expect...
(The "third behavior" is the `delete-frame' option.) As I understand
things, both the `kill-terminal' and `delete-frame' options were added
to support my mental model in particular (we were the only two people
commenting on the bug at that time). From my perspective,
`kill-terminal` (and now `kill-last-terminal' as well) are just the
result of some miscommunication between the two of us in bug#51377.
Unless there's a strong argument for keeping them around, I think it
would be best to remove them so that there are only 2 options (well, 3
if you include the default behavior).
I hope the above explains things reasonably thoroughly for everyone
(hence all the citations to previous discussions). If pressed, I could
probably create a full specification of the behavior I'd like to see,
but I find it quite a bit easier to explain by way of a patch. If anyone
needs clarification on any of the above, just let me know and I'll try
to elaborate.
[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html
[2] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html
[3] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02207.html
[4] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02367.html
[5] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02373.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 20:38:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 51993 <at> debbugs.gnu.org (full text, mbox):
>> This is not a bug, this is the intented behavior of that feature
>
> I started that discussion (and participated throughout it), and I don't
> think we actually agreed that this was the intended behavior.
>
This is the behavior I intended (and described in the docstring and
manual), if you prefer. And you did not make further comments in
bug#51377, which can be interpreted as a kind of agreement.
>
> I should stress that the case I brought up above is just a
> counterexample to show a problem with a previous implementation strategy
>
Which problem?
>
> The current behavior on Emacs 29 certainly isn't what I personally
> intended when bringing the idea up on emacs-devel.
>
Is the current behavior of Emacs 29 with my patch and
(server-stop-automatically 'kill-terminal) still not what you want? If
not, what is missing?
>
> I'm concerned that we're now up to 4 different behaviors, when I think
> two of them are just the result of a miscommunication between the two of
> us.
>
They are not, AFAICS. The four behaviors are four reasonable options,
each of which can (and is) described in a short paragraph, and corresponds
to a different user preference. I see no reason to remove any of the
current three behaviors because of an unspecified "problem". Especially
given that all these behaviors are implemented in only ~50 lines of Lisp.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 22:09:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 11/23/2021 12:37 PM, Gregory Heytings wrote:
>
>>> This is not a bug, this is the intented behavior of that feature
>>
>> I started that discussion (and participated throughout it), and I
>> don't think we actually agreed that this was the intended behavior.
>>
>
> This is the behavior I intended (and described in the docstring and
> manual), if you prefer. And you did not make further comments in
> bug#51377, which can be interpreted as a kind of agreement.
Unfortunately, I was sidetracked by other things and didn't have a
chance to comment before Lars merged the patch. Since it had already
been merged, I thought it best to follow up in a separate bug once I had
made concise steps to reproduce the issue and a patch to fix it.
>> I should stress that the case I brought up above is just a
>> counterexample to show a problem with a previous implementation strategy
>>
>
> Which problem?
Prior to that comment, your proposed implementation would kill Emacs on
a timer when there were no non-daemon frames left, which could result in
unsaved changes to files being lost. I replied to point that out and
showed some steps to reproduce it:
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html>.
>> The current behavior on Emacs 29 certainly isn't what I personally
>> intended when bringing the idea up on emacs-devel.
>>
>
> Is the current behavior of Emacs 29 with my patch and
> (server-stop-automatically 'kill-terminal) still not what you want? If
> not, what is missing?
If I'm understanding your patch, the behavior I'm looking for is
essentially a combination of `kill-terminal' and `delete-last-frame'. I
may be misunderstanding it though, since the call tree in your patch
confuses me a bit: with `kill-terminal',
`server-save-buffers-kill-terminal` calls
`server-stop-automatically--handle-delete-frame', which then calls
`server-save-buffers-kill-terminal' again.
One of my other goals in my patch was to simplify the logic in
`server-save-buffers-kill-terminal' and
`server-stop-automatically--handle-delete-frame' somewhat. Rather than
to have `server-stop-automatically--handle-delete-frame' check if it was
called by `save-buffers-kill-terminal', I found that the implementation
was simpler (to me, anyway) if that logic was lifted up into
`server-save-buffers-kill-terminal'.
One benefit of this simplification is that it causes fewer changes in
behavior compared to not using `server-stop-automatically'. For example,
normally when a user kills an emacsclient terminal, Emacs will prompt
about saving files *before* deleting any frames. This is nice because it
allows the user to back out by pressing C-g, leaving Emacs in (almost)
the same state it was previously. My patch handles that and allows the
user to press C-g and leave all the current frames open.
With your patch in this bug, using `kill-terminal' and pressing C-x C-c
will close all frames for the current client but the current one, and
only then prompt the user to save buffers. Thus, pressing C-g will leave
the user with only that last client frame still open.
(Note: to test this behavior, you probably need multiple clients open as
I outlined in the first post to this bug.)
>> I'm concerned that we're now up to 4 different behaviors, when I think
>> two of them are just the result of a miscommunication between the two
>> of us.
>
> They are not, AFAICS. The four behaviors are four reasonable options,
> each of which can (and is) described in a short paragraph, and
> corresponds to a different user preference. I see no reason to remove
> any of the current three behaviors because of an unspecified "problem".
> Especially given that all these behaviors are implemented in only ~50
> lines of Lisp.
I've specified the problems. I can try to clarify if there's any
confusion though. This bug is one such problem.
I don't think that a user who opts in to stopping the Emacs daemon
automatically is *also* opting in to changing the behavior of whether
Emacs will prompt about saving files when killing a (non-last) client.
Since there are other clients, the daemon won't be killed, and so the
behavior should be identical to what happens without
`server-stop-automatically'. As a user, I would find it very strange
that enabling `server-stop-automatically' would change Emacs' behavior
in ways *other than* stopping the server in certain cases.
Of course, a user may indeed want to be able to kill a client (but not
the daemon) without being prompted to save files, but I think that's
independent of whether the daemon should be stopped when the last client
exits. If users *do* want this behavior, we could add a totally separate
option for it; this would allow users who don't want to be prompted but
also don't want `server-stop-automatically' to use it.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 22:50:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 51993 <at> debbugs.gnu.org (full text, mbox):
>>> I should stress that the case I brought up above is just a
>>> counterexample to show a problem with a previous implementation
>>> strategy
>>
>> Which problem?
>
> Prior to that comment, your proposed implementation would kill Emacs on
> a timer when there were no non-daemon frames left, which could result in
> unsaved changes to files being lost. I replied to point that out and
> showed some steps to reproduce it:
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html>.
>
I don't think it's useful to discuss a problem that was raised and fixed a
month ago.
>
> If I'm understanding your patch, the behavior I'm looking for is
> essentially a combination of `kill-terminal' and `delete-last-frame'.
>
If so, a fifth behavior could be implemented. But you would have to
describe what you want, and it should be something reasonable. Could you
please do that, and explain what you want when:
1. You delete a frame that is not the last one
2. You delete the last frame
3. You kill a terminal that is not the last one
4. You kill a terminal that is the last one
>
> I've specified the problems. I can try to clarify if there's any
> confusion though. This bug is one such problem.
>
As I said earlier, the problem you described in this bug report was not a
bug, at least in the sense that it was not something that was not
explicitly intended by the one who wrote the code (and documented). And
the behavior you wanted is handled by the patch I sent, without removing
any of the other existing behaviors. But now you apparently want
something else again.
>
> I don't think that a user who opts in to stopping the Emacs daemon
> automatically is *also* opting in to changing the behavior of whether
> Emacs will prompt about saving files when killing a (non-last) client.
> Since there are other clients, the daemon won't be killed, and so the
> behavior should be identical to what happens without
> `server-stop-automatically'. As a user, I would find it very strange
> that enabling `server-stop-automatically' would change Emacs' behavior
> in ways *other than* stopping the server in certain cases.
>
Yet this is what you're asking. If you want Emacs to prompt you whether
the files should be saved and the process killed when you delete the last
frame, you want to change the way Emacs prompts about saving files and
killing processes, because this (namely prompting the user when the last
frame is deleted) isn't happening without server-stop-automatically.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 23 Nov 2021 23:44:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 51993 <at> debbugs.gnu.org (full text, mbox):
Eli, Lars: I'm not sure what either of you would like to do about this
bug, but it seems that most of the conflict is due to a miscommunication
between me and Gregory (that's my impression, anyway). I hope my
previous messages have explained my thoughts on the matter fairly
thoroughly (if not, just let me know). However, I'm not sure there's
much more I can add to the discussion beyond this message.
On 11/23/2021 2:49 PM, Gregory Heytings wrote:
> As I said earlier, the problem you described in this bug report was not
> a bug, at least in the sense that it was not something that was not
> explicitly intended by the one who wrote the code (and documented). And
> the behavior you wanted is handled by the patch I sent, without removing
> any of the other existing behaviors. But now you apparently want
> something else again.
I've only wanted one behavior since I started this discussion on Oct
19[1]. However, rather than making sure we understand each other's
descriptions (or consulting the patches I've posted throughout the
discussion) and have properly identified the corner cases to handle,
you've instead implemented the behavior "for" me, even though I said
from the beginning that I was looking to write the patch myself. I never
posted a rigorous specification of the behavior I wanted for that
reason: I was soliciting feedback to develop a patch that meets my needs
(along with the needs of anyone who spoke up at the time, if possible).
The fact that you opted to help by authoring your own patches is
appreciated, but it ultimately doesn't help me because we seem to be
talking at cross purposes and your impressions of what I want aren't
what I actually want. Moreover, if our interpretations *don't* match up
and I bring up an issue with a proposed patch, that doesn't mean that I
want an additional option or that I've changed my mind; it just means
that we haven't reached an understanding yet.
I certainly don't expect you to do any additional work here. I'm
perfectly happy to provide patches implementing the behavior I have in
mind, and to adjust them as needed if you or anyone else has feedback on
them. While I could probably construct a rigorous specification for the
behavior I want so that someone else (e.g. you) could implement it, that
would probably end up just being a set of test cases extracted from the
patch I already have.
As an aside, I mentioned this previously, but I think it would be
valuable to write some automated test cases to verify that things work
as expected. However, I didn't see a way to test creating/destroying
Emacs servers/clients via ERT. I'm certainly open to doing so if someone
points me in the right direction though.
>> I don't think that a user who opts in to stopping the Emacs daemon
>> automatically is *also* opting in to changing the behavior of whether
>> Emacs will prompt about saving files when killing a (non-last) client.
>> Since there are other clients, the daemon won't be killed, and so the
>> behavior should be identical to what happens without
>> `server-stop-automatically'. As a user, I would find it very strange
>> that enabling `server-stop-automatically' would change Emacs' behavior
>> in ways *other than* stopping the server in certain cases.
>>
>
> Yet this is what you're asking. If you want Emacs to prompt you whether
> the files should be saved and the process killed when you delete the
> last frame, you want to change the way Emacs prompts about saving files
> and killing processes, because this (namely prompting the user when the
> last frame is deleted) isn't happening without server-stop-automatically.
That's not relevant to the case I'm discussing above. I specifically
said I'm talking about the behavior when killing the *non-last* client.
In that case, the server won't be stopped, no matter how
`server-stop-automatically' is configured.
[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Wed, 24 Nov 2021 00:00:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 51993 <at> debbugs.gnu.org (full text, mbox):
>
> I never posted a rigorous specification of the behavior I wanted for
> that reason: I was soliciting feedback to develop a patch that meets my
> needs.
>
That's not what you did, you posted a patch claiming that one of the
existing behaviors had a bug and should be replaced with another one.
>
> you've instead implemented the behavior "for" me
>
That's not what I did, I tried to implement a function that provides
different possible behaviors corresponding to different needs and
expectations, not just the behavior you expected. And I did this based on
your feedback, what others said on emacs-devel, and my own feelings. I
hoped (and thought until today) that the behavior you expected was one of
them.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Wed, 24 Nov 2021 01:11:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 11/23/2021 3:59 PM, Gregory Heytings wrote:
>>
>> I never posted a rigorous specification of the behavior I wanted for
>> that reason: I was soliciting feedback to develop a patch that meets
>> my needs.
>>
>
> That's not what you did, you posted a patch claiming that one of the
> existing behaviors had a bug and should be replaced with another one.
That's in reference to my original message on Oct 19[1], which I cited
earlier in the paragraph that you excerpted the quote from. At the time,
I said:
> I didn't see any options to configure this behavior in Emacs, but
> looking over the code, it shouldn't be that hard for me to write a patch
> to add this as an option. Before I started though, I wanted to see what
> others thought. Is this a behavior others would be interested in? If so,
> are there any other particulars I should take into account in my patch?
--------------------
On 11/23/2021 3:59 PM, Gregory Heytings wrote:
>> you've instead implemented the behavior "for" me
>>
>
> That's not what I did, I tried to implement a function that provides
> different possible behaviors corresponding to different needs and
> expectations, not just the behavior you expected. And I did this based
> on your feedback, what others said on emacs-devel, and my own feelings.
> I hoped (and thought until today) that the behavior you expected was one
> of them.
And that's fine. I have no issue supporting behaviors other than the one
I personally want. (For example, I have no problem with the `empty'
behavior, even though I wouldn't personally find it useful.) However, I
also want to be sure that we don't provide *unnecessarily* many options
here, since that adds to the maintenance burden and may produce
unexpected behavior for users if they don't quite understand the
distinction between each possible setting.
In this case, I think it's better to continue prompting users when
killing a non-last client just like Emacs 28[2] does. There might be
some value in avoiding that prompt, but I think a user who *doesn't* use
`server-stop-automatically' is just as likely to want that behavior, so
if we want to support that, we should provide a separate configuration
option to do so.
In particular, I think it's valuable to prompt users in this case
because it comes up when using emacsclient and committing code. If I set
`EDITOR="emacsclient -c"' and run `git commit', then under Emacs 28, I
can close the client with `C-x C-c'[3] and it will prompt me if I forgot
to save the commit message. That's useful because without the prompt, it
would (usually) just abort the commit due to an empty message.
I think that behavior is worth preserving. (Likewise, I think it's worth
preserving the ability to keep all your open frames by pressing C-g if
Emacs prompts you about saving files when you try to kill a client.)
Thus, I implemented the patch that you can see in the original message.
--------------------
In addition, I'm not certain that we need both `delete-frame' (or
`delete-last-frame' as it's called in your latest patch) and
`kill-terminal'. The only difference between the two is that in the
former, individually closing the last non-daemon frame (e.g. by `C-x 5
0' or clicking the X on the frame's titlebar) also kills the daemon. I
prefer the behavior of `delete-frame', since my mental model is that the
daemon should be killed *whenever* the last non-daemon frame is closed,
no matter *how* it's closed. Maybe someone has a strong opinion in the
other direction and actively wants the `kill-terminal' behavior instead.
I don't have a problem with keeping that option around if someone would
actually use it, but I'm skeptical of that. In this case though, I'm
happy to present my argument and then defer to the Emacs maintainers to
make the final call, whatever that may be. If it stays, that's fine with
me; if it goes, that's fine too.
--------------------
Since, as you mentioned before, we have different mental models for how
this works, I'm mainly trying to explain through both prose and patches
how this should work under my mental model. If I understand your prior
messages correctly, your mental model is best encapsulated by the
`empty' configuration. I'm 100% happy to keep that around as-is, even
though it's not my preferred solution. However, my preferred solution is
"something close to `delete-frame'".
Your patch in bug#51377 gets most of the way to what I wanted, but there
are a couple of corner cases that don't work like I expect, hence this
bug. Whether the behavior is a true "bug", a miscommunication, or
something else doesn't matter much to me. All I'm looking for is
something along the following lines (note: this isn't 100% rigorous, so
don't take it as a precise specification):
1) When killing a non-last client (including by closing the last of its
frames), the behavior should be exactly the same as the default
emacsclient behavior. (That's what this bug is about.)
2) When killing the last client (including by closing the last of its
frames), the behavior should be exactly the same as killing Emacs when
*not* using a server. (Roughly speaking, that means calling
`save-buffers-kill-emacs'. It would also be nice to have an option to
silence the "This Emacs session has clients; exit anyway?" prompt,
though I can certainly understand others wanting to keep it around in
this case.)
Again, that may not be 100% precise, but it's the mental model I've used
while implementing patches for this on my end. The specific
implementation I use has shifted somewhat over time as I find corner
cases, and also through consulting your patches. Using
`delete-frame-functions' as in your code greatly simplified my
implementation, for example. Thanks for that.
Hopefully my delays in following up on bug#51377 haven't been *too*
disruptive. As mentioned, I was unfortunately too busy to devote
sufficient time to it back then (my plan when posting to emacs-devel was
just to collect feedback and to work on the implementation slowly over
the next several weeks). Now my schedule is a bit less hectic, and I'm
hoping to address the last few concerns I had after taking the time to
test things out more thoroughly.
I'm certainly not trying to step on your toes or undo your patch. Just
to account for certain corner cases that I didn't have a chance to
investigate (or resolve) at the time.
[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html
[2] Or without `server-stop-automatically'
[3] You could also use `C-x #' here, but I find `C-x C-c' easier to
type, so prefer the latter in cases where either would do what I want.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 29 Nov 2021 05:40:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 51993 <at> debbugs.gnu.org (full text, mbox):
To help move things along, I've drafted an initial version of a proposal
describing possible behaviors of `server-stop-automatically' at a high
level. Hopefully that will make it easier to discuss how each setting
should work, and we can refine the proposal until we're happy with it.
Then any implementation changes should "just" be a matter of following
the final proposal.
I welcome any comments or suggested additions to this proposal. I
covered things how I see them, but I may not have provided enough detail
for everyone to understand all the nuances, and other people might have
their own use cases they'd like to see addressed.
I can also post this proposal to emacs-devel if people think it would be
better to get a wider range of feedback on it. Once we can get some
level of consensus on the proposal, then hopefully we'll know the right
thing to do about this bug.
--------------------
Automatically Shutting Down the Emacs Daemon
Revision 1
* Introduction
Since Emacs 23, `emacsclient' has had the ability to automatically start
an Emacs daemon by passing `--alternate-editor=""' (or setting the
ALTERNATE_EDITOR environment variable to the empty string). In Emacs 29,
the ability to automatically shut down the daemon was added for
symmetry. This allows users who prefer to start the Emacs daemon on an
as-needed basis to configure the daemon to stop when finished. However,
"finished" is a vague term and doesn't precisely describe the conditions
that would cause the Emacs daemon to be shut down.
Note: for the discussion below, "clients" refers to both "true clients"
(members of `server-clients') and "pseudo-clients" (each frame with the
`client' parameter set to `nowait'). This is essentially the logic used
by `server-save-buffers-kill-terminal' since Emacs 23.
* Proposed Behaviors
I propose two main ways of determining when the Emacs daemon should be
shut down automatically. Each has different pros and cons, and so there
are arguments for supporting one, both, or neither in Emacs going
forward. However, I think each are distinct enough that supporting both
would be reasonable.
** Implicit Shutdown
For this behavior, the Emacs daemon will be silently stopped once
nothing would be lost by stopping it. In particular, this means that the
Emacs daemon should continue so long as there's at least one a) active
client, b) unsaved file, or c) running subprocess (with non-nil
`process-query-on-exit-flag'). If none of these exist, then the Emacs
daemon will be stopped.
One open question is how this should interact with options that control
how `save-buffers-kill-emacs' prompts the user. If
`confirm-kill-processes' is nil, should the Emacs daemon then be stopped
even if there are active subprocesses?
*** Pros
A benefit of this behavior is that because the shutdown occurs
implicitly, there's no change to how users interact with Emacs. For
example, `save-buffers-kill-terminal' will prompt the user to save only
those files passed to `emacsclient'.
*** Cons
However, the behavior's benefit has another side to it: it can be
difficult for a user to predict whether the Emacs daemon will be
stopped. For example, a stray unsaved file that the user forgot about
could be the difference between the daemon continuing to run or being
stopped.
Furthermore, since an Emacs daemon with no clients is out of sight, it
may be some time before the user realizes their mistake, leading to
confusion about why some other program appears to be reading an
out-of-date version of the file. That said, this aspect is equally a
concern when *not* automatically stopping the Emacs daemon at all.
** Explicit Shutdown
(Note: this is the behavior I personally prefer.) In this case, Emacs
will be stopped as soon as there are no active clients, prompting the
user to save their work as needed. In other words, when a client is
about to be deleted for any reason (calling
`server-save-buffers-kill-terminal', deleting the last frame associated
with the client, etc), then:
- If this is not the last client, behave as in Emacs 28; that is, prompt
to save any files passed to `emacsclient' before deleting the client.
- If this is the last client, prompt the user to save all their work
before killing Emacs entirely; that is, call `save-buffers-kill-emacs'.
In this case, the behavior should be identical (or as close as possible)
to killing a "regular", non-server Emacs in a similar state.
One open question is: should Emacs warn the user that it will the daemon
when deleting the last client? Currently it does so in
`server-kill-emacs-query-function', asking, "This Emacs session has
clients; exit anyway?" It may be useful to keep this warning, possibly
rewording it in this case to explain the situation better. It's also
possible that it's just extra noise and should be eliminate in this
case. (It may even make sense to provide a separate configuration option.)
*** Pros
One of the main goals with Explicit Shutdown is to resolve a concern
mentioned above: an Emacs daemon with no clients is out of sight, so
it's easy to forget about it. If the daemon is holding onto unsaved
work, it may take a while until the user realizes this. By prompting to
save *all* the user's work before closing the last client, it's much
harder to make this mistake.
*** Cons
This behavior makes it more difficult for a user to know ahead of time
whether `save-buffers-kill-terminal' (bound by default to `C-x C-c')
will kill the client or Emacs entirely. The prompt mentioned above in
`server-kill-emacs-query-function' does let the user bail out though, if
they didn't mean to kill Emacs entirely.
However, `save-buffers-kill-terminal' already has this complexity
(almost), since it will kill Emacs entirely when executed from a
non-client frame. That said, it's possible to tell what will happen in
this case by looking at the mode line: if it starts with something like
"U:@---", it's a client frame and thus an observant user will know what
`save-buffers-kill-terminal' will do.
* Delayed Shutdown
It may also be worth considering whether the Emacs daemon should be
stopped immediately when the conditions are met, or whether it would be
better to delay it by a short time. Some operations, such as `git rebase
-i', can open the $EDITOR, close it, and then reopen it in rapid
succession. In a case like that, it would be more efficient if the Emacs
daemon weren't stopped right away.
That said, I don't think this is a critical problem, and believe it
would be ok to decide what to do about this later.
* Current Behaviors
Currently, Emacs 29 supports Implicit Shutdown (called `empty') and has
two slightly-different variations on Explicit Shutdown (called
`kill-terminal' and `delete-frame'). `empty' is implemented as a timer
that periodically calls `server-stop-automatically--maybe-kill-emacs' to
check if there's any need to keep the daemon running (non-daemon frames,
unsaved files, or running processes); if not, it stops the daemon. This
differs very slightly from the proposed spec above, since it checks for
frames, not active clients. Some clients (e.g. `emacsclient --eval FOO')
don't create any frames, so it may be useful to enhance the current
implementation of Implicit Shutdown to account for this.
`kill-terminal' is implemented in
`server-stop-automatically--handle-delete-frame' (called by
`server-save-buffers-kill-terminal'). It first deletes all frames
associated with the current client *except* the current one. Then, if
there are any non-daemon frames aside from the current one, it just
deletes that frame; if it's the last non-daemon frame, it calls
`save-buffers-kill-emacs'.
`delete-frame' works like `kill-terminal' above, but will also call
`save-buffers-kill-emacs' when closing the last non-daemon frame using
other means, such as clicking the "X" in the frame's title bar on a GUI
system.
As mentioned above, `kill-terminal' and `delete-frame' work similarly to
the Explicit Shutdown behavior, but there are some differences. For
example, when killing a non-last client, `kill-terminal' and
`delete-frame' don't prompt to save files passed to `emacsclient'. When
killing the last client, they delete all the non-current frames before
calling `save-buffers-kill-emacs', meaning that pressing `C-g' to cancel
when prompted will still result in all but one frame going away.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 29 Nov 2021 12:42:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: eliz <at> gnu.org, larsi <at> gnus.org, gregory <at> heytings.org
> Date: Sun, 28 Nov 2021 21:39:46 -0800
>
> To help move things along, I've drafted an initial version of a proposal
> describing possible behaviors of `server-stop-automatically' at a high
> level. Hopefully that will make it easier to discuss how each setting
> should work, and we can refine the proposal until we're happy with it.
> Then any implementation changes should "just" be a matter of following
> the final proposal.
>
> I welcome any comments or suggested additions to this proposal. I
> covered things how I see them, but I may not have provided enough detail
> for everyone to understand all the nuances, and other people might have
> their own use cases they'd like to see addressed.
>
> I can also post this proposal to emacs-devel if people think it would be
> better to get a wider range of feedback on it. Once we can get some
> level of consensus on the proposal, then hopefully we'll know the right
> thing to do about this bug.
Thanks.
However, I'm not sure I understand the way forward, as you see it.
AFAIU, there are fundamental disagreements between you and Gregory,
and I don't think the disagreements are there because one of you
doesn't understand the proposals of the other.
So how could these disagreements be reconciled? Either you-two come
up with some compromise that is acceptable by both of you, or Lars and
myself make the decision for you (and we don't guarantee you will like
it), or things are left as they are now. What will it be?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 29 Nov 2021 13:41:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 51993 <at> debbugs.gnu.org (full text, mbox):
>
> However, I'm not sure I understand the way forward, as you see it.
> AFAIU, there are fundamental disagreements between you and Gregory, and
> I don't think the disagreements are there because one of you doesn't
> understand the proposals of the other.
>
> So how could these disagreements be reconciled?
>
I'm in the process of implementing something that should satisfy everyone,
but it's not finished yet.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 29 Nov 2021 19:13:01 GMT)
Full text and
rfc822 format available.
Message #44 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 11/29/2021 4:41 AM, Eli Zaretskii wrote:
> However, I'm not sure I understand the way forward, as you see it.
> AFAIU, there are fundamental disagreements between you and Gregory,
> and I don't think the disagreements are there because one of you
> doesn't understand the proposals of the other.
I think there may be some of both. Since, from Gregory's perspective
I've been changing what I want, and from my perspective I've always
wanted the same thing, it seems that there's at least some
miscommunication happening. Hopefully my proposal helps to explain my
position more clearly (and in a single message, rather than scattered
across several).
There may still be some disagreements, and I'm happy to incorporate
anything I've missed into the proposal if people would find that useful.
On the other hand, if it's easier to just work on a patch that everyone
would be happy with (or at least be able to tolerate), that's ok too.
> So how could these disagreements be reconciled? Either you-two come
> up with some compromise that is acceptable by both of you, or Lars and
> myself make the decision for you (and we don't guarantee you will like
> it), or things are left as they are now. What will it be?
Hopefully Gregory and I can reach a compromise; the Implicit Shutdown
case in my proposal is an initial attempt at incorporating Gregory's
desired behavior (it was the behavior Gregory first proposed in the
thread on emacs-devel). I'm primarily interested in the Explicit
Shutdown case though; I may have missed some things in that section as
well, but like I said, I'm happy to add more to it if anyone has things
they'd like to be added.
That said, no matter if Gregory and I can reach a compromise, I think it
would be helpful if you and/or Lars could take a look if you have time
to be sure the behavior makes sense to you and that it's something you'd
want to be in Emacs. Since this might have relevance for other Emacs
features (e.g. emacsclient.desktop[1]), I think it would be good to have
as many experienced eyes on this as possible.
In the end, so long as everyone is clear what the behavior should be,
and that it's documented as such so there's no confusion about what's a
bug and what's intended behavior, then I don't have any (major)
problems. Obviously, I'd be happy if my preferred behavior were a part
of Emacs, but if you and Lars don't think it should be, then I can just
continue what I've been doing: add advice to the necessary server.el
functions in my config so things work how I like.
[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01846.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 29 Nov 2021 19:32:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/29/2021 5:40 AM, Gregory Heytings wrote:
>
> I'm in the process of implementing something that should satisfy
> everyone, but it's not finished yet.
Thanks. Hopefully my proposal helps to explain more precisely my mental
model and how I'd like things to work. If there's anything that's not
clear, just let me know and I'll try to elaborate on it.
In case it would help, I've also attached the relevant bits of my Emacs
configuration, which implements the Explicit Shutdown behavior I've
described. Overall, it's similar to the first patch I posted in this
bug, but it's usable in Emacs 27+ (maybe even earlier).
However, I think it has (at least) one problem: if `server-buffer-done'
(indirectly called by `server-edit' / `C-x #') deletes the last client,
it doesn't stop the daemon. I haven't had a chance to look into this in
detail yet though. I think the most consistent behavior would be to stop
the daemon in this case, since then the logic is simply "stop the daemon
when the last client would be deleted", and we don't have to worry about
exceptions to that rule.
[server-advice.el (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sat, 01 Jan 2022 00:12:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 11/29/2021 11:31 AM, Jim Porter wrote:
> On 11/29/2021 5:40 AM, Gregory Heytings wrote:
>>
>> I'm in the process of implementing something that should satisfy
>> everyone, but it's not finished yet.
>
> Thanks. Hopefully my proposal helps to explain more precisely my mental
> model and how I'd like things to work. If there's anything that's not
> clear, just let me know and I'll try to elaborate on it.
Just checking to see if there have been any updates here. As before, I'm
happy to assist wherever needed, be it writing patches or just providing
feedback. Of course, no worries if you're busy with other things; I
don't think there's any urgent need to merge a fix.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 09 Sep 2022 17:57:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 51993 <at> debbugs.gnu.org (full text, mbox):
Gregory Heytings <gregory <at> heytings.org> writes:
>> However, I'm not sure I understand the way forward, as you see
>> it. AFAIU, there are fundamental disagreements between you and
>> Gregory, and I don't think the disagreements are there because one
>> of you doesn't understand the proposals of the other.
>>
>> So how could these disagreements be reconciled?
> I'm in the process of implementing something that should satisfy
> everyone, but it's not finished yet.
This was half a year ago, and I'm not sure whether anything further was
done here?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 09 Sep 2022 18:05:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 9/9/2022 10:55 AM, Lars Ingebrigtsen wrote:
> Gregory Heytings <gregory <at> heytings.org> writes:
>
>>> However, I'm not sure I understand the way forward, as you see
>>> it. AFAIU, there are fundamental disagreements between you and
>>> Gregory, and I don't think the disagreements are there because one
>>> of you doesn't understand the proposals of the other.
>>>
>>> So how could these disagreements be reconciled?
>
>> I'm in the process of implementing something that should satisfy
>> everyone, but it's not finished yet.
>
> This was half a year ago, and I'm not sure whether anything further was
> done here?
For what it's worth, I've been brainstorming some ideas for how to
abstract things out a bit further to make it easy for users to tune the
behavior just to their liking. I haven't gotten very far on that yet,
though.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sun, 09 Oct 2022 22:10:01 GMT)
Full text and
rfc822 format available.
Message #59 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 9/9/2022 11:04 AM, Jim Porter wrote:
> On 9/9/2022 10:55 AM, Lars Ingebrigtsen wrote:
>> Gregory Heytings <gregory <at> heytings.org> writes:
>>
>>>> However, I'm not sure I understand the way forward, as you see
>>>> it. AFAIU, there are fundamental disagreements between you and
>>>> Gregory, and I don't think the disagreements are there because one
>>>> of you doesn't understand the proposals of the other.
>>>>
>>>> So how could these disagreements be reconciled?
>>
>>> I'm in the process of implementing something that should satisfy
>>> everyone, but it's not finished yet.
>>
>> This was half a year ago, and I'm not sure whether anything further was
>> done here?
>
> For what it's worth, I've been brainstorming some ideas for how to
> abstract things out a bit further to make it easy for users to tune the
> behavior just to their liking. I haven't gotten very far on that yet,
> though.
Attached is a very WIP patch that hopefully gets across the idea I had.
Hopefully the explanation below is sufficiently-clear; if anyone has
questions, or if I missed some detail, let me know.
(Note: the first patch is just a bugfix so that 'server-delete-client'
doesn't get called when Emacs *creates* the client.)
The patch adds two new hooks: 'server-before-delete-client-functions'
and 'server-after-delete-client-functions'. These should give people the
ability to add whatever behaviors they think make sense when closing an
emacsclient connection. The default behavior is the same as the current
default (call 'save-some-buffers' before deleting the client, and do
nothing after). To do something like the 'delete-frame' configuration of
the current 'server-stop-automatically' API, you might do something like
this:
(add-hook
'server-before-delete-client-functions
(lambda (proc arg)
(when (>= 1 (seq-count
(lambda (frame)
"Return non-nil if FRAME's client is another process."
(not (equal (frame-parameter frame 'client) proc)))
(frame-list)))
;; If there are no non-daemon frames other than ones owned by
;; this client, we want to do all the prompting that
;; 'save-buffers-kill-emacs' does, but without really killing.
;; HACK ALERT: This is a pretty clumsy way to do this...
(cl-letf (((symbol-function 'kill-emacs) #'ignore))
(save-buffers-kill-emacs arg))
;; Stop running other hooks.
t)))
(add-hook 'server-after-delete-client-functions
(lambda (proc)
;; Kill emacs if the only frame left is the daemon frame.
(unless (cdr (frame-list))
(kill-emacs))))
(Some helper functions might make these hooks simpler, of course.)
This interface has some other potentially-interesting uses. For example,
the after-deletion hook could start a timer and only kill Emacs if no
new clients connected after N seconds[1]. That would be useful in cases
like `git rebase -i` where Git might run your $EDITOR multiple times in
a row. It would be a waste of CPU to shut down and restart the Emacs
daemon every time.
One thing I'm not sure about yet: should the 'empty' setting for
'server-stop-automatically' just become a hook on
'server-after-delete-client-functions'? It's not *quite* the same, since
the 'empty' setting will also leave the daemon running until any active
subprocesses finish too. Maybe it would make sense for
'server-stop-automatically' to stick around, but just be a boolean (when
non-nil, use the current 'empty' behavior)? Then we keep all the nice
features of the 'empty' setting, and the other behaviors can be
implemented via these new hooks.
[1] This might need an additional hook like
'server-after-create-client-functions'.
[0001-Only-delete-server-client-from-the-sentinel-when-the.patch (text/plain, attachment)]
[0002-WIP-Add-hooks-before-after-deleting-Emacs-clients.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Mon, 10 Oct 2022 06:05:01 GMT)
Full text and
rfc822 format available.
Message #62 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Sun, 9 Oct 2022 15:09:15 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 51993 <at> debbugs.gnu.org
>
> The patch adds two new hooks: 'server-before-delete-client-functions'
> and 'server-after-delete-client-functions'. These should give people the
> ability to add whatever behaviors they think make sense when closing an
> emacsclient connection. The default behavior is the same as the current
> default (call 'save-some-buffers' before deleting the client, and do
> nothing after). To do something like the 'delete-frame' configuration of
> the current 'server-stop-automatically' API, you might do something like
> this:
Isn't this a bit of over-engineering for such a simple problem? Why
couldn't we have a user option to decide what to do, and then just do
it? The place where we delete client frames is well determined, so
doing something sensible there should be easy.
Hooks make sense when some Lisp program needs to turn on or off some
aspects of Emacs behavior, not when a user needs to control that
behavior. For users, controlling behavior with hooks should be
reserved to relatively complex and/or obscure aspects of the behavior.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Thu, 20 Oct 2022 03:15:02 GMT)
Full text and
rfc822 format available.
Message #65 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 10/9/2022 11:04 PM, Eli Zaretskii wrote:
>> Date: Sun, 9 Oct 2022 15:09:15 -0700
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 51993 <at> debbugs.gnu.org
>>
>> The patch adds two new hooks: 'server-before-delete-client-functions'
>> and 'server-after-delete-client-functions'. These should give people the
>> ability to add whatever behaviors they think make sense when closing an
>> emacsclient connection. The default behavior is the same as the current
>> default (call 'save-some-buffers' before deleting the client, and do
>> nothing after). To do something like the 'delete-frame' configuration of
>> the current 'server-stop-automatically' API, you might do something like
>> this:
>
> Isn't this a bit of over-engineering for such a simple problem? Why
> couldn't we have a user option to decide what to do, and then just do
> it? The place where we delete client frames is well determined, so
> doing something sensible there should be easy.
I'd be happy with a simple user option, provided we can all agree how
things should work (or if we can't *all* agree, that you and Lars can
decide at least). I thought adding a hook might get around the problem
of not being able to agree, though.
I proposed a couple of behaviors that I described in as much detail as I
could in the hopes of avoiding confusion and coming to an agreement
here:
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg02245.html>.
To summarize it briefly, the behavior I would personally prefer is this.
When deleting an emacs client by any means (e.g. 'C-x C-c', clicking the
X on the last frame of a client, etc):
a) if this is not the last client, behave the same as Emacs 28:
prompt to save files specified when starting "emacsclient", and then
delete that client.
b) if this *is* the last client, prompt the user to save everything
(as with 'save-buffers-kill-emacs'), and then delete the client + kill
the Emacs daemon.
I'm certainly open to supporting other options if people have different
preferences, and I can work on a patch to support these so long as we
come to an agreement/decision/compromise about the expected behavior(s).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Thu, 20 Oct 2022 06:24:02 GMT)
Full text and
rfc822 format available.
Message #68 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Wed, 19 Oct 2022 20:14:38 -0700
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> I proposed a couple of behaviors that I described in as much detail as I
> could in the hopes of avoiding confusion and coming to an agreement
> here:
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg02245.html>.
>
> To summarize it briefly, the behavior I would personally prefer is this.
> When deleting an emacs client by any means (e.g. 'C-x C-c', clicking the
> X on the last frame of a client, etc):
>
> a) if this is not the last client, behave the same as Emacs 28:
> prompt to save files specified when starting "emacsclient", and then
> delete that client.
>
> b) if this *is* the last client, prompt the user to save everything
> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
> the Emacs daemon.
You mean, in b), instead of just deleting the frame and leaving the
daemon run, you want to shut down Emacs in its entirety, as if the
user invoked kill-emacs? I'm okay with that as an optional behavior,
although I myself won't use it, as it's too dangerous.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 21 Oct 2022 05:52:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 10/19/2022 11:23 PM, Eli Zaretskii wrote:
>> Date: Wed, 19 Oct 2022 20:14:38 -0700
>> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> a) if this is not the last client, behave the same as Emacs 28:
>> prompt to save files specified when starting "emacsclient", and then
>> delete that client.
>>
>> b) if this *is* the last client, prompt the user to save everything
>> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
>> the Emacs daemon.
>
> You mean, in b), instead of just deleting the frame and leaving the
> daemon run, you want to shut down Emacs in its entirety, as if the
> user invoked kill-emacs? I'm okay with that as an optional behavior,
> although I myself won't use it, as it's too dangerous.
Almost. I'd like it to be as if the user invoked
'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the
user about everything[1] that might be lost by killing Emacs.
This already exists as an option -- (server-stop-automatically
'delete-frame)[2], but I also find the current behavior too dangerous.
My original message outlines one of the problems with the current
implementation: it changes the behavior of (a) in my description above.
> $ emacs -Q --daemon
> $ emacsclient -a "" -c foo.txt
> $ emacsclient -a "" -c bar.txt
>
> ;; In the first client frame:
> foobar ;; Insert some text
> C-x C-c
> ;; Emacs prompts "Save file /path/to/foo.txt?..."
>
> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
As mentioned earlier in this bug, I'm open to providing a patch to
support options beyond the one I suggested (quoted at the beginning of
this message), but I want to be sure we agree on what the options should
look like so that I'm not breaking Gregory's (or anyone else's)
preferred behavior here.
(Of course, all of this would remain opt-in. I don't want to change the
default behavior here, since it would break years of established precedent.)
[1] Well, everything that Emacs usually asks about in a non-daemon
configuration, anyway.
[2] It's not hooked up to Customize, though I don't think that would be
too hard to do.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 21 Oct 2022 06:39:02 GMT)
Full text and
rfc822 format available.
Message #74 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Thu, 20 Oct 2022 22:51:42 -0700
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> >> b) if this *is* the last client, prompt the user to save everything
> >> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
> >> the Emacs daemon.
> >
> > You mean, in b), instead of just deleting the frame and leaving the
> > daemon run, you want to shut down Emacs in its entirety, as if the
> > user invoked kill-emacs? I'm okay with that as an optional behavior,
> > although I myself won't use it, as it's too dangerous.
>
> Almost. I'd like it to be as if the user invoked
> 'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the
> user about everything[1] that might be lost by killing Emacs.
That should already happen, if you just call save-buffers-kill-emacs
in that case, right?
> This already exists as an option -- (server-stop-automatically
> 'delete-frame)[2], but I also find the current behavior too dangerous.
> My original message outlines one of the problems with the current
> implementation: it changes the behavior of (a) in my description above.
>
> > $ emacs -Q --daemon
> > $ emacsclient -a "" -c foo.txt
> > $ emacsclient -a "" -c bar.txt
> >
> > ;; In the first client frame:
> > foobar ;; Insert some text
> > C-x C-c
> > ;; Emacs prompts "Save file /path/to/foo.txt?..."
> >
> > Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
I'm not sure I see the direct relevance, and I don't think I see a bug
in the above behavior. I'm probably missing something, but what?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sat, 22 Oct 2022 03:47:01 GMT)
Full text and
rfc822 format available.
Message #77 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 10/20/2022 11:38 PM, Eli Zaretskii wrote:
>> Date: Thu, 20 Oct 2022 22:51:42 -0700
>> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>>>> b) if this *is* the last client, prompt the user to save everything
>>>> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
>>>> the Emacs daemon.
>>>
>>> You mean, in b), instead of just deleting the frame and leaving the
>>> daemon run, you want to shut down Emacs in its entirety, as if the
>>> user invoked kill-emacs? I'm okay with that as an optional behavior,
>>> although I myself won't use it, as it's too dangerous.
>>
>> Almost. I'd like it to be as if the user invoked
>> 'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the
>> user about everything[1] that might be lost by killing Emacs.
>
> That should already happen, if you just call save-buffers-kill-emacs
> in that case, right?
Yeah. My expectation is that I can type 'C-x C-c' (or 'M-x
save-buffers-kill-terminal') to kill a client, and if it's the last
client, instead kill Emacs entirely (like 'M-x
save-buffers-kill-emacs'). So roughly speaking, the change would be that
you can set 'save-buffer-kill-terminal' to work like
'save-buffer-kill-emacs' when there's only 1 client left.
>> This already exists as an option -- (server-stop-automatically
>> 'delete-frame)[2], but I also find the current behavior too dangerous.
>> My original message outlines one of the problems with the current
>> implementation: it changes the behavior of (a) in my description above.
>>
>>> $ emacs -Q --daemon
>>> $ emacsclient -a "" -c foo.txt
>>> $ emacsclient -a "" -c bar.txt
>>>
>>> ;; In the first client frame:
>>> foobar ;; Insert some text
>>> C-x C-c
>>> ;; Emacs prompts "Save file /path/to/foo.txt?..."
>>>
>>> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
>
> I'm not sure I see the direct relevance, and I don't think I see a bug
> in the above behavior. I'm probably missing something, but what?
The issue in the quote above is that if you enable automatic server
shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
even when it wouldn't stop the server (i.e. when there are other active
clients). That's surprising to me, I wouldn't expect that setting to
affect cases when it decides *not* to kill the Emacs daemon.
(This is relevant to the previous discussion since fixing this would get
Emacs's automatic server shutdown - aka killing the daemon - close to
the way I described there. Gregory mentioned[1] that the current
behavior is intended, although we've had some difficulty coming to an
agreement on how all this should work. Hence why I thought a hook might
help here: if there are strong opinions in various directions, maybe a
simple option isn't enough. Or maybe this is a case where you and/or
Lars would be in a better position to make a final decision...)
[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51993
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sat, 22 Oct 2022 06:58:01 GMT)
Full text and
rfc822 format available.
Message #80 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Fri, 21 Oct 2022 20:46:47 -0700
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> >>> $ emacs -Q --daemon
> >>> $ emacsclient -a "" -c foo.txt
> >>> $ emacsclient -a "" -c bar.txt
> >>>
> >>> ;; In the first client frame:
> >>> foobar ;; Insert some text
> >>> C-x C-c
> >>> ;; Emacs prompts "Save file /path/to/foo.txt?..."
> >>>
> >>> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
> >
> > I'm not sure I see the direct relevance, and I don't think I see a bug
> > in the above behavior. I'm probably missing something, but what?
>
> The issue in the quote above is that if you enable automatic server
> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
> even when it wouldn't stop the server (i.e. when there are other active
> clients). That's surprising to me, I wouldn't expect that setting to
> affect cases when it decides *not* to kill the Emacs daemon.
Sounds like a bug to me, because it contradicts what the doc string
says.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 25 Oct 2022 03:11:01 GMT)
Full text and
rfc822 format available.
Message #83 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 10/21/2022 11:57 PM, Eli Zaretskii wrote:
>> Date: Fri, 21 Oct 2022 20:46:47 -0700
>> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> The issue in the quote above is that if you enable automatic server
>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>> even when it wouldn't stop the server (i.e. when there are other active
>> clients). That's surprising to me, I wouldn't expect that setting to
>> affect cases when it decides *not* to kill the Emacs daemon.
>
> Sounds like a bug to me, because it contradicts what the doc string
> says.
That's how it seems to me too. In that case, I can update the patch I
attached in my original message[1] to fix the bug. If we want to
preserve the current behavior that Emacs 29 has exactly, we could also
add a separate setting for how to handle killing the non-last client;
then the two would be independently customizable. I'm not sure this is
necessary, but if others think it is, I'm happy to write a patch for it.
[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg01702.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sun, 30 Oct 2022 22:34:01 GMT)
Full text and
rfc822 format available.
Message #86 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 10/24/2022 8:10 PM, Jim Porter wrote:
> That's how it seems to me too. In that case, I can update the patch I
> attached in my original message[1] to fix the bug. If we want to
> preserve the current behavior that Emacs 29 has exactly, we could also
> add a separate setting for how to handle killing the non-last client;
> then the two would be independently customizable. I'm not sure this is
> necessary, but if others think it is, I'm happy to write a patch for it.
While working through this, I found a tangentially-related bug that
occurs regardless of whether 'server-stop-automatically' is set:
bug#58909. Since the discussion here is pretty long already, I opted to
file a new issue for it, but in practice, it might be easier to write a
patch that fixes both this bug and that one; there's a fair amount of
crossover in the code for these.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Tue, 29 Nov 2022 05:32:02 GMT)
Full text and
rfc822 format available.
Message #89 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 10/24/2022 8:10 PM, Jim Porter wrote:
> On 10/21/2022 11:57 PM, Eli Zaretskii wrote:
>>> Date: Fri, 21 Oct 2022 20:46:47 -0700
>>> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>>> From: Jim Porter <jporterbugs <at> gmail.com>
>>>
>>> The issue in the quote above is that if you enable automatic server
>>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>>> even when it wouldn't stop the server (i.e. when there are other active
>>> clients). That's surprising to me, I wouldn't expect that setting to
>>> affect cases when it decides *not* to kill the Emacs daemon.
>>
>> Sounds like a bug to me, because it contradicts what the doc string
>> says.
>
> That's how it seems to me too. ...
Ok, after quite a delay, here's a patch for this. Previously, the
function 'server-stop-automatically--handle-delete-frame' responded to
both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which
made it more complex. I've moved the 'C-x C-c' case into
'server-save-buffers-kill-terminal', which simplifies
'server-stop-automatically--handle-delete-frame'.
The updated 'server-save-buffers-kill-terminal' should now make sure
that the new stop-automatically behavior only happens when there are no
other client processes (or nowait frames).
I've also attached a version of the diff excluding the whitespace
changes to make reviewing the code easier.
[0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch (text/plain, attachment)]
[no-whitespace-change.diff (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Thu, 01 Dec 2022 17:31:02 GMT)
Full text and
rfc822 format available.
Message #92 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 28 Nov 2022 21:31:02 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>
> >>> The issue in the quote above is that if you enable automatic server
> >>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
> >>> even when it wouldn't stop the server (i.e. when there are other active
> >>> clients). That's surprising to me, I wouldn't expect that setting to
> >>> affect cases when it decides *not* to kill the Emacs daemon.
> >>
> >> Sounds like a bug to me, because it contradicts what the doc string
> >> says.
> >
> > That's how it seems to me too. ...
> Ok, after quite a delay, here's a patch for this. Previously, the
> function 'server-stop-automatically--handle-delete-frame' responded to
> both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which
> made it more complex. I've moved the 'C-x C-c' case into
> 'server-save-buffers-kill-terminal', which simplifies
> 'server-stop-automatically--handle-delete-frame'.
>
> The updated 'server-save-buffers-kill-terminal' should now make sure
> that the new stop-automatically behavior only happens when there are no
> other client processes (or nowait frames).
We want this on the release branch, right? Then please make it the minimal
change which fixes the immediate cause of the bug, and does nothing else: no
refactoring, no reshuffling of the code or making it nicer or less
complicated -- all that just makes the risk of new bugs higher and the job
of reviewing the patch harder.
> + (if (length> (frame-list) (if server-stop-automatically 2 1))
> + ;; If there are any other frames, only delete this one.
> + ;; When `server-stop-automatically' is set, don't count
> + ;; the daemon frame.
> (progn (save-some-buffers arg)
> (delete-frame))
> ;; If we're the last frame standing, kill Emacs.
> (save-buffers-kill-emacs arg)))
This part is easily understood.
> ((processp proc)
> + (if (or (not server-stop-automatically)
> + (length> server-clients 1)
> + (seq-some
> + (lambda (frame)
> + (when-let ((p (frame-parameter frame 'client)))
> + (not (eq proc p))))
> + (frame-list)))
> (let ((buffers (process-get proc 'buffers)))
> (save-some-buffers
> arg (if buffers
This part is also easily understood.
> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
> ;; ARG is non-nil), since we're not killing
> ;; Emacs (unlike `save-buffers-kill-emacs').
> (and arg t)))
> - (server-delete-client proc)))
> - (t (error "Invalid client frame"))))))
> + (server-delete-client proc))
> + ;; If `server-stop-automatically' is set, there are no
> + ;; other client processes, and no other client frames
> + ;; (e.g. `nowait' frames), kill Emacs.
> + (save-buffers-kill-emacs arg)))
> + (t (error "Invalid client frame")))))
But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
the original code, and I don't understand why. The bug wasn't about this,
was it?
> (defun server-stop-automatically--handle-delete-frame (frame)
> "Handle deletion of FRAME when `server-stop-automatically' is used."
> - (when server-stop-automatically
> - (if (if (and (processp (frame-parameter frame 'client))
> - (eq this-command 'save-buffers-kill-terminal))
> - (progn
> - (dolist (f (frame-list))
> - (when (and (eq (frame-parameter frame 'client)
> - (frame-parameter f 'client))
> - (not (eq frame f)))
> - (set-frame-parameter f 'client nil)
> - (let ((server-stop-automatically nil))
> - (delete-frame f))))
> - (if (cddr (frame-list))
> - (let ((server-stop-automatically nil))
> - (delete-frame frame)
> - nil)
> - t))
> + (when (and server-stop-automatically
> (null (cddr (frame-list))))
> (let ((server-stop-automatically nil))
> (save-buffers-kill-emacs)
> - (delete-frame frame)))))
> + (delete-frame frame))))
And here you completely rewrote a function.
I'm okay with installing the original changes on master, if you indeed
believe the new code is much cleaner (but then please explain why you think
so, because I don't think I see that just by looking at the diffs). But for
the release branch, I'm not comfortable with making such serious changes in
a part of server.el that is already way too complicated, what with all the
fancy shutdown options we strive to support. There be dragons, and I have
no intention to release Emacs 29 with buggy server-client editing. So for
the release branch, please prepare a safer version of the change, which only
changes the code which is the immediate cause of the incorrect behavior.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 02 Dec 2022 01:10:01 GMT)
Full text and
rfc822 format available.
Message #95 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
>> Date: Mon, 28 Nov 2022 21:31:02 -0800
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
>>
>>>>> The issue in the quote above is that if you enable automatic server
>>>>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>>>>> even when it wouldn't stop the server (i.e. when there are other active
>>>>> clients). That's surprising to me, I wouldn't expect that setting to
>>>>> affect cases when it decides *not* to kill the Emacs daemon.
>>>>
>>>> Sounds like a bug to me, because it contradicts what the doc string
>>>> says.
>>>
>>> That's how it seems to me too. ...
>> Ok, after quite a delay, here's a patch for this. Previously, the
>> function 'server-stop-automatically--handle-delete-frame' responded to
>> both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which
>> made it more complex. I've moved the 'C-x C-c' case into
>> 'server-save-buffers-kill-terminal', which simplifies
>> 'server-stop-automatically--handle-delete-frame'.
>>
>> The updated 'server-save-buffers-kill-terminal' should now make sure
>> that the new stop-automatically behavior only happens when there are no
>> other client processes (or nowait frames).
>
> We want this on the release branch, right? Then please make it the minimal
> change which fixes the immediate cause of the bug, and does nothing else: no
> refactoring, no reshuffling of the code or making it nicer or less
> complicated -- all that just makes the risk of new bugs higher and the job
> of reviewing the patch harder.
Thanks for taking a look. I believe we'd want this on the release
branch. Here's the absolute minimum I could manage, although it doesn't
quite fix everything that my previous patch does. In particular, when
server-stop-automatically is set to 'kill-terminal' (or 'delete-frame'):
* If you type 'C-x C-c' ('save-buffers-kill-terminal') in a nowait
client frame, and there are still other (non-daemon) frames, nothing
happens. You'd have to use 'C-x 5 0' ('delete-frame') instead. Fixing
this would basically mean doing 90% of my original patch, so it's
probably too risky.
* If you type 'C-u C-x C-c', it doesn't silently save all the relevant
buffers. That's because 'server-save-buffers-kill-terminal' doesn't
forward the prefix arg to
'server-stop-automatically--handle-delete-frame'. That's a separate (but
closely related) bug, so I didn't fix that either.
In a followup message, I'll show the breakdown of my previous patch into
smaller steps with some more detailed explanation of why I think it
simplifies things enough to be worth making the change on the master branch.
[0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 02 Dec 2022 01:43:02 GMT)
Full text and
rfc822 format available.
Message #98 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
> I'm okay with installing the original changes on master, if you indeed
> believe the new code is much cleaner (but then please explain why you think
> so, because I don't think I see that just by looking at the diffs). But for
> the release branch, I'm not comfortable with making such serious changes in
> a part of server.el that is already way too complicated, what with all the
> fancy shutdown options we strive to support. There be dragons, and I have
> no intention to release Emacs 29 with buggy server-client editing. So for
> the release branch, please prepare a safer version of the change, which only
> changes the code which is the immediate cause of the incorrect behavior.
Attached is a patch series that explains in more detail how I arrived at
the previous version of my patch. This is basically a reconstruction of
the steps I took when writing it originally. I'll describe my overall
plan and then address the specific comments you had after that. (This
might be overly-verbose, but I wanted to put as much detail in as I
could in the hopes of addressing all your concerns.)
Prior to my patch, 'server-stop-automatically--handle-delete-frame'
(henceforth 'SSA--handle-delete-frame') can get called in two
situations: when someone calls 'delete-frame' (it's a hook on
'delete-frame-functions') or when someone calls
'save-buffers-kill-emacs' ('server-save-buffers-kill-emacs' delegates to
it when configured to). To help make the logic easier to follow, I split
it into two: one for handling 'delete-frame', and one for handling
'save-buffers-kill-terminal'. See patches 0001 and 0002.
>> (defun server-stop-automatically--handle-delete-frame (frame)
>> "Handle deletion of FRAME when `server-stop-automatically' is used."
>> - (when server-stop-automatically
>> - (if (if (and (processp (frame-parameter frame 'client))
>> - (eq this-command 'save-buffers-kill-terminal))
>> - (progn
>> - (dolist (f (frame-list))
>> - (when (and (eq (frame-parameter frame 'client)
>> - (frame-parameter f 'client))
>> - (not (eq frame f)))
>> - (set-frame-parameter f 'client nil)
>> - (let ((server-stop-automatically nil))
>> - (delete-frame f))))
>> - (if (cddr (frame-list))
>> - (let ((server-stop-automatically nil))
>> - (delete-frame frame)
>> - nil)
>> - t))
>> + (when (and server-stop-automatically
>> (null (cddr (frame-list))))
>> (let ((server-stop-automatically nil))
>> (save-buffers-kill-emacs)
>> - (delete-frame frame)))))
>> + (delete-frame frame))))
>
> And here you completely rewrote a function.
In patch 0002, you can see (most of) this change that you mentioned:
since I made one function for each case as described above, the second
'if' statement's conditional is always false, so I just got rid of the
conditional and the then-clause, leaving only the else-clause: (null
(cddr (frame-list))). I also simplified this function a bit in the last
patch, 0007.
Now for the rest of the patch series.
The original bug is that the behavior of
'server-save-buffers-kill-terminal' when there are multiple clients
should be the same regardless of the SSA setting (it wasn't). So next, I
made 'SSA--handle-kill-terminal' have the same basic structure as
'server-save-buffers-kill-terminal' (patch 0003). That makes it easier
to see the differences between the two.
Patch 0005 is where the real fix is: it makes sure that when we *don't*
want to kill Emacs, 'SSA--handle-kill-terminal' does the same thing as
'server-save-buffers-kill-terminal'.
>> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>> ;; ARG is non-nil), since we're not killing
>> ;; Emacs (unlike `save-buffers-kill-emacs').
>> (and arg t)))
>> - (server-delete-client proc)))
>> - (t (error "Invalid client frame"))))))
>> + (server-delete-client proc))
>> + ;; If `server-stop-automatically' is set, there are no
>> + ;; other client processes, and no other client frames
>> + ;; (e.g. `nowait' frames), kill Emacs.
>> + (save-buffers-kill-emacs arg)))
>> + (t (error "Invalid client frame")))))
>
> But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
> the original code, and I don't understand why. The bug wasn't about this,
> was it?
In patch 0005, you can start to see this block of code take shape:
because we want to handle the "don't kill Emacs" case in
'SSA--handle-kill-terminal', we add the 'save-some-buffers' +
'server-delete-client' code there, resulting in something that looks
similar to the above hunk.
Then, in patch 0006, I just merge 'server-save-buffers-kill-terminal'
and 'SSA--handle-kill-terminal', since most of the code is shared at
this point. Finally, patch 0007 is just a bit of cleanup; with all of
these applied, server.el should be identical to my previous patch.
Hopefully this explains things reasonably well, and doesn't go into too
much (or too little) detail. If there are any other bits that you have
concerns about, just let me know.
[0001-Duplicate-server-stop-automatically-handle-delete-fr.patch (text/plain, attachment)]
[0002-Simplify-server-stop-automatically-handlers.patch (text/plain, attachment)]
[0003-Restructure-server-stop-automatically-handle-kill-te.patch (text/plain, attachment)]
[0004-Remove-unnecessary-delete-frame-calls-after-save-buf.patch (text/plain, attachment)]
[0005-This-is-the-commit-that-actually-fixes-bug-51993.patch (text/plain, attachment)]
[0006-Merge-kill-terminal-implementations.patch (text/plain, attachment)]
[0007-Simplify-server-stop-automatically-handle-delete-fra.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 02 Dec 2022 14:11:02 GMT)
Full text and
rfc822 format available.
Message #101 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Please in the future avoid changing the Subject when discussing bugs: it
makes it harder for me and others to follow the discussion by grouping
messages by Subject.]
> Date: Thu, 1 Dec 2022 17:09:24 -0800
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> > We want this on the release branch, right? Then please make it the minimal
> > change which fixes the immediate cause of the bug, and does nothing else: no
> > refactoring, no reshuffling of the code or making it nicer or less
> > complicated -- all that just makes the risk of new bugs higher and the job
> > of reviewing the patch harder.
>
> Thanks for taking a look. I believe we'd want this on the release
> branch. Here's the absolute minimum I could manage, although it doesn't
> quite fix everything that my previous patch does.
Thanks. Surprisingly, the previous version was easier to review and agree
to in some of its parts, because it kept more of the original code, and only
changed the conditions when save-some-buffers or save-buffers-kill-emacs
were called. This version of the patch completely rewrites the affected
code. Was that really necessary?
> In particular, when
> server-stop-automatically is set to 'kill-terminal' (or 'delete-frame'):
>
> * If you type 'C-x C-c' ('save-buffers-kill-terminal') in a nowait
> client frame, and there are still other (non-daemon) frames, nothing
> happens. You'd have to use 'C-x 5 0' ('delete-frame') instead. Fixing
> this would basically mean doing 90% of my original patch, so it's
> probably too risky.
>
> * If you type 'C-u C-x C-c', it doesn't silently save all the relevant
> buffers. That's because 'server-save-buffers-kill-terminal' doesn't
> forward the prefix arg to
> 'server-stop-automatically--handle-delete-frame'. That's a separate (but
> closely related) bug, so I didn't fix that either.
Please tell more about why these two situations cannot be handled by
similarly simple changes.
What made the original patch risky from my POV is that it rewrote too much
of the original code. Would it be possible to retain as much as possible of
the original code, and just change the conditions for calling
save-some-buffers etc., and/or add calls to it where it previously wasn't
called for some reason?
Please understand where I stand: use of emacsclient is very popular and very
widespread, so I'd like to avoid adding regressions to it as result of
enhancements we make. If I conclude that the new features installed as part
of Emacs 29 development are too complicated and/or include semi-buggy
aspects, I'd rather revert those new features than risk regressions in
emacsclient and in the server behavior.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 02 Dec 2022 14:33:02 GMT)
Full text and
rfc822 format available.
Message #104 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Thu, 1 Dec 2022 17:42:18 -0800
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> Attached is a patch series that explains in more detail how I arrived at
> the previous version of my patch. This is basically a reconstruction of
> the steps I took when writing it originally. I'll describe my overall
> plan and then address the specific comments you had after that. (This
> might be overly-verbose, but I wanted to put as much detail in as I
> could in the hopes of addressing all your concerns.)
Thanks, but unfortunately this methodology doesn't make it easier for me to
review the changes.
Personally, I'd prefer to have a single place with the logic of what happens
when the user closes the last client frame or the last frame on a specific
terminal. But I'd like to make progress in this anyway, at least on master.
So I'm okay with having your code installed on master as it is, if that is
what you prefer.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Fri, 02 Dec 2022 21:34:02 GMT)
Full text and
rfc822 format available.
Message #107 received at 51993 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 12/2/2022 6:10 AM, Eli Zaretskii wrote:
> [Please in the future avoid changing the Subject when discussing bugs: it
> makes it harder for me and others to follow the discussion by grouping
> messages by Subject.]
Sorry about that. I thought it would be easier to see that I'd split the
topic into two separate tracks. I'll keep that in mind for the future
though.
> Thanks. Surprisingly, the previous version was easier to review and agree
> to in some of its parts, because it kept more of the original code, and only
> changed the conditions when save-some-buffers or save-buffers-kill-emacs
> were called.
I think I must have misinterpreted where your main concerns were, and
went down the wrong avenue as a result. I think your other comments here
(and re-reading your previous message) have helped me get a better idea
of your concerns, so I'll try to address your earlier message better.
On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
>> (defun server-stop-automatically--handle-delete-frame (frame)
>> "Handle deletion of FRAME when `server-stop-automatically' is used."
[snip -- long diff removed]
>
> And here you completely rewrote a function.
Thinking about this some more, that change wasn't strictly necessary for
29.1, since it should really just be dead code, and won't hurt anything.
I've split this part out into a separate commit that we could apply to
only the master branch. That simplifies the first commit (for 29.1) a
fair bit.
>> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>> ;; ARG is non-nil), since we're not killing
>> ;; Emacs (unlike `save-buffers-kill-emacs').
>> (and arg t)))
>> - (server-delete-client proc)))
>> - (t (error "Invalid client frame"))))))
>> + (server-delete-client proc))
>> + ;; If `server-stop-automatically' is set, there are no
>> + ;; other client processes, and no other client frames
>> + ;; (e.g. `nowait' frames), kill Emacs.
>> + (save-buffers-kill-emacs arg)))
>> + (t (error "Invalid client frame")))))
>
> But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
> the original code, and I don't understand why. The bug wasn't about this,
> was it?
I believe this change is the most important part of the patch, so I'll
try to explain why I added this. The 'server-stop-automatically' feature
causes the Emacs daemon to be killed when certain conditions are met.
For the 'kill-terminal' case[1], that means that, "when the last client
frame is being closed with C-x C-c, you are asked whether [various
questions], and if so, the server is stopped." (From the manual section
on this.)
The "last client frame" check is the conditional just before this hunk
(you mentioned that this part was fine, so I didn't include it in this
message). The latter part of the sentence in the manual is just a
description of 'save-buffers-kill-emacs'. So I've tried to take the text
of the manual and turn it into code.
Another way of looking at it: the new 'save-buffers-kill-emacs' call
above is really the result of me moving the handling for this condition
from 'server-stop-automatically--handle-delete-frame' into
'server-save-buffers-kill-terminal'. Previously, that function was
responsible for calling 'save-buffers-kill-emacs' at the right time.
Doing it this way lets me avoid calling 'SSA--handle-delete-frame' so
that we get the benefit of using the longstanding implementation of
'server-save-buffers-kill-terminal', just with the minimum of necessary
enhancements to make this stop-automatically setting behave as documented.
[1] This also applies to 'delete-frame', but that setting does more
beyond this.
[29.1--0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch (text/plain, attachment)]
[master--0002-Remove-dead-code-from-server-stop-automatically-hand.patch (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sun, 04 Dec 2022 17:58:02 GMT)
Full text and
rfc822 format available.
Message #110 received at 51993 <at> debbugs.gnu.org (full text, mbox):
> Date: Fri, 2 Dec 2022 13:33:39 -0800
> Cc: larsi <at> gnus.org, 51993 <at> debbugs.gnu.org, gregory <at> heytings.org
> From: Jim Porter <jporterbugs <at> gmail.com>
>
> > And here you completely rewrote a function.
>
> Thinking about this some more, that change wasn't strictly necessary for
> 29.1, since it should really just be dead code, and won't hurt anything.
> I've split this part out into a separate commit that we could apply to
> only the master branch. That simplifies the first commit (for 29.1) a
> fair bit.
Thanks, I'm okay with committing these two patches, each one to its branch.
(But I guess you need to wait with installing the second one until the first
one is merged to master, right?)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#51993; Package
emacs.
(Sun, 04 Dec 2022 22:27:01 GMT)
Full text and
rfc822 format available.
Message #113 received at 51993 <at> debbugs.gnu.org (full text, mbox):
On 12/4/2022 9:56 AM, Eli Zaretskii wrote:
> Thanks, I'm okay with committing these two patches, each one to its branch.
> (But I guess you need to wait with installing the second one until the first
> one is merged to master, right?)
Thanks again for the reviews. I've merged the first part to the 29
branch as 4bcdb1cc65bf779b6479f99a7aa767ab83b3bae1.
Like you said, the second patch (for master) should wait until the next
merge of 29 to master. There's no rush on this though, so I'll just set
a reminder to do that when it's ready. (I could theoretically merge 29
to master myself, but since I've never done that, I'll leave it to the
experts. :))
Reply sent
to
Jim Porter <jporterbugs <at> gmail.com>:
You have taken responsibility.
(Tue, 06 Dec 2022 22:21:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Jim Porter <jporterbugs <at> gmail.com>:
bug acknowledged by developer.
(Tue, 06 Dec 2022 22:21:02 GMT)
Full text and
rfc822 format available.
Message #118 received at 51993-done <at> debbugs.gnu.org (full text, mbox):
On 12/4/2022 2:26 PM, Jim Porter wrote:
> Thanks again for the reviews. I've merged the first part to the 29
> branch as 4bcdb1cc65bf779b6479f99a7aa767ab83b3bae1.
>
> Like you said, the second patch (for master) should wait until the next
> merge of 29 to master.
And now I've merged the cleanup patch to master as
bcf4d96db3a61e0d02a584fa9fceb049cdad6fe8. Closing this bug now.
Thanks once again.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Wed, 04 Jan 2023 12:24:08 GMT)
Full text and
rfc822 format available.
This bug report was last modified 1 year and 110 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.