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

[Jim Porter <jporterbugs@gmail.com>]

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

On 12/2/2022 6:10 AM, Eli Zaretskii wrote:
> [Please in the future avoid changing the Subject when discussing bugs: it
> makes it harder for me and others to follow the discussion by grouping
> messages by Subject.]

Sorry about that. I thought it would be easier to see that I'd split the 
topic into two separate tracks. I'll keep that in mind for the future 
though.

> Thanks.  Surprisingly, the previous version was easier to review and agree
> to in some of its parts, because it kept more of the original code, and only
> changed the conditions when save-some-buffers or save-buffers-kill-emacs
> were called.

I think I must have misinterpreted where your main concerns were, and 
went down the wrong avenue as a result. I think your other comments here 
(and re-reading your previous message) have helped me get a better idea 
of your concerns, so I'll try to address your earlier message better.

On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
>>  (defun server-stop-automatically--handle-delete-frame (frame)
>>    "Handle deletion of FRAME when `server-stop-automatically' is used."
[snip -- long diff removed]
> 
> And here you completely rewrote a function.

Thinking about this some more, that change wasn't strictly necessary for 
29.1, since it should really just be dead code, and won't hurt anything. 
I've split this part out into a separate commit that we could apply to 
only the master branch. That simplifies the first commit (for 29.1) a 
fair bit.

>> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>>                          ;; ARG is non-nil), since we're not killing
>>                          ;; Emacs (unlike `save-buffers-kill-emacs').
>>  		        (and arg t)))
>> -	       (server-delete-client proc)))
>> -	    (t (error "Invalid client frame"))))))
>> +	         (server-delete-client proc))
>> +             ;; If `server-stop-automatically' is set, there are no
>> +             ;; other client processes, and no other client frames
>> +             ;; (e.g. `nowait' frames), kill Emacs.
>> +             (save-buffers-kill-emacs arg)))
>> +	  (t (error "Invalid client frame")))))
> 
> But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
> the original code, and I don't understand why.  The bug wasn't about this,
> was it?

I believe this change is the most important part of the patch, so I'll 
try to explain why I added this. The 'server-stop-automatically' feature 
causes the Emacs daemon to be killed when certain conditions are met. 
For the 'kill-terminal' case[1], that means that, "when the last client 
frame is being closed with C-x C-c, you are asked whether [various 
questions], and if so, the server is stopped." (From the manual section 
on this.)

The "last client frame" check is the conditional just before this hunk 
(you mentioned that this part was fine, so I didn't include it in this 
message). The latter part of the sentence in the manual is just a 
description of 'save-buffers-kill-emacs'. So I've tried to take the text 
of the manual and turn it into code.

Another way of looking at it: the new 'save-buffers-kill-emacs' call 
above is really the result of me moving the handling for this condition 
from 'server-stop-automatically--handle-delete-frame' into 
'server-save-buffers-kill-terminal'. Previously, that function was 
responsible for calling 'save-buffers-kill-emacs' at the right time. 
Doing it this way lets me avoid calling 'SSA--handle-delete-frame' so 
that we get the benefit of using the longstanding implementation of 
'server-save-buffers-kill-terminal', just with the minimum of necessary 
enhancements to make this stop-automatically setting behave as documented.

[1] This also applies to 'delete-frame', but that setting does more 
beyond this.

[-- Attachment #2: 29.1--0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch --]
[-- Type: text/plain, Size: 4324 bytes --]

From 6081e156ba34f9f7d8b212be8201639f2f689219 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 2 Dec 2022 12:14:50 -0800
Subject: [PATCH 1/2] Make killing a non-last client work the same no matter
 the auto-stop setting

Previously, if 'server-stop-automatically' was configured for
'kill-terminal' or 'delete-frame', killing a client via
'save-buffers-kill-terminal' wouldn't prompt about the saving files in
the client's buffer list (as it does when not using those settings).
This change ensures that those settings only apply when killing the
last client, as described in the manual (bug#51993).

* lisp/server.el (server-save-buffers-kill-terminal): Handle
'server-stop-automatically' behavior in this function, rather than
calling 'server-stop-automatically--handle-delete-frame'.
---
 lisp/server.el | 60 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..7e713eaecd 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1780,29 +1780,43 @@ 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 (length> (frame-list) (if server-stop-automatically 2 1))
+               ;; If there are any other frames, only delete this one.
+               ;; When `server-stop-automatically' is set, don't count
+               ;; the daemon frame.
+	       (progn (save-some-buffers arg)
+		      (delete-frame))
+	     ;; If we're the last frame standing, kill Emacs.
+	     (save-buffers-kill-emacs arg)))
+	  ((processp proc)
+           (if (or (not server-stop-automatically)
+                   (length> server-clients 1)
+                   (seq-some
+                    (lambda (frame)
+                      (when-let ((p (frame-parameter frame 'client)))
+                        (not (eq proc p))))
+                    (frame-list)))
+               ;; If `server-stop-automatically' is not enabled, there
+               ;; are any other clients, or there are frames not owned
+               ;; by the current client (e.g. `nowait' frames), then
+               ;; we just want to delete this client.
+	       (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))
+             ;; Otherwise, we want to 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."
-- 
2.25.1


[-- Attachment #3: master--0002-Remove-dead-code-from-server-stop-automatically-hand.patch --]
[-- Type: text/plain, Size: 1991 bytes --]

From a14b042982dc59767495905a9d45e0542124c1e6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 2 Dec 2022 12:15:46 -0800
Subject: [PATCH 2/2] ; Remove dead code from
 'server-stop-automatically--handle-delete-frame'

* lisp/server.el (server-stop-automatically--handle-delete-frame):
Since 'this-command' is never 'save-buffers-kill-terminal' in this
function, we can remove the second 'if' block and it's then-form,
leaving only the else-form.  Additionally, remove the 'delete-frame'
call; it's not necessary, since we just killed Emacs on the prior
line.
---
 lisp/server.el | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 7e713eaecd..aa62b25a89 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1820,26 +1820,10 @@ server-save-buffers-kill-terminal
 
 (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 server-stop-automatically
+             (null (cddr (frame-list))))
+    (let ((server-stop-automatically nil))
+      (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