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

From: Jim Porter <[email-masked]>
Date: Sun, 6 Nov 2022 12:23:58 -0800

On 11/2/2022 11:25 PM, Eli Zaretskii wrote:
>> Date: Wed, 2 Nov 2022 15:09:21 -0700
>> Cc: [email-masked]
>> From: Jim Porter <[email-masked]>
>>
>> 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.

From 24a51f080f0e790b0681a1745fd32f7d967437e1 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From d4d08aec0b0669e111bef2613775020f24ab9562 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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