bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

(Note: this is tangentially related to bug#51993, but it happens 
regardless of whether 'server-stop-automatically' is enabled.)

In most cases when you do something to exit an Emacs client, it prompts 
you to save buffers associated with that client. For example:

   $ emacs -Q -f server-start
   $ emacsclient -c foo.txt

   ;; type some random characters

   C-x #
   ;; or...
   C-x C-c

In both cases, Emacs will ask to save foo.txt, though the prompts will 
be slightly different. That's good, since whatever invoked "emacsclient 
-c foo.txt" is likely waiting for the user to have saved that file.

However, if you use 'C-x 5 0' instead, that terminates the Emacs client, 
but *doesn't* prompt to save foo.txt. I think it should prompt in this 
case too: all three of 'C-x #', 'C-x C-c', and 'C-x 5 0' have the effect 
(in this simple case) of deleting the Emacs client and returning to the 
calling process. (A user who wanted to bail out of an Emacs client 
without saving should use 'sever-edit-abort' instead.)

Attached is a WIP patch to do this. It's WIP because it will need to 
have some special handling for the 'server-stop-automatically' case so 
that it doesn't prompt twice in some cases. I also changed how 
'delete-frame' calls 'delete-frame-functions'. Hopefully the change I 
made is correct; I'm hesitant to change low-level code like that, but I 
think it's the right thing to do for this case at least. (Maybe that 
change should be called out in NEWS?)

Ultimately, it might make sense to merge this bug with bug#51993, but 
the discussion in that bug is already pretty long, and I think it would 
just confuse matters to add even more tangents to that discussion.

[-- Attachment #2: 0001-When-deleting-the-last-frame-of-an-Emacs-client-ask-.patch --]
[-- Type: text/plain, Size: 3646 bytes --]

From 870d99540483d39174860190872d7e61f2205fad Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 29 Oct 2022 20:40:49 -0700
Subject: [PATCH] When deleting the last frame of an Emacs client, ask to save

* src/frame.c (delete_frame): Call 'delete-frame-functions' with
'call2' instead of 'safe_call2' to allow hooks to quit, cancelling the
frame's deletion.

* lisp/server.el (server-save-some-buffers): New function...
(server-handle-delete-frame, server-save-buffers-kill-terminal):
... use it.
---
 lisp/server.el | 31 +++++++++++++++++++------------
 src/frame.c    |  2 +-
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..de4c637caa 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -479,6 +479,20 @@ server-unselect-display
       (when (buffer-live-p buffer)
 	(kill-buffer buffer)))))
 
+(defun server-save-some-buffers (proc &optional arg)
+  "Save modified file-visiting buffers from the emacsclient file list.
+PROC is the emacsclient process.  If ARG is non-nil, save all
+with no questions."
+  (let ((buffers (process-get proc 'buffers)))
+    (save-some-buffers
+     arg (if buffers
+             ;; Only files from emacsclient file list.
+             (lambda () (memq (current-buffer) buffers))
+           ;; If there's no emacsclient file list: don't override
+           ;; `save-some-buffers-default-predicate' (unless ARG is
+           ;; non-nil).
+           (and arg t)))))
+
 (defun server-handle-delete-frame (frame)
   "Delete the client connection when the emacsclient frame is deleted.
 \(To be used from `delete-frame-functions'.)"
@@ -492,7 +506,9 @@ server-handle-delete-frame
                             (eq proc (frame-parameter f 'client))))
                      (frame-list))))
       (server-log (format "server-handle-delete-frame, frame %s" frame) proc)
-      (server-delete-client proc 'noframe)))) ; Let delete-frame delete the frame later.
+      (server-save-some-buffers proc))
+      ;; Let delete-frame delete the frame later.
+      (server-delete-client proc 'noframe)))
 
 (defun server-handle-suspend-tty (terminal)
   "Notify the client process that its tty device is suspended."
@@ -1752,17 +1768,8 @@ server-save-buffers-kill-terminal
 	       ;; If we're the last frame standing, kill Emacs.
 	       (save-buffers-kill-emacs arg)))
 	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
+             (server-save-some-buffers proc arg)
+	     (server-delete-client proc))
 	    (t (error "Invalid client frame"))))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
diff --git a/src/frame.c b/src/frame.c
index f076a5ba54..8a85d5a400 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2037,7 +2037,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
       x_clipboard_manager_save_frame (frame);
 #endif
 
-      safe_call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
+      call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
     }
 
   /* delete_frame_functions may have deleted any frame, including this
-- 
2.25.1

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Sun, 30 Oct 2022 15:29:30 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
>    $ emacs -Q -f server-start
>    $ emacsclient -c foo.txt
> 
>    ;; type some random characters
> 
>    C-x #
>    ;; or...
>    C-x C-c
> 
> In both cases, Emacs will ask to save foo.txt, though the prompts will 
> be slightly different. That's good, since whatever invoked "emacsclient 
> -c foo.txt" is likely waiting for the user to have saved that file.
> 
> However, if you use 'C-x 5 0' instead, that terminates the Emacs client, 
> but *doesn't* prompt to save foo.txt. I think it should prompt in this 
> case too: all three of 'C-x #', 'C-x C-c', and 'C-x 5 0' have the effect 
> (in this simple case) of deleting the Emacs client and returning to the 
> calling process. (A user who wanted to bail out of an Emacs client 
> without saving should use 'sever-edit-abort' instead.)

I'm uneasy with this incompatible behavior change.  I can think of
some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
the user intends to keep editing the file, and no application is
waiting for the client to finish.  Why break such flows?

Deleting a frame does no harm as long as Emacs is still up and running
after that, so IMO forcing the user to answer such a prompt could be
an annoyance.

Thanks.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 5:44 AM, Eli Zaretskii wrote:
> I'm uneasy with this incompatible behavior change.  I can think of
> some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
> the user intends to keep editing the file, and no application is
> waiting for the client to finish.  Why break such flows?

Maybe we should add an option like 
'server-save-before-deleting-last-frame', in case users prefer the old 
behavior? I had originally planned to add an option like this for the 
same reason you mention, but I didn't want to be too aggressive with 
adding extra options if they aren't really needed. Since you mentioned 
it too though, I think this should at least have an option.

Alternately, we could add a keybinding for 'server-edit-abort'. Then 
users could easily use that command to support this flow. That has the 
benefit that it's a command which is explicitly for saying, "I'm done 
with this file and don't want to save it," though it does break users' 
muscle memory.

Still another possibility would be to prompt only when closing a frame 
by clicking the "X" on the frame's title bar. When using a 
non-client/server Emacs, doing this prompts the user to save when 
closing the last frame. ('C-x 5 0' in this case would just error out, 
since it refuses to delete the last frame.) That's because clicking the 
X calls 'handle-delete-frame' which calls 'save-buffers-kill-emacs' in 
that case. Then clicking the "X" would behave similarly when deleting 
the last frame of a client and deleting the last frame overall.

I think I prefer one of the first two though.

> Deleting a frame does no harm as long as Emacs is still up and running
> after that, so IMO forcing the user to answer such a prompt could be
> an annoyance.

I think it does about the same amount of harm as pressing 'C-x C-c' in 
an emacs client. Since my patch only affects the case when there's a 
single frame for the client, both 'C-x C-c' and 'C-x 5 0' would do the 
same thing (aside from prompting): they close the current frame and 
terminate the client connection.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 10:36:57 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 10/31/2022 5:44 AM, Eli Zaretskii wrote:
> > I'm uneasy with this incompatible behavior change.  I can think of
> > some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
> > the user intends to keep editing the file, and no application is
> > waiting for the client to finish.  Why break such flows?
> 
> Maybe we should add an option like 
> 'server-save-before-deleting-last-frame', in case users prefer the old 
> behavior?

I tend to think we shouldn't change the behavior here, not at all.
Not even as an option.  I see no problem here that we must solve.

> Alternately, we could add a keybinding for 'server-edit-abort'.

Users who need this can easily make a binding for them.  There's no
need to have such a binding by default.

> Still another possibility would be to prompt only when closing a frame 
> by clicking the "X" on the frame's title bar. When using a 
> non-client/server Emacs, doing this prompts the user to save when 
> closing the last frame. ('C-x 5 0' in this case would just error out, 
> since it refuses to delete the last frame.) That's because clicking the 
> X calls 'handle-delete-frame' which calls 'save-buffers-kill-emacs' in 
> that case. Then clicking the "X" would behave similarly when deleting 
> the last frame of a client and deleting the last frame overall.

But "last frame of a client" ≠ "the last frame".  So I see no reason
for a similar behavior.

> > Deleting a frame does no harm as long as Emacs is still up and running
> > after that, so IMO forcing the user to answer such a prompt could be
> > an annoyance.
> 
> I think it does about the same amount of harm as pressing 'C-x C-c' in 
> an emacs client.

How can you say that?  "C-x C-c" kills the entire terminal, whereas
"C-x 5 0" kills just one frame!

> Since my patch only affects the case when there's a single frame for
> the client, both 'C-x C-c' and 'C-x 5 0' would do the same thing
> (aside from prompting): they close the current frame and terminate
> the client connection.

I disagree, sorry.  We are talking about maybe a minor inconvenience
in a pretty special use case.  It is unjustified to change long-time
behavior for such weak reasons.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 11:25 AM, Eli Zaretskii wrote:
>> I think it does about the same amount of harm as pressing 'C-x C-c' in
>> an emacs client.
> 
> How can you say that?  "C-x C-c" kills the entire terminal, whereas
> "C-x 5 0" kills just one frame!

When there's only one frame left for the client, 'C-x 5 0' *also* kills 
the entire terminal. That's the only case I want to add a prompt to. I 
agree that if you're only killing a frame, there's no sense in 
prompting. But when killing a frame gets upgraded into killing the 
entire terminal, it then has the same end result as having pressed 'C-x 
C-c'. Shouldn't Emacs then ask the user about what to do before killing 
the terminal?

Of course, 'C-x C-c' also has the effect of killing any other frames for 
that client, but that that doesn't apply there's only one frame. Still, 
'C-x C-c' still prompts in that case too. My understanding is that it 
does so because when an application is waiting for the emacsclient to 
finish, you can't go back from killing the terminal/client; you *can* 
(usually) go back if you're only killing a frame.

See my other message about this too: we could make sure Emacs only 
prompts the user when deleting a frame if doing so would kill a terminal 
that's actually waiting for some files to be saved. If the client was 
invoked with "--no-wait", we could just silently delete the frame: 
there's no application waiting for a file.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 12:38:04 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 58909@debbugs.gnu.org
> 
> On 10/31/2022 11:25 AM, Eli Zaretskii wrote:
> >> I think it does about the same amount of harm as pressing 'C-x C-c' in
> >> an emacs client.
> > 
> > How can you say that?  "C-x C-c" kills the entire terminal, whereas
> > "C-x 5 0" kills just one frame!
> 
> When there's only one frame left for the client, 'C-x 5 0' *also* kills 
> the entire terminal.

But "C-x 5 0" doesn't have the same significance as "C-x C-c".  They
are different in the non-client use, and they are different in the
client use.  That someone could perceive them as very similar because
some aspects of what they do are the same doesn't mean they are
similar enough to behave the same wrt the prompt to save.

> Of course, 'C-x C-c' also has the effect of killing any other frames for 
> that client, but that that doesn't apply there's only one frame.

It would (or at least could) be possible to reason that way.  But it
could be argued that forcing the user to distinguish between the last
"C-x 5 0" and any other is bad UI.  When I type "C-x 5 0" I don't want
to expect different behavior depending on how many frames the current
client has.  "C-x 5 0" means "delete this frame", no more, no less.

> Still, 
> 'C-x C-c' still prompts in that case too. My understanding is that it 
> does so because when an application is waiting for the emacsclient to 
> finish, you can't go back from killing the terminal/client; you *can* 
> (usually) go back if you're only killing a frame.

My interpretation of why "C-x C-c" prompts is that it does the same as
when you use it from a non-client frame.  We want the same UX in both
cases.  "C-x 5 0" should IMO likewise produce the same behavior in
both cases.

> See my other message about this too: we could make sure Emacs only 
> prompts the user when deleting a frame if doing so would kill a terminal 
> that's actually waiting for some files to be saved. If the client was 
> invoked with "--no-wait", we could just silently delete the frame: 
> there's no application waiting for a file.

This has the same problem, IMO: it changes long-time behavior of
client frames in incompatible ways.  I don't think it's right, since
the reasons for these changes are rather weak.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 12:52 PM, Eli Zaretskii wrote:
>> Date: Mon, 31 Oct 2022 12:38:04 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 58909@debbugs.gnu.org
>>
>> Still,
>> 'C-x C-c' still prompts in that case too. My understanding is that it
>> does so because when an application is waiting for the emacsclient to
>> finish, you can't go back from killing the terminal/client; you *can*
>> (usually) go back if you're only killing a frame.
> 
> My interpretation of why "C-x C-c" prompts is that it does the same as
> when you use it from a non-client frame.  We want the same UX in both
> cases.  "C-x 5 0" should IMO likewise produce the same behavior in
> both cases.

That's actually my goal too: I'd like to ensure that the UX for a client 
frame is as similar as possible as for a non-client frame. Since we 
disagree on what that similarity should be, I guess that means we have 
different mental models for some part of this. If it helps, I'll try to 
explain how I think of it.

 From a client frame, 'C-x C-c' prompts to save all the file-buffers 
associated with that client, and then kills the client, which kills the 
frames for that client. That's similar to 'C-x C-c' in a non-client 
setting: it kills *all* frames and prompts to save *all* file-buffers. 
So I see the client as a "sub" Emacs: it owns some frames and 
(partially) owns some buffers. Commands on the client then work on that 
subset, and only that subset. Since frames can only be owned by a single 
client, in my mind frame commands also work within that client's subset.

For frame-deletion, non-client Emacs lets you kill a single frame via 
'C-x 5 0' or clicking the X in the frame's title bar. However, if it's 
the last frame, then:

   C-x 5 0    -> Error: "Attempt to delete the sole visible or iconified 
frame"
   Clicking X -> Call 'save-buffers-kill-emacs' (see 'handle-delete-frame')

To me, that would mean that the matching UX for a client with a single 
frame (but possibly other frames for Emacs overall) is:

   C-x 5 0    -> Error
   Clicking X -> Call 'save-buffers-kill-terminal'

Signaling an error for 'C-x 5 0' seems excessively strict to me, so 
calling 'save-buffers-kill-terminal' in both cases seemed like a good 
compromise to me. Instead of getting an error, the user gets a prompt, 
but can still proceed with killing that frame if they're sure.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 13:28:01 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > My interpretation of why "C-x C-c" prompts is that it does the same as
> > when you use it from a non-client frame.  We want the same UX in both
> > cases.  "C-x 5 0" should IMO likewise produce the same behavior in
> > both cases.
> 
> That's actually my goal too: I'd like to ensure that the UX for a client 
> frame is as similar as possible as for a non-client frame. Since we 
> disagree on what that similarity should be, I guess that means we have 
> different mental models for some part of this. If it helps, I'll try to 
> explain how I think of it.

Thanks.  Our notions of "similarity" are different.  For me, it's the
immediate effect of the command; for you, it's what happens in Emacs
under the hood, in particular how many other "similar" frames remain.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1757 bytes --]

On 10/31/2022 5:44 AM, Eli Zaretskii wrote:
>> Date: Sun, 30 Oct 2022 15:29:30 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> However, if you use 'C-x 5 0' instead, that terminates the Emacs client,
>> but *doesn't* prompt to save foo.txt. I think it should prompt in this
>> case too: all three of 'C-x #', 'C-x C-c', and 'C-x 5 0' have the effect
>> (in this simple case) of deleting the Emacs client and returning to the
>> calling process. (A user who wanted to bail out of an Emacs client
>> without saving should use 'sever-edit-abort' instead.)
> 
> I'm uneasy with this incompatible behavior change.  I can think of
> some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
> the user intends to keep editing the file, and no application is
> waiting for the client to finish.  Why break such flows?

After thinking about this some more, I realized that I didn't properly 
address this part of your message. If no application is waiting for the 
client to finish, then the user hopefully used "--no-wait" when starting 
emacsclient. We could avoid prompting in that case.[1]

If an application *is* waiting for the client to finish, then 
"--no-wait" would be unset. In that case, prompting the user has some 
value. See the attached patch. (I changed the code to only prompt when 
deleting the last frame of a *non-nowait* client.)

[1] Currently, 'C-x C-c' from a nowait frame prompts via 
'save-some-buffers' (so long as there's another frame; see 
'server-save-buffers-kill-terminal'). Maybe we could avoid prompting 
then too? After prompting, it just calls 'delete-frame', so it's very 
similar to 'C-x 5 0' in this scenario... Still, as you say, it's 
probably good to be careful about making incompatible changes here.

[-- Attachment #2: 0001-When-deleting-the-last-frame-of-an-Emacs-client-ask-.patch --]
[-- Type: text/plain, Size: 3898 bytes --]

From 68b9f41685256870f3409ea468971ee0b64cf136 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 29 Oct 2022 20:40:49 -0700
Subject: [PATCH] When deleting the last frame of an Emacs client, ask to save

* src/frame.c (delete_frame): Call 'delete-frame-functions' with
'call2' instead of 'safe_call2' to allow hooks to quit, cancelling the
frame's deletion.

* lisp/server.el (server-save-some-buffers): New function...
(server-handle-delete-frame, server-save-buffers-kill-terminal):
... use it.
---
 lisp/server.el | 33 ++++++++++++++++++++-------------
 src/frame.c    |  2 +-
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..f04f993426 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -479,12 +479,26 @@ server-unselect-display
       (when (buffer-live-p buffer)
 	(kill-buffer buffer)))))
 
+(defun server-save-some-buffers (proc &optional arg)
+  "Save modified file-visiting buffers from the emacsclient file list.
+PROC is the emacsclient process.  If ARG is non-nil, save all
+with no questions."
+  (let ((buffers (process-get proc 'buffers)))
+    (save-some-buffers
+     arg (if buffers
+             ;; Only files from emacsclient file list.
+             (lambda () (memq (current-buffer) buffers))
+           ;; If there's no emacsclient file list: don't override
+           ;; `save-some-buffers-default-predicate' (unless ARG is
+           ;; non-nil).
+           (and arg t)))))
+
 (defun server-handle-delete-frame (frame)
   "Delete the client connection when the emacsclient frame is deleted.
 \(To be used from `delete-frame-functions'.)"
   (let ((proc (frame-parameter frame 'client)))
     (when (and (frame-live-p frame)
-	       proc
+	       (processp proc)
 	       ;; See if this is the last frame for this client.
                (not (seq-some
                      (lambda (f)
@@ -492,7 +506,9 @@ server-handle-delete-frame
                             (eq proc (frame-parameter f 'client))))
                      (frame-list))))
       (server-log (format "server-handle-delete-frame, frame %s" frame) proc)
-      (server-delete-client proc 'noframe)))) ; Let delete-frame delete the frame later.
+      (server-save-some-buffers proc))
+      ;; Let delete-frame delete the frame later.
+      (server-delete-client proc 'noframe)))
 
 (defun server-handle-suspend-tty (terminal)
   "Notify the client process that its tty device is suspended."
@@ -1752,17 +1768,8 @@ server-save-buffers-kill-terminal
 	       ;; If we're the last frame standing, kill Emacs.
 	       (save-buffers-kill-emacs arg)))
 	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
+             (server-save-some-buffers proc arg)
+	     (server-delete-client proc))
 	    (t (error "Invalid client frame"))))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
diff --git a/src/frame.c b/src/frame.c
index f076a5ba54..8a85d5a400 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2037,7 +2037,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
       x_clipboard_manager_save_frame (frame);
 #endif
 
-      safe_call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
+      call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
     }
 
   /* delete_frame_functions may have deleted any frame, including this
-- 
2.25.1

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 12:28:23 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > I'm uneasy with this incompatible behavior change.  I can think of
> > some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
> > the user intends to keep editing the file, and no application is
> > waiting for the client to finish.  Why break such flows?
> 
> After thinking about this some more, I realized that I didn't properly 
> address this part of your message. If no application is waiting for the 
> client to finish, then the user hopefully used "--no-wait" when starting 
> emacsclient.

No, I meant that the user invoked emacsclient from the shell prompt,
for example.  As opposed to some application invoking emacsclient via
$EDITOR or similar.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 12:43 PM, Eli Zaretskii wrote:
>> Date: Mon, 31 Oct 2022 12:28:23 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>>> I'm uneasy with this incompatible behavior change.  I can think of
>>> some legitimate use cases where "C-x 5 0" should not prompt, e.g., if
>>> the user intends to keep editing the file, and no application is
>>> waiting for the client to finish.  Why break such flows?
>>
>> After thinking about this some more, I realized that I didn't properly
>> address this part of your message. If no application is waiting for the
>> client to finish, then the user hopefully used "--no-wait" when starting
>> emacsclient.
> 
> No, I meant that the user invoked emacsclient from the shell prompt,
> for example.  As opposed to some application invoking emacsclient via
> $EDITOR or similar.

If a user simply typed "emacsclient file.txt", that would visit file.txt 
in an existing frame (if possible), so that frame wouldn't be associated 
with the emacsclient process the user just started. Pressing 'C-x 5 0' 
wouldn't need to prompt then: it's not deleting the last frame 
associated with that client, so the code in 'server-handle-delete-frame' 
doesn't apply. (Note: It's possible that the frame in question is the 
last frame of some *other* client though.)

A user might instead type "emacsclient -c file.txt" (or use "-t", etc) 
to create an all-new frame. In that case, my patch would prompt. But if 
the user is already typing "emacsclient -c file.txt", then "emacsclient 
-nc foo.txt" is just one more character, and it would make it explicit 
that the client isn't waiting around for file.txt. Then my patch would 
*not* prompt.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 13:01:55 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> A user might instead type "emacsclient -c file.txt" (or use "-t", etc) 
> to create an all-new frame. In that case, my patch would prompt. But if 
> the user is already typing "emacsclient -c file.txt", then "emacsclient 
> -nc foo.txt" is just one more character, and it would make it explicit 
> that the client isn't waiting around for file.txt. Then my patch would 
> *not* prompt.

Yes, this new behavior can be worked around, in more than one way.
But people get annoyed by changes which invalidate their muscle memory
of many years.  So we must in each case like this to have a very good
reason for such changes, otherwise people would rightfully conclude
that our decisions are more-or-less are arbitrary and basically prefer
someone's workflows over those of others.  I'm sorry, but past
behavior gets more weight in such situations, just because it was here
first.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 1:21 PM, Eli Zaretskii wrote:
>> Date: Mon, 31 Oct 2022 13:01:55 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> A user might instead type "emacsclient -c file.txt" (or use "-t", etc)
>> to create an all-new frame. In that case, my patch would prompt. But if
>> the user is already typing "emacsclient -c file.txt", then "emacsclient
>> -nc foo.txt" is just one more character, and it would make it explicit
>> that the client isn't waiting around for file.txt. Then my patch would
>> *not* prompt.
> 
> Yes, this new behavior can be worked around, in more than one way.
> But people get annoyed by changes which invalidate their muscle memory
> of many years.  So we must in each case like this to have a very good
> reason for such changes, otherwise people would rightfully conclude
> that our decisions are more-or-less are arbitrary and basically prefer
> someone's workflows over those of others.

I can definitely understand that position. It's an aspect of Emacs that 
I know I appreciate very much. Emacs has been around for a long time, 
and hopefully will be around for a lot longer. I'm not sure I have a 
great sense of how to balance Emacs' history with ensuring that the next 
decade (and more!) of Emacs is on the best footing possible.

In any case, unless my other recent messages have convinced you 
otherwise, what do you think of the following:

1) For now, just make the change in my patch to 'delete-frame' in 
src/frame.c to allow hooks in 'delete-frame-functions' to quit out of 
frame deletion. That way, users who want the rest of the behavior in my 
patch can just replace 'server-handle-delete-frame' with their own Elisp 
function. This change isn't entirely without risk, since it could cause 
some hooks to go from silently signaling an error to making it 
impossible to delete frames, but I'm not sure that will come up in 
practice (and if it does, the hooks can be fixed easily enough).

2) After the Emacs 29 branch is cut, maybe (emphasis on maybe) discuss 
the changes to prompting on emacs-devel, and possibly even commit it to 
the master branch with the caveat that if it causes problems for anyone, 
we back it out. Obviously not everyone follows emacs-devel, but this 
would give people a chance to provide feedback, positive or negative.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Mon, 31 Oct 2022 14:06:16 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> 1) For now, just make the change in my patch to 'delete-frame' in 
> src/frame.c to allow hooks in 'delete-frame-functions' to quit out of 
> frame deletion. That way, users who want the rest of the behavior in my 
> patch can just replace 'server-handle-delete-frame' with their own Elisp 
> function. This change isn't entirely without risk, since it could cause 
> some hooks to go from silently signaling an error to making it 
> impossible to delete frames, but I'm not sure that will come up in 
> practice (and if it does, the hooks can be fixed easily enough).

I don't see how this would serve well the use case you want to enable.
We are talking about prompting the user to save unsaved buffers, yes?
So adding a hook in server-delete-client sounds like a much better
solution to me, as it doesn't affect the (much more general)
delete-frame in any way.

> 2) After the Emacs 29 branch is cut, maybe (emphasis on maybe) discuss 
> the changes to prompting on emacs-devel, and possibly even commit it to 
> the master branch with the caveat that if it causes problems for anyone, 
> we back it out. Obviously not everyone follows emacs-devel, but this 
> would give people a chance to provide feedback, positive or negative.

You can start the discussion now, from my POV.  But if having a hook
in server-delete-client is good enough, I don't see why we would need
to discuss an actual behavior change.

(And the proviso of backing out changes doesn't work in this project,
IME: people get defensive about their changes, and perceive reverting
them as personal insult.  So we do that only in very extreme cases.)

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 10/31/2022 11:39 PM, Eli Zaretskii wrote:
>> Date: Mon, 31 Oct 2022 14:06:16 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> 1) For now, just make the change in my patch to 'delete-frame' in
>> src/frame.c to allow hooks in 'delete-frame-functions' to quit out of
>> frame deletion. That way, users who want the rest of the behavior in my
>> patch can just replace 'server-handle-delete-frame' with their own Elisp
>> function. This change isn't entirely without risk, since it could cause
>> some hooks to go from silently signaling an error to making it
>> impossible to delete frames, but I'm not sure that will come up in
>> practice (and if it does, the hooks can be fixed easily enough).
> 
> I don't see how this would serve well the use case you want to enable.
> We are talking about prompting the user to save unsaved buffers, yes?
> So adding a hook in server-delete-client sounds like a much better
> solution to me, as it doesn't affect the (much more general)
> delete-frame in any way.

I think a hook on 'server-delete-client' could work well. It'd make it 
easier to write hooks that run at the right time compared to using other 
existing hooks. In fact, I had a similar idea for bug#51993[1]. In that 
case I ended up adding 2 hooks to 'server-delete-client', but that was 
just to work around a strange bug I saw in testing; I could probably fix 
that in a better way with some more effort so that we only need one new 
hook.

However, I'm not sure how to do this in a complete way without tweaking 
'delete-frame-functions'. Deleting a frame can cause Emacs to delete the 
client if that was the last frame for the client; that's 
long-established behavior, so we shouldn't change it. But that poses a 
problem. If 1) I delete a frame, 2) it calls 'server-delete-client', and 
3) some 'server-delete-client-hook' prompts me, then I might try to quit 
out via C-g. Without my change to how 'delete-frame-functions' are run, 
then C-g would only quit out of 'server-delete-client', but would still 
delete the frame. At least for some emacsclient use cases, that could be 
a problem.

For example, suppose I have a system called "remotehost" with an "emacs 
--daemon" instance and EDITOR="emacsclient -c":

   me@localhost $ ssh -X remotehost
   ...
   me@remotehost $ git commit

   ;; emacsclient starts and creates a new frame on my local display.
   ;; Start editing the git commit message.
   ;; Get distracted, do some other stuff...

   ;; ... finish up the other stuff, click "X" on the emacsclient frame.
   Save file /home/me/src/project/.git/COMMIT_EDITMSG?
   ;; Realize, "Oh yeah! I forgot to finish this commit message."
   C-g

Without the 'delete-frame-functions' change, I'd now be left with no 
Emacs frames on my localhost, but the emacsclient is still running. That 
would be inconvenient, since I'd have to do more work to fix the 
situation. The best way I can think of would be to start another 
emacsclient locally, do the edits to COMMIT_EDITMSG, and then 'C-x #' to 
finish editing. It'd be a lot nicer if 'C-g' stopped the frame from 
getting deleted. (Incidentally, that's how it would work in a regular, 
non-client/server Emacs. Clicking "X" on the last frame actually calls 
'save-buffer-kill-emacs' instead of 'delete-frame', and you can 'C-g' 
out of that to keep the frame open.)

> You can start the discussion now, from my POV.  But if having a hook
> in server-delete-client is good enough, I don't see why we would need
> to discuss an actual behavior change.

Yeah, let's finish up the discussion here, and if I have any open 
questions that could use a wider audience, I'll post to emacs-devel 
afterwards.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-10/msg00908.html

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On 11/1/2022 9:11 AM, Jim Porter wrote:
> However, I'm not sure how to do this in a complete way without tweaking 
> 'delete-frame-functions'. ...
Attached is a (hopefully) safer version of my change to 'delete-frame'. 
In this patch, a hook can only cancel frame deletion if FORCE is 
non-nil. This way, if there were ever some bug with a hook, you (or some 
code) can still force-delete the frame.

This would probably still need a NEWS entry, but I'm personally less 
worried about this change compared to my previous revision. What do you 
think?

[-- Attachment #2: 0001-Let-delete-frame-functions-quit-out-of-frame-deletio.patch --]
[-- Type: text/plain, Size: 1112 bytes --]

From ea7a6ff541ccfae474f32ffa1dedd3667ede63b7 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 1 Nov 2022 15:28:18 -0700
Subject: [PATCH] Let 'delete-frame-functions' quit out of frame deletion if
 FORCE is nil

* src/frame.c (delete_frame): When FORCE is nil, call
'delete-frame-functions' with 'call2' instead of 'safe_call2' to allow
hooks to quit out.
---
 src/frame.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/frame.c b/src/frame.c
index f076a5ba54..9722efa505 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2037,7 +2037,11 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
       x_clipboard_manager_save_frame (frame);
 #endif
 
-      safe_call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
+      /* If FORCE is nil, allow hooks to quit out of frame deletion.  */
+      if (NILP (force))
+	call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
+      else
+	safe_call2 (Qrun_hook_with_args, Qdelete_frame_functions, frame);
     }
 
   /* delete_frame_functions may have deleted any frame, including this
-- 
2.25.1

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Tue, 1 Nov 2022 15:39:29 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 58909@debbugs.gnu.org
> 
> Attached is a (hopefully) safer version of my change to 'delete-frame'. 
> In this patch, a hook can only cancel frame deletion if FORCE is 
> non-nil. This way, if there were ever some bug with a hook, you (or some 
> code) can still force-delete the frame.

Sorry, but I still don't like this.  We currently call this hook via
safe_call because we don't want to let the hook prevent the deletion
of the frame.

I prefer to solve this in server.el, not in lower-level primitives.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On 11/2/2022 5:16 AM, Eli Zaretskii wrote:
>> Date: Tue, 1 Nov 2022 15:39:29 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 58909@debbugs.gnu.org
>>
>> Attached is a (hopefully) safer version of my change to 'delete-frame'.
>> In this patch, a hook can only cancel frame deletion if FORCE is
>> non-nil. This way, if there were ever some bug with a hook, you (or some
>> code) can still force-delete the frame.
> 
> Sorry, but I still don't like this.  We currently call this hook via
> safe_call because we don't want to let the hook prevent the deletion
> of the frame.
> 
> I prefer to solve this in server.el, not in lower-level primitives.

Hm, ok. That means I'd need to make sure 'C-x 5 0' calls something other 
than 'delete-frame', so that I could add a prompt to it that can prevent 
deletion of the frame.

Two questions then:

1. If 'delete-frame' is a lower-level primitive, should 'C-x 5 0' call 
it directly, or should there be a higher-level function for it to call? 
If we had some higher-level function, then user-level commands could 
call that, but low-level code could still use 'delete-frame'. This 
user-level command would then be able to run some hook that can prevent 
deletion of the frame.

2. Since server.el has a minor mode (unsurprisingly named 
'server-mode'), maybe one option is to do command remapping. If we remap 
'delete-frame' to some new 'server-delete-frame' when 'server-mode' is 
active, then this should work. See attached for a quick sketch of how 
this would look.

What do you think about one of these?

[-- Attachment #2: 0001-WIP-Try-using-a-keymap-for-server-mode.patch --]
[-- Type: text/plain, Size: 1408 bytes --]

From 2ce994de124b7609089923090201cbb77fa035bc Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 2 Nov 2022 09:22:43 -0700
Subject: [PATCH] [WIP] Try using a keymap for server-mode

---
 lisp/server.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..c8f90720f7 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -796,6 +796,18 @@ server-running-p
 	t)
     (file-error nil)))
 
+(defvar-keymap server-keymap
+  "<remap> <delete-frame>" #'server-delete-frame
+  ;; FIXME: Add 'C-x #' to this keymap instead of doing it globally?
+  )
+
+(defun server-delete-frame ()
+  "FIXME: This function is just a quick test implementation to
+override `delete-frame'."
+  (interactive)
+  (save-some-buffers)
+  (delete-frame))
+
 ;;;###autoload
 (define-minor-mode server-mode
   "Toggle Server mode.
@@ -805,6 +817,11 @@ server-mode
 `server-start' for details."
   :global t
   :version "22.1"
+  ;; FIXME: To start a server, you can call `server-mode' or
+  ;; `server-start'.  The latter doesn't activate the minor mode.  To
+  ;; use this keymap, we'd need to make sure we always activate the
+  ;; minor mode.
+  :keymap server-keymap
   ;; Fixme: Should this check for an existing server socket and do
   ;; nothing if there is one (for multiple Emacs sessions)?
   (server-start (not server-mode)))
-- 
2.25.1

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Wed, 2 Nov 2022 09:36:52 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > Sorry, but I still don't like this.  We currently call this hook via
> > safe_call because we don't want to let the hook prevent the deletion
> > of the frame.
> > 
> > I prefer to solve this in server.el, not in lower-level primitives.
> 
> Hm, ok. That means I'd need to make sure 'C-x 5 0' calls something other 
> than 'delete-frame', so that I could add a prompt to it that can prevent 
> deletion of the frame.

Why?  delete-frame eventually calls a function from server.el, doesn't
it?  We've been through that already.  Why cannot you do this inside
that server.el function?

If that's because you want to support the C-g case, then don't: that
is a separate problem.  You wanted to give the user the opportunity to
save the buffers, and that doesn't require any support for C-g.
Besides, if the user types C-g when presented with the save-buffers
prompt, how do you know that the user intends to abort deletion of the
frame, and not break out of the saving operation?

And finally, even if the user does want to prevent the deletion of the
frame, and fails in doing that, how is that a catastrophe?  Emacs is
still running and the unsaved buffers are still in the session.

I really don't see why we need to jump through any hoops for such a
simple situation.

> 1. If 'delete-frame' is a lower-level primitive, should 'C-x 5 0' call 
> it directly, or should there be a higher-level function for it to call? 
> If we had some higher-level function, then user-level commands could 
> call that, but low-level code could still use 'delete-frame'. This 
> user-level command would then be able to run some hook that can prevent 
> deletion of the frame.
> 
> 2. Since server.el has a minor mode (unsurprisingly named 
> 'server-mode'), maybe one option is to do command remapping. If we remap 
> 'delete-frame' to some new 'server-delete-frame' when 'server-mode' is 
> active, then this should work. See attached for a quick sketch of how 
> this would look.
> 
> What do you think about one of these?

Waaaay too complicated for such a simple problem.  Please, let's just
prompt the user from server-delete-client (and do that under some
opt-in option), and be done with that.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

First, I just wanted to say thanks for your patience with this. I think 
this discussion has gone on a lot longer than either of us intended. I 
want to emphasize that while I'm not flexible in how my Emacs 
configuration works, I *am* flexible in what actually gets committed to 
Emacs proper. If you think my changes belong in Emacs, then I'll do the 
necessary work to get them in a merge-able state; if not, I can just 
apply them to my local configuration. Thanks again.

On 11/2/2022 10:11 AM, Eli Zaretskii wrote:
>> Date: Wed, 2 Nov 2022 09:36:52 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Hm, ok. That means I'd need to make sure 'C-x 5 0' calls something other
>> than 'delete-frame', so that I could add a prompt to it that can prevent
>> deletion of the frame.
> 
> Why?  delete-frame eventually calls a function from server.el, doesn't
> it?  We've been through that already.  Why cannot you do this inside
> that server.el function?
> 
> If that's because you want to support the C-g case, then don't: that
> is a separate problem.

Yeah, this subthread about 'delete-frame' was just to support the C-g 
case. Aside from that, I think doing this inside 'server-delete-client' 
would be fine. However, at least for this bug, adding a hook that runs 
in 'server-delete-client' doesn't add anything that 
'delete-frame-functions' doesn't already allow. ('server-delete-client' 
is called from a hook in 'delete-frame-functions' in this case anyway.)

> Waaaay too complicated for such a simple problem.  Please, let's just
> prompt the user from server-delete-client (and do that under some
> opt-in option), and be done with that.

If you mean adding a defcustom, I thought we'd agreed not to do that 
(see the beginning of your message here[1]). I'm fine with that; if I 
have to write a hook to fix the issue I saw in the original message, I 
don't mind. (Of course, I'm happy to upstream my code if there's 
interest, but if not, that's ok too.) Instead, my goal was just to 
provide the bare minimum of changes to Emacs to enable a user to modify 
this to their liking without relying too heavily on 'advice-add'. :)

Would it make sense (possibly in a separate bug) to at least make sure 
that the 'server-mode' minor mode is always active when the server is 
running? The server.el code sometimes uses 'server-mode' and sometimes 
uses 'server-start', with the end result being that the minor mode may 
or may not be active depending on the way the server was started. That 
seems like a bug all on its own. If just this part were fixed, it would 
go 95% of the way to make it easy for me to make Emacs work just the way 
I like, and without having to make any major changes to Emacs itself. 
This change could be as simple as renaming 'server-start' to 
'server--start', and then have a new 'server-start' that activates the 
minor mode.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-10/msg02642.html

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Wed, 2 Nov 2022 11:17:00 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > If that's because you want to support the C-g case, then don't: that
> > is a separate problem.
> 
> Yeah, this subthread about 'delete-frame' was just to support the C-g 
> case. Aside from that, I think doing this inside 'server-delete-client' 
> would be fine. However, at least for this bug, adding a hook that runs 
> in 'server-delete-client' doesn't add anything that 
> 'delete-frame-functions' doesn't already allow. ('server-delete-client' 
> is called from a hook in 'delete-frame-functions' in this case anyway.)

We cannot have behavior specific to server.el coded outside of
server.el.  So I don't think I understand what you are saying here.
My suggestion is not to add a hook, it is to add a prompt for the user
when the last client frame is deleted, and do it optionally.

> > Waaaay too complicated for such a simple problem.  Please, let's just
> > prompt the user from server-delete-client (and do that under some
> > opt-in option), and be done with that.
> 
> If you mean adding a defcustom, I thought we'd agreed not to do that 
> (see the beginning of your message here[1]).

Yes, but you keep pushing...

> Would it make sense (possibly in a separate bug) to at least make sure 
> that the 'server-mode' minor mode is always active when the server is 
> running? The server.el code sometimes uses 'server-mode' and sometimes 
> uses 'server-start', with the end result being that the minor mode may 
> or may not be active depending on the way the server was started. That 
> seems like a bug all on its own.

Maybe it's a bug, but then it's really old.

I'm not sure I understand the purpose of this minor mode, and we
ourselves start the server in the daemon mode by a direct call to
server-start.  Also, server-start is a command, and many users (myself
included) invoke it interactively.

Why is it important to have the mode turned on?  What can a mode do
that we cannot do without a mode?

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

On 11/2/2022 11:42 AM, Eli Zaretskii wrote:
>> Date: Wed, 2 Nov 2022 11:17:00 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>>> If that's because you want to support the C-g case, then don't: that
>>> is a separate problem.
>>
>> Yeah, this subthread about 'delete-frame' was just to support the C-g
>> case. Aside from that, I think doing this inside 'server-delete-client'
>> would be fine. However, at least for this bug, adding a hook that runs
>> in 'server-delete-client' doesn't add anything that
>> 'delete-frame-functions' doesn't already allow. ('server-delete-client'
>> is called from a hook in 'delete-frame-functions' in this case anyway.)
> 
> We cannot have behavior specific to server.el coded outside of
> server.el.  So I don't think I understand what you are saying here.
> My suggestion is not to add a hook, it is to add a prompt for the user
> when the last client frame is deleted, and do it optionally.

I just meant that the existing 'delete-frame-functions' hook lets me do 
what I want by writing a hook in my local configuration (except for the 
C-g issue). I agree that for any changes to Emacs, the changes should be 
in server.el as much as possible.

>> If you mean adding a defcustom, I thought we'd agreed not to do that
>> (see the beginning of your message here[1]).
> 
> Yes, but you keep pushing...

Sorry if I seem pushy. I really am open to different solutions here, and 
I just want to present some various options I see to make it 
possible/easy to adjust the behavior of server.el. My goal is really 
just to offer what improvements I can to Emacs; if they don't belong, 
that's ok. I can just use them locally.

In short, consider my messages/patches as just an offer to help.

> I'm not sure I understand the purpose of this minor mode, and we
> ourselves start the server in the daemon mode by a direct call to
> server-start.  Also, server-start is a command, and many users (myself
> included) invoke it interactively.
> 
> Why is it important to have the mode turned on?  What can a mode do
> that we cannot do without a mode?

If starting the server activates a minor mode, then Emacs can use a 
minor-mode keymap for server-mode. That would make it easy for users to 
add keybindings that only take effect when the server is running. In my 
case, I could locally remap 'delete-frame' to some new function when the 
server is running, and then I could make that function do exactly what I 
want. Other users might want to add other keybindings to this keymap 
(maybe one for 'server-edit-abort').

In fact, for my purposes, even just having daemon mode call 
'(server-mode 1)' instead of '(server-start)' would be a good improvement.

There are other ways to do this, but having a minor mode makes it 
easier. I attached a quick patch that shows the changes I'd make so that 
users could have a 'sever-mode-keymap' to add to; if you think any of 
these changes are too risky, I don't mind removing them. If you think 
*all* of them are too risky, that's ok too. I can just do something 
equivalent in my local config.

[-- Attachment #2: 0001-WIP-Try-using-a-keymap-for-server-mode.patch --]
[-- Type: text/plain, Size: 2116 bytes --]

From 0e638e4b1481e60b6d8211b15d55d3fde6291b31 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 2 Nov 2022 09:22:43 -0700
Subject: [PATCH] [WIP] Try using a keymap for server-mode

---
 lisp/frame.el   | 6 +++++-
 lisp/server.el  | 5 +++++
 lisp/startup.el | 2 +-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lisp/frame.el b/lisp/frame.el
index 400f8a44ee..f1888623e4 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -128,7 +128,11 @@ handle-delete-frame
                        (not (frame-parent frame-1))
                        (not (frame-parameter frame-1 'delete-before)))
               (throw 'other-frame t))))
-	(delete-frame frame t)
+        ;; NOTE: This change is optional.  If you think it adds too
+        ;; much complexity, let's remove it.
+        (funcall (or (command-remapping #'delete-frame)
+                     #'delete-frame)
+                 t)
       ;; Gildea@x.org says it is ok to ask questions before terminating.
       (save-buffers-kill-emacs))))
 
diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..aec6677f1d 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -796,6 +796,10 @@ server-running-p
 	t)
     (file-error nil)))
 
+;; NOTE: This change is optional, but it makes it easier for users to
+;; add keybindings for `server-mode'.
+(defvar-keymap server-mode-keymap)
+
 ;;;###autoload
 (define-minor-mode server-mode
   "Toggle Server mode.
@@ -805,6 +809,7 @@ server-mode
 `server-start' for details."
   :global t
   :version "22.1"
+  :keymap server-mode-keymap
   ;; Fixme: Should this check for an existing server socket and do
   ;; nothing if there is one (for multiple Emacs sessions)?
   (server-start (not server-mode)))
diff --git a/lisp/startup.el b/lisp/startup.el
index 70267fc857..a494e42d25 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1604,7 +1604,7 @@ command-line
   (let ((dn (daemonp)))
     (when dn
       (when (stringp dn) (setq server-name dn))
-      (server-start)
+      (server-mode 1)
       (if server-process
 	  (daemon-initialized)
 	(if (stringp dn)
-- 
2.25.1

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Wed, 2 Nov 2022 12:16:30 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >> If you mean adding a defcustom, I thought we'd agreed not to do that
> >> (see the beginning of your message here[1]).
> > 
> > Yes, but you keep pushing...
> 
> Sorry if I seem pushy. I really am open to different solutions here, and 
> I just want to present some various options I see to make it 
> possible/easy to adjust the behavior of server.el. My goal is really 
> just to offer what improvements I can to Emacs; if they don't belong, 
> that's ok. I can just use them locally.
> 
> In short, consider my messages/patches as just an offer to help.

Isn't the last suggestion, of having an optional prompt in
server-delete-client, good enough?

> > Why is it important to have the mode turned on?  What can a mode do
> > that we cannot do without a mode?
> 
> If starting the server activates a minor mode, then Emacs can use a 
> minor-mode keymap for server-mode. That would make it easy for users to 
> add keybindings that only take effect when the server is running. In my 
> case, I could locally remap 'delete-frame' to some new function when the 
> server is running, and then I could make that function do exactly what I 
> want. Other users might want to add other keybindings to this keymap 
> (maybe one for 'server-edit-abort').
> 
> In fact, for my purposes, even just having daemon mode call 
> '(server-mode 1)' instead of '(server-start)' would be a good improvement.

I'm fine with doing that, but we must also make sure this mode is
turned on when users invoke server-start interactively.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 11/2/2022 12:23 PM, Eli Zaretskii wrote:
> Isn't the last suggestion, of having an optional prompt in
> server-delete-client, good enough?

'server-delete-client' is called in a bunch of spots, so at minimum, I'd 
want to be extra-careful that prompting doesn't break one of those 
cases. It would also be nice if I could fix the C-g issue, at least in 
my local configuration.

Still, I'll take a look at doing this if you think it would be good to 
add to Emacs. (If you think it's not needed, I really don't mind leaving 
the code as-is.)

>>> Why is it important to have the mode turned on?  What can a mode do
>>> that we cannot do without a mode?
>>
>> If starting the server activates a minor mode, then Emacs can use a
>> minor-mode keymap for server-mode. [snip]
> 
> I'm fine with doing that, but we must also make sure this mode is
> turned on when users invoke server-start interactively.

I think that makes sense, and it should be fairly straightforward. I'll 
work on a patch for this.

Thanks again for the extensive feedback.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Wed, 2 Nov 2022 12:57:57 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 11/2/2022 12:23 PM, Eli Zaretskii wrote:
> > Isn't the last suggestion, of having an optional prompt in
> > server-delete-client, good enough?
> 
> 'server-delete-client' is called in a bunch of spots, so at minimum, I'd 
> want to be extra-careful that prompting doesn't break one of those 
> cases.

Sorry, I meant server-handle-delete-frame.

> It would also be nice if I could fix the C-g issue, at least in my
> local configuration.

You could rebind "C-x 5 0" to a different command, and do whatever you
want there.

> Still, I'll take a look at doing this if you think it would be good to 
> add to Emacs. (If you think it's not needed, I really don't mind leaving 
> the code as-is.)

I thought my opinions on this were clear from the very beginning...

> >> If starting the server activates a minor mode, then Emacs can use a
> >> minor-mode keymap for server-mode. [snip]
> > 
> > I'm fine with doing that, but we must also make sure this mode is
> > turned on when users invoke server-start interactively.
> 
> I think that makes sense, and it should be fairly straightforward. I'll 
> work on a patch for this.

Thanks.

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On 11/2/2022 1:09 PM, Eli Zaretskii wrote:
>> 'server-delete-client' is called in a bunch of spots, so at minimum, I'd
>> want to be extra-careful that prompting doesn't break one of those
>> cases.
> 
> Sorry, I meant server-handle-delete-frame.

Oh, ok. That would be easy then. But since your preference was to just 
leave things as they are, let's not make this change. I can adjust 
things locally without it.

>>>> If starting the server activates a minor mode, then Emacs can use a
>>>> minor-mode keymap for server-mode. [snip]
>>>
>>> I'm fine with doing that, but we must also make sure this mode is
>>> turned on when users invoke server-start interactively.
>>
>> I think that makes sense, and it should be fairly straightforward. I'll
>> work on a patch for this.
> 
> Thanks.

Ok, here's a patch for that. It just sets the 'server-mode' variable 
(and 'global-minor-modes' to be polite) inside 'server-start'. It would 
probably be more elegant if we could make 'server-start' just call 
'(server-mode)' (instead of the other way around like it is now), but I 
think it would be hard to do that while staying 100% compatible. Does 
this seem ok?

[-- Attachment #2: 0001-Enable-disable-server-mode-when-starting-stopping-th.patch --]
[-- Type: text/plain, Size: 2452 bytes --]

From 687f2ab4803c41f6a33280048243b0962816d2b5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 2 Nov 2022 09:22:43 -0700
Subject: [PATCH] Enable/disable 'server-mode' when starting/stopping the
 server

* lisp/server.el (server-mode-keymap): New keymap...
(server-mode): ... use it.
(server-start): Update the 'server-mode' variable (and sync to
'global-minor-modes') when starting/stopping the server.
---
 lisp/server.el | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..18548186b1 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -670,7 +670,6 @@ server-start
                             "/tmp/")
                  (ignore-errors
                    (delete-directory (file-name-directory server-file))))))
-         (setq server-mode nil) ;; already set by the minor mode code
          (display-warning
           'server
           (concat "Unable to start the Emacs server.\n"
@@ -688,7 +687,9 @@ server-start
       (if leave-dead
 	  (progn
 	    (unless (eq t leave-dead) (server-log (message "Server stopped")))
-	    (setq server-process nil))
+            (setq server-mode nil
+                  global-minor-modes (delq 'server-mode global-minor-modes)
+                  server-process nil))
 	;; Make sure there is a safe directory in which to place the socket.
 	(server-ensure-safe-dir server-dir)
 	(when server-process
@@ -728,6 +729,8 @@ server-start
 			       :plist '(:authenticated t)))))
 	  (unless server-process (error "Could not start server process"))
 	  (process-put server-process :server-file server-file)
+          (setq server-mode t)
+          (push 'server-mode global-minor-modes)
 	  (when server-use-tcp
 	    (let ((auth-key (server-get-auth-key)))
 	      (process-put server-process :auth-key auth-key)
@@ -796,6 +799,10 @@ server-running-p
 	t)
     (file-error nil)))
 
+;; This keymap is empty, but allows users to define keybindings to use
+;; when `server-mode' is active.
+(defvar-keymap server-mode-keymap)
+
 ;;;###autoload
 (define-minor-mode server-mode
   "Toggle Server mode.
@@ -805,6 +812,7 @@ server-mode
 `server-start' for details."
   :global t
   :version "22.1"
+  :keymap server-mode-keymap
   ;; Fixme: Should this check for an existing server socket and do
   ;; nothing if there is one (for multiple Emacs sessions)?
   (server-start (not server-mode)))
-- 
2.25.1

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Wed, 2 Nov 2022 15:09:21 -0700
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >>> I'm fine with doing that, but we must also make sure this mode is
> >>> turned on when users invoke server-start interactively.
> >>
> >> I think that makes sense, and it should be fairly straightforward. I'll
> >> work on a patch for this.
> > 
> > Thanks.
> 
> Ok, here's a patch for that. It just sets the 'server-mode' variable 
> (and 'global-minor-modes' to be polite) inside 'server-start'. It would 
> probably be more elegant if we could make 'server-start' just call 
> '(server-mode)' (instead of the other way around like it is now), but I 
> think it would be hard to do that while staying 100% compatible. Does 
> this seem ok?

Yes, but let's wait for a few days before installing to give people
chance to chime in.

Thanks.

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On 11/2/2022 11:25 PM, Eli Zaretskii wrote:
>> Date: Wed, 2 Nov 2022 15:09:21 -0700
>> Cc: 58909@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Ok, here's a patch for that. It just sets the 'server-mode' variable
>> (and 'global-minor-modes' to be polite) inside 'server-start'. It would
>> probably be more elegant if we could make 'server-start' just call
>> '(server-mode)' (instead of the other way around like it is now), but I
>> think it would be hard to do that while staying 100% compatible. Does
>> this seem ok?
> 
> Yes, but let's wait for a few days before installing to give people
> chance to chime in.

Sounds good to me. In the meantime, I added some regression tests for my 
change (see the first attached patch). I'll take a look at adding some 
more tests for server.el code too (in another bug#, though).

I also attached a second patch to show an idea I had: since the first 
patch adds a minor-mode keymap for 'server-mode' (and ensures that the 
mode is activated whenever the server is running), we could get rid of 
the server-specific logic from lisp/files.el. Instead of mapping 'C-x 
C-c' to 'save-buffers-kill-terminal' all the time, Emacs could map it to 
'save-buffers-kill-emacs' by default. Then, when the server is running, 
remap 'save-buffers-kill-emacs' to 'save-buffers-kill-terminal'.

Given that the old code already works, I'm not sure this change is worth 
the risk (it would be very bad if I made a mistake and broke the ability 
to exit Emacs). Still, I *think* this should work just the same as the 
old code, and it helps keep server-specific code inside server.el.

I don't have any strong opinions on whether this second patch should get 
merged (I'm totally fine with throwing it out), but I wanted to show off 
at least one theoretical benefit of the new keymap.

[-- Attachment #2: 0001-Enable-disable-server-mode-when-starting-stopping-th.patch --]
[-- Type: text/plain, Size: 4299 bytes --]

From 24a51f080f0e790b0681a1745fd32f7d967437e1 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 2 Nov 2022 09:22:43 -0700
Subject: [PATCH 1/2] Enable/disable 'server-mode' when starting/stopping the
 server

* lisp/server.el (server-mode-map): New keymap...
(server-mode): ... use it.
(server-start): Update the 'server-mode' variable (and sync to
'global-minor-modes') when starting/stopping the server.

* test/lisp/server-tests.el: New file.
---
 lisp/server.el            | 12 ++++++++++--
 test/lisp/server-tests.el | 41 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 test/lisp/server-tests.el

diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..553890ce29 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -670,7 +670,6 @@ server-start
                             "/tmp/")
                  (ignore-errors
                    (delete-directory (file-name-directory server-file))))))
-         (setq server-mode nil) ;; already set by the minor mode code
          (display-warning
           'server
           (concat "Unable to start the Emacs server.\n"
@@ -688,7 +687,9 @@ server-start
       (if leave-dead
 	  (progn
 	    (unless (eq t leave-dead) (server-log (message "Server stopped")))
-	    (setq server-process nil))
+            (setq server-mode nil
+                  global-minor-modes (delq 'server-mode global-minor-modes)
+                  server-process nil))
 	;; Make sure there is a safe directory in which to place the socket.
 	(server-ensure-safe-dir server-dir)
 	(when server-process
@@ -728,6 +729,8 @@ server-start
 			       :plist '(:authenticated t)))))
 	  (unless server-process (error "Could not start server process"))
 	  (process-put server-process :server-file server-file)
+          (setq server-mode t)
+          (push 'server-mode global-minor-modes)
 	  (when server-use-tcp
 	    (let ((auth-key (server-get-auth-key)))
 	      (process-put server-process :auth-key auth-key)
@@ -796,6 +799,10 @@ server-running-p
 	t)
     (file-error nil)))
 
+;; This keymap is empty, but allows users to define keybindings to use
+;; when `server-mode' is active.
+(defvar-keymap server-mode-map)
+
 ;;;###autoload
 (define-minor-mode server-mode
   "Toggle Server mode.
@@ -805,6 +812,7 @@ server-mode
 `server-start' for details."
   :global t
   :version "22.1"
+  :keymap server-mode-map
   ;; Fixme: Should this check for an existing server socket and do
   ;; nothing if there is one (for multiple Emacs sessions)?
   (server-start (not server-mode)))
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
new file mode 100644
index 0000000000..351b8ef8d1
--- /dev/null
+++ b/test/lisp/server-tests.el
@@ -0,0 +1,41 @@
+;;; server-tests.el --- Emacs server test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'server)
+
+;;; Tests:
+
+(ert-deftest server-test/server-start-sets-minor-mode ()
+  "Ensure that calling `server-start' also sets `server-mode' properly."
+  (server-start)
+  (unwind-protect
+      (progn
+        ;; Make sure starting the server activates the minor mode.
+        (should (eq server-mode t))
+        (should (memq 'server-mode global-minor-modes)))
+    ;; Always stop the server, even if the above checks fail.
+    (server-start t))
+  ;; Make sure stopping the server deactivates the minor mode.
+  (should (eq server-mode nil))
+  (should-not (memq 'server-mode global-minor-modes)))
+
+;;; server-tests.el ends here
-- 
2.25.1


[-- Attachment #3: 0002-IDEA-Take-advantage-of-new-server-mode-map-to-handle.patch --]
[-- Type: text/plain, Size: 8283 bytes --]

From d4d08aec0b0669e111bef2613775020f24ab9562 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 6 Nov 2022 12:05:45 -0800
Subject: [PATCH 2/2] [IDEA] Take advantage of new 'server-mode-map' to handle
 remapping 'C-x C-c'

---
 doc/emacs/entering.texi |  6 +++---
 doc/lispref/os.texi     | 20 ++++++++++----------
 lisp/files.el           | 16 +---------------
 lisp/help.el            |  2 +-
 lisp/menu-bar.el        |  2 +-
 lisp/server.el          | 20 +++++++++++++++++---
 lisp/startup.el         |  2 +-
 lisp/tutorial.el        |  2 +-
 8 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/doc/emacs/entering.texi b/doc/emacs/entering.texi
index 6069da0380..76e5bfca0f 100644
--- a/doc/emacs/entering.texi
+++ b/doc/emacs/entering.texi
@@ -98,16 +98,16 @@ Exiting
 
 @table @kbd
 @item C-x C-c
-Kill Emacs (@code{save-buffers-kill-terminal}).
+Kill Emacs (@code{save-buffers-kill-emacs}).
 @item C-z
 On a text terminal, suspend Emacs; on a graphical display,
 iconify (or ``minimize'') the selected frame (@code{suspend-frame}).
 @end table
 
 @kindex C-x C-c
-@findex save-buffers-kill-terminal
+@findex save-buffers-kill-emacs
   @dfn{Killing} Emacs means terminating the Emacs program.  To do
-this, type @kbd{C-x C-c} (@code{save-buffers-kill-terminal}).  A
+this, type @kbd{C-x C-c} (@code{save-buffers-kill-emacs}).  A
 two-character key sequence is used to make it harder to type by
 accident.  If there are any modified file-visiting buffers when you
 type @kbd{C-x C-c}, Emacs first offers to save these buffers.  If you
diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 3e16ac0eb4..fcad449e0c 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -725,12 +725,12 @@ Killing Emacs
 @cindex SIGINT
 @cindex operating system signal
   The @code{kill-emacs} function is normally called via the
-higher-level command @kbd{C-x C-c}
-(@code{save-buffers-kill-terminal}).  @xref{Exiting,,, emacs, The GNU
-Emacs Manual}.  It is also called automatically if Emacs receives a
-@code{SIGTERM} or @code{SIGHUP} operating system signal (e.g., when the
-controlling terminal is disconnected), or if it receives a
-@code{SIGINT} signal while running in batch mode (@pxref{Batch Mode}).
+higher-level command @kbd{C-x C-c} (@code{save-buffers-kill-emacs}).
+@xref{Exiting,,, emacs, The GNU Emacs Manual}.  It is also called
+automatically if Emacs receives a @code{SIGTERM} or @code{SIGHUP}
+operating system signal (e.g., when the controlling terminal is
+disconnected), or if it receives a @code{SIGINT} signal while running
+in batch mode (@pxref{Batch Mode}).
 
 @defvar kill-emacs-hook
 This normal hook is run by @code{kill-emacs}, before it kills Emacs.
@@ -745,12 +745,12 @@ Killing Emacs
   When Emacs is killed, all the information in the Emacs process,
 aside from files that have been saved, is lost.  Because killing Emacs
 inadvertently can lose a lot of work, the
-@code{save-buffers-kill-terminal} command queries for confirmation if
-you have buffers that need saving or subprocesses that are running.
-It also runs the abnormal hook @code{kill-emacs-query-functions}:
+@code{save-buffers-kill-emacs} command queries for confirmation if you
+have buffers that need saving or subprocesses that are running.  It
+also runs the abnormal hook @code{kill-emacs-query-functions}:
 
 @defopt kill-emacs-query-functions
-When @code{save-buffers-kill-terminal} is killing Emacs, it calls the
+When @code{save-buffers-kill-emacs} is killing Emacs, it calls the
 functions in this hook, after asking the standard questions and before
 calling @code{kill-emacs}.  The functions are called in order of
 appearance, with no arguments.  Each function can ask for additional
diff --git a/lisp/files.el b/lisp/files.el
index a282532258..9f88a3e646 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8072,20 +8072,6 @@ save-buffers-kill-emacs
          (funcall confirm "Really exit Emacs? "))
      (kill-emacs nil restart))))
 
-(defun save-buffers-kill-terminal (&optional arg)
-  "Offer to save each buffer, then kill the current connection.
-If the current frame has no client, kill Emacs itself using
-`save-buffers-kill-emacs'.
-
-With prefix ARG, silently save all file-visiting buffers, then kill.
-
-If emacsclient was started with a list of file names to edit, then
-only these files will be asked to be saved."
-  (interactive "P")
-  (if (frame-parameter nil 'client)
-      (server-save-buffers-kill-terminal arg)
-    (save-buffers-kill-emacs arg)))
-
 (defun restart-emacs ()
   "Kill the current Emacs process and start a new one.
 This goes through the same shutdown procedure as
@@ -8700,7 +8686,7 @@ ctl-x-map
 (define-key ctl-x-map "i" 'insert-file)
 (define-key esc-map "~" 'not-modified)
 (define-key ctl-x-map "\C-d" 'list-directory)
-(define-key ctl-x-map "\C-c" 'save-buffers-kill-terminal)
+(define-key ctl-x-map "\C-c" 'save-buffers-kill-emacs)
 (define-key ctl-x-map "\C-q" 'read-only-mode)
 
 (define-key ctl-x-4-map "f" 'find-file-other-window)
diff --git a/lisp/help.el b/lisp/help.el
index b25a8ce299..41f654965d 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -130,7 +130,7 @@ help-button-cache
 
 (defvar help-quick-sections
   '(("File"
-     (save-buffers-kill-terminal . "exit")
+     (save-buffers-kill-emacs . "exit")
      (find-file . "find")
      (write-file . "write")
      (save-buffer . "save")
diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 849e0f7723..4e746c3483 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -84,7 +84,7 @@ menu-bar-file-menu
 
     ;; The "File" menu items
     (bindings--define-key menu [exit-emacs]
-      '(menu-item "Quit" save-buffers-kill-terminal
+      '(menu-item "Quit" save-buffers-kill-emacs
                   :help "Save unsaved buffers, then exit"))
 
     (bindings--define-key menu [separator-exit]
diff --git a/lisp/server.el b/lisp/server.el
index 553890ce29..af79fa046f 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -799,9 +799,8 @@ server-running-p
 	t)
     (file-error nil)))
 
-;; This keymap is empty, but allows users to define keybindings to use
-;; when `server-mode' is active.
-(defvar-keymap server-mode-map)
+(defvar-keymap server-mode-map
+  "<remap> <save-buffers-kill-emacs>" #'save-buffers-kill-terminal)
 
 ;;;###autoload
 (define-minor-mode server-mode
@@ -1773,6 +1772,21 @@ server-save-buffers-kill-terminal
 	       (server-delete-client proc)))
 	    (t (error "Invalid client frame"))))))
 
+;;;###autoload
+(defun save-buffers-kill-terminal (&optional arg)
+  "Offer to save each buffer, then kill the current connection.
+If the current frame has no client, kill Emacs itself using
+`save-buffers-kill-emacs'.
+
+With prefix ARG, silently save all file-visiting buffers, then kill.
+
+If emacsclient was started with a list of file names to edit, then
+only these files will be asked to be saved."
+  (interactive "P")
+  (if (frame-parameter nil 'client)
+      (server-save-buffers-kill-terminal arg)
+    (save-buffers-kill-emacs arg)))
+
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
   (when server-stop-automatically
diff --git a/lisp/startup.el b/lisp/startup.el
index 70267fc857..2d38f99850 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -2308,7 +2308,7 @@ normal-no-mouse-startup-screen
                    'action (lambda (_button) (view-order-manuals))
                    'follow-link t)
     (insert (substitute-command-keys
-             "\t   \\[view-order-manuals]\tExit Emacs\t   \\[save-buffers-kill-terminal]")))
+             "\t   \\[view-order-manuals]\tExit Emacs\t   \\[save-buffers-kill-emacs]")))
 
   ;; Say how to use the menu bar with the keyboard.
   (insert "\n")
diff --git a/lisp/tutorial.el b/lisp/tutorial.el
index 2c787ae559..ba3a5505c2 100644
--- a/lisp/tutorial.el
+++ b/lisp/tutorial.el
@@ -209,7 +209,7 @@ tutorial--default-keys
            `((ESC-prefix [27])
              (Control-X-prefix [?\C-x])
              (mode-specific-command-prefix [?\C-c])
-             (save-buffers-kill-terminal [?\C-x ?\C-c])
+             (save-buffers-kill-emacs [?\C-x ?\C-c])
 
              ;; * SUMMARY
              (scroll-up-command [?\C-v])
-- 
2.25.1

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> Date: Sun, 6 Nov 2022 12:23:58 -0800
> Cc: 58909@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> I also attached a second patch to show an idea I had: since the first 
> patch adds a minor-mode keymap for 'server-mode' (and ensures that the 
> mode is activated whenever the server is running), we could get rid of 
> the server-specific logic from lisp/files.el. Instead of mapping 'C-x 
> C-c' to 'save-buffers-kill-terminal' all the time, Emacs could map it to 
> 'save-buffers-kill-emacs' by default. Then, when the server is running, 
> remap 'save-buffers-kill-emacs' to 'save-buffers-kill-terminal'.

This means a different binding for "C-x C-c" depending on situation,
and the binding to save-buffers-kill-terminal depending on a minor
mode.  Previously, "C-x C-c" would do its job regardless of the mode.
I'm not sure this is for the best.  I'd be interested to hear from
others, if anyone has an opinion.

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Robert Pluim]

>>>>> On Tue, 08 Nov 2022 16:47:02 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> Date: Sun, 6 Nov 2022 12:23:58 -0800
    >> Cc: 58909@debbugs.gnu.org
    >> From: Jim Porter <jporterbugs@gmail.com>
    >> 
    >> I also attached a second patch to show an idea I had: since the first 
    >> patch adds a minor-mode keymap for 'server-mode' (and ensures that the 
    >> mode is activated whenever the server is running), we could get rid of 
    >> the server-specific logic from lisp/files.el. Instead of mapping 'C-x 
    >> C-c' to 'save-buffers-kill-terminal' all the time, Emacs could map it to 
    >> 'save-buffers-kill-emacs' by default. Then, when the server is running, 
    >> remap 'save-buffers-kill-emacs' to 'save-buffers-kill-terminal'.

    Eli> This means a different binding for "C-x C-c" depending on situation,
    Eli> and the binding to save-buffers-kill-terminal depending on a minor
    Eli> mode.  Previously, "C-x C-c" would do its job regardless of the mode.
    Eli> I'm not sure this is for the best.  I'd be interested to hear from
    Eli> others, if anyone has an opinion.

Rules of Emacs:

1. If it works, donʼt change it
2. See Rule 1
3. No, really, see Rule 1

I honestly donʼt see what benefit the minor mode would bring here (and
it might just bring new and 'interesting' bugs).

Robert
-- 

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Jim Porter <jporterbugs@gmail.com>,  58909@debbugs.gnu.org
> Date: Tue, 08 Nov 2022 16:08:04 +0100
> 
> I honestly donʼt see what benefit the minor mode would bring here (and
> it might just bring new and 'interesting' bugs).

The minor mode already exists, and has been with us for many years.

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Robert Pluim]

>>>>> On Tue, 08 Nov 2022 17:13:02 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Jim Porter <jporterbugs@gmail.com>,  58909@debbugs.gnu.org
    >> Date: Tue, 08 Nov 2022 16:08:04 +0100
    >> 
    >> I honestly donʼt see what benefit the minor mode would bring here (and
    >> it might just bring new and 'interesting' bugs).

    Eli> The minor mode already exists, and has been with us for many years.

OK. I donʼt see what benefit the *use* of the minor mode in this way
brings :-)

Robert
-- 

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 11/8/2022 7:29 AM, Robert Pluim wrote:
> OK. I donʼt see what benefit the *use* of the minor mode in this way
> brings :-)

The actual change that I'd like to merge is just to make the minor mode 
activate/deactivate consistently, whereas previously it would activate 
in some situations where you start the Emacs server, but not others. I'd 
classify that as "not working".

Having the minor mode makes it easier for third-party code (user configs 
or packages) to define key bindings that are only active when the server 
is running. That's useful to me, since I don't *always* use the Emacs 
server.

As for the change to how 'C-x C-c' is bound, I don't have any strong 
opinion on merging it. It's probably more risk than it's worth, and if 
we were to merge it, I'd prefer to do so after cutting the 29 branch, 
since then there'd be more time to find any bugs.[1] However, since the 
only real benefit is to clean up some dependencies in the code, I'm not 
sure it's worth the effort.

[1] I'd also want to have regression tests for all of that.

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Robert Pluim]

>>>>> On Tue, 8 Nov 2022 08:52:10 -0800, Jim Porter <jporterbugs@gmail.com> said:

    Jim> On 11/8/2022 7:29 AM, Robert Pluim wrote:
    >> OK. I donʼt see what benefit the *use* of the minor mode in this way
    >> brings :-)

    Jim> The actual change that I'd like to merge is just to make the minor
    Jim> mode activate/deactivate consistently, whereas previously it would
    Jim> activate in some situations where you start the Emacs server, but not
    Jim> others. I'd classify that as "not working".

That sounds good

    Jim> Having the minor mode makes it easier for third-party code (user
    Jim> configs or packages) to define key bindings that are only active when
    Jim> the server is running. That's useful to me, since I don't *always* use
    Jim> the Emacs server.

as does this

    Jim> As for the change to how 'C-x C-c' is bound, I don't have any strong
    Jim> opinion on merging it. It's probably more risk than it's worth, and if
    Jim> we were to merge it, I'd prefer to do so after cutting the 29 branch,
    Jim> since then there'd be more time to find any bugs.[1] However, since
    Jim> the only real benefit is to clean up some dependencies in the code,
    Jim> I'm not sure it's worth the effort.

I think it would be too risky a change.

Robert
-- 

bug#58909: 29.0.50; [PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 11/9/2022 2:06 AM, Robert Pluim wrote:
> That sounds good
[snip]
> as does this
[snip]
> I think it would be too risky a change.

Thanks. I agree with all that.

Since there haven't been any further comments in the past week, I've 
merged just the first patch (to make 'server-mode' (de)activate at the 
right times) as 0147e1ed831151dddac65727886d5a70bbab9f02.

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Stefan Kangas]

Jim Porter <jporterbugs@gmail.com> writes:

> On 11/9/2022 2:06 AM, Robert Pluim wrote:
>> That sounds good
> [snip]
>> as does this
> [snip]
>> I think it would be too risky a change.
>
> Thanks. I agree with all that.
>
> Since there haven't been any further comments in the past week, I've merged just the first patch (to make
> 'server-mode' (de)activate at the right times) as 0147e1ed831151dddac65727886d5a70bbab9f02.

This was a long thread.  From skimming it, it's not clear to me if
there's anything left to do here?

bug#58909: 29.0.50; [WIP PATCH] Deleting the last frame of an emacsclient doesn't ask to save

[Jim Porter]

On 9/7/2023 2:03 PM, Stefan Kangas wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> On 11/9/2022 2:06 AM, Robert Pluim wrote:
>>> That sounds good
>> [snip]
>>> as does this
>> [snip]
>>> I think it would be too risky a change.
>>
>> Thanks. I agree with all that.
>>
>> Since there haven't been any further comments in the past week, I've merged just the first patch (to make
>> 'server-mode' (de)activate at the right times) as 0147e1ed831151dddac65727886d5a70bbab9f02.
> 
> This was a long thread.  From skimming it, it's not clear to me if
> there's anything left to do here?

Yeah, I'd say we're done here; I just forgot to close the bug at the 
time. Closing this now.