Subject: bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

From: Jim Porter <[email-masked]>
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."

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...
From 563036eaf0924f4777f25e74e9f611419fc031cf Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Fri, 19 Nov 2021 19:49:38 -0800
Subject: [PATCH 1/2] Ensure killing an emacsclient terminal prompts to save
 files

Previously, when using 'server-stop-automatically', killing an
emacsclient terminal wouldn't prompt the user to save the files
initially passed to emacsclient (unless it was the last client).

* lisp/server.el (server-save-buffers-kill-terminal): Check
'server-stop-automatically' and handle killing Emas as needed.
(server-stop-automatically--handle-delete-frame): Remove logic
specific to 'save-buffers-kill-terminal'.
---
 lisp/server.el | 94 +++++++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 2f003a380a..e2d20b1b02 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1727,52 +1727,60 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; 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)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (cond ((eq proc 'nowait)
+           ;; Nowait frames have no client buffer list.
+           (if (if server-stop-automatically
+                   (cddr (frame-list))
+                 (cdr (frame-list)))
+               ;; If there are any other frames (excluding the daemon
+               ;; frame when `server-stop-automatically' is non-nil),
+               ;; only delete this frame.  FIXME: It would be nice to
+               ;; delete any other frames created by this frame (as
+               ;; when killing the terminal of an ordinary client
+               ;; below), but we can't distinguish separate groups of
+               ;; nowait frames currently.
+               (progn (save-some-buffers arg)
+                      (delete-frame))
+             ;; If we're the last frame standing, kill Emacs.
+             (save-buffers-kill-emacs arg)))
+          ((processp proc)
+           (if (seq-some
+                (lambda (frame)
+                  (let ((p (frame-parameter frame 'client)))
+                    (unless (and server-stop-automatically (null p))
+                      (not (equal proc p)))))
+                (frame-list))
+               ;; If there are any frames from other clients, only
+               ;; delete this client.  If `server-stop-automatically'
+               ;; is nil, check for frames from the server process as
+               ;; well.
+               (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))
+             ;; If all frames are from this client, kill Emacs.
+             (save-buffers-kill-emacs arg)))
+          (t (error "Invalid client frame")))))
 
 (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))
-	  (null (cddr (frame-list))))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+  (when (and ;; Check that the frame is a client frame.
+             ;; Note: `server-delete-client' sets `client' to nil
+             ;; before calling `delete-frame'. That's good, since we
+             ;; don't want to call `save-buffers-kill-emacs' in that
+             ;; case anyway.
+             (frame-parameter frame 'client)
+             (null (cddr (frame-list))))
+    (save-buffers-kill-emacs)))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1

From cccba7784d7a5af41b651ab0c29d2339bebc6732 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Fri, 19 Nov 2021 20:14:33 -0800
Subject: [PATCH 2/2] Don't explicitly delete client frames when killing Emacs
 anyway

This resolves an obscure issue where killing an emacsclient terminal
when there were no other frames would fail. Now, killing the terminal
properly kills Emacs.

* lisp/server (server-start): Add 'noframe' option to avoid deleting
frames.
(server-force-stop): Pass 'noframe' to 'server-stop'.
---
 lisp/server.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index e2d20b1b02..ec32da4fdb 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -609,7 +609,7 @@ server-get-auth-key
     (server-generate-key)))
 
 ;;;###autoload
-(defun server-start (&optional leave-dead inhibit-prompt)
+(defun server-start (&optional leave-dead inhibit-prompt noframe)
   "Allow this Emacs process to be a server for client processes.
 This starts a server communications subprocess through which client
 \"editors\" can send your editing commands to this Emacs job.
@@ -619,6 +619,10 @@ server-start
 Optional argument LEAVE-DEAD (interactively, a prefix arg) means just
 kill any existing server communications subprocess.
 
+If NOFRAME is non-nil, let any existing frames associated with a
+client process live.  This is useful, for example, when killing
+Emacs, in which case the frames will die anyway.
+
 If a server is already running, restart it.  If clients are
 running, ask the user for confirmation first, unless optional
 argument INHIBIT-PROMPT is non-nil.
@@ -681,7 +685,7 @@ server-start
          (setq leave-dead t)))
       ;; If this Emacs already had a server, clear out associated status.
       (while server-clients
-	(server-delete-client (car server-clients)))
+	(server-delete-client (car server-clients) noframe))
       ;; Now any previous server is properly stopped.
       (if leave-dead
 	  (progn
@@ -740,7 +744,7 @@ server-start
 (defun server-force-stop ()
   "Kill all connections to the current server.
 This function is meant to be called from `kill-emacs-hook'."
-  (server-start t t))
+  (server-start t t 'noframe))
 
 ;;;###autoload
 (defun server-force-delete (&optional name)
-- 
2.25.1