GNU bug report logs - #59668
29.0.50; [PATCH] Make 'server-stop-automatically' into a defcustom

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 59668 in the body.
You can then email your comments to 59668 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Make 'server-stop-automatically' into a defcustom
Date: Mon, 28 Nov 2022 20:23:17 -0800

[Message part 1 (text/plain, inline)]

The manual says:

  The Emacs server can optionally be stopped automatically when
  certain conditions are met.  To do this, call the function
  @code{server-stop-automatically} in your init file (@pxref{Init
  File}), with one of the following arguments...

It'd be nice if this were a defcustom so that people who prefer the 
Customize interface could use that instead of editing their init files. 
Here's a patch for that.

I've tried to make sure this is as robust as possible, so that 
everything is always properly set, especially when adjusting the value 
in the Customize UI. It should even work correctly if a user stopped the 
server temporarily and restarted it (you might do this to clear out old 
clients, do some work, and then make the server available again). Maybe 
this last bit is a little paranoid, but it was minimal extra work 
compared to getting the Customize part working.

One question though: should this only go on the master branch, or should 
it go into the 29 branch? To me, it seems like it could go either way, 
though I think it'd be nice to make this easier for users in 29. I'll do 
whatever the maintainers think is best though.

If it goes on the master branch only, I'll add back the function form of 
'server-stop-automatically' for compatibility, and then mark it obsolete.

[0001-Make-server-stop-automatically-into-a-defcustom.patch (text/plain, attachment)]

Message #10 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50;
 [PATCH] Make 'server-stop-automatically' into a defcustom
Date: Thu, 01 Dec 2022 19:08:07 +0200

> Date: Mon, 28 Nov 2022 20:23:17 -0800
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
>    The Emacs server can optionally be stopped automatically when
>    certain conditions are met.  To do this, call the function
>    @code{server-stop-automatically} in your init file (@pxref{Init
>    File}), with one of the following arguments...
> 
> It'd be nice if this were a defcustom so that people who prefer the 
> Customize interface could use that instead of editing their init files. 
> Here's a patch for that.

Thanks.

> One question though: should this only go on the master branch, or should 
> it go into the 29 branch? To me, it seems like it could go either way, 
> though I think it'd be nice to make this easier for users in 29. I'll do 
> whatever the maintainers think is best though.

Let's not start installing new features on the release branch.  We have
enough new stuff there already.  Besides, the changes you suggest are hardly
trivial, and saying just "make a function into a defcustom" doesn't come
close to describing it, IMO.

So please install on master, once you take care of the comments below.

> If it goes on the master branch only, I'll add back the function form of 
> 'server-stop-automatically' for compatibility, and then mark it obsolete.

I see no reason to make the function obsolete.  It does not harm to have
both a variable and a function by the same name.

> -@findex server-stop-automatically
> +@vindex server-stop-automatically
>    The Emacs server can optionally be stopped automatically when
> -certain conditions are met.  To do this, call the function
> -@code{server-stop-automatically} in your init file (@pxref{Init
> -File}), with one of the following arguments:
> +certain conditions are met.  To do this, set the option
> +@code{server-stop-automatically} to one of the following values:
>  
>  @itemize
>  @item
> -With the argument @code{empty}, the server is stopped when it has no
> +With the value @code{empty}, the server is stopped when it has no
>  clients, no unsaved file-visiting buffers and no running processes
>  anymore.
>  
>  @item
> -With the argument @code{delete-frame}, when the last client frame is
> +With the value @code{delete-frame}, when the last client frame is
>  being closed, you are asked whether each unsaved file-visiting buffer
>  must be saved and each unfinished process can be stopped, and if so,
>  the server is stopped.

This @itemize list should be converted to a @table, formatted like this:

  @item empty
  This value caused the server to be stopped when...

  @item delete-frame
  This value means that when the last client frame is deleted...

etc., I guess you get the idea.

> @@ -1780,7 +1784,8 @@ 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
> +  (if (and (daemonp)
> +           (memq server-stop-automatically '(kill-terminal delete-frame))

Why is this needed?  I guess I don't understand why non-trivial code changes
are in a patch that was supposed to just add a defcustom?

> +(defun server-apply-stop-automatically ()
> +  "Apply the current value of `server-stop-automatically'.
> +This function adds or removes the necessary helpers to manage
> +stopping the Emacs server automatically, depending on the whether
> +the server is running or not.  This function only applies when
> +running Emacs as a daemon."

And why this significant refactoring of the original function? was there
something wrong with it?

Message #13 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Thu, 1 Dec 2022 10:33:57 -0800

On 12/1/2022 9:08 AM, Eli Zaretskii wrote:
>> Date: Mon, 28 Nov 2022 20:23:17 -0800
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> One question though: should this only go on the master branch, or should
>> it go into the 29 branch? To me, it seems like it could go either way,
>> though I think it'd be nice to make this easier for users in 29. I'll do
>> whatever the maintainers think is best though.
> 
> Let's not start installing new features on the release branch. ...
Sounds good to me.

>> If it goes on the master branch only, I'll add back the function form of
>> 'server-stop-automatically' for compatibility, and then mark it obsolete.
> 
> I see no reason to make the function obsolete.  It does not harm to have
> both a variable and a function by the same name.

Fine by me. The function will just be a one-liner anyway: (setopt 
server-stop-automatically arg).

> This @itemize list should be converted to a @table, formatted like this:
> 
>    @item empty
>    This value caused the server to be stopped when...
> 
>    @item delete-frame
>    This value means that when the last client frame is deleted...
> 
> etc., I guess you get the idea.

Will do. I made the minimal set of changes to the manual, but while I'm 
here, I agree that it would be good to improve things further.

>> @@ -1780,7 +1784,8 @@ 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
>> +  (if (and (daemonp)
>> +           (memq server-stop-automatically '(kill-terminal delete-frame))
> 
> Why is this needed?  I guess I don't understand why non-trivial code changes
> are in a patch that was supposed to just add a defcustom?

It's due to a change in the meaning of the 'server-stop-automatically' 
value. Previously, it was an internal variable that was set to nil after 
calling "(server-stop-automatically 'empty)", or when calling 
'server-stop-automatically' in a non-daemon session. Since 
'server-stop-automatically' is now a defcustom, that means that a) it 
can really be set to 'empty', and b) it can be non-nil in non-daemon 
sessions. So this extra code is just to account for the change in meaning.

I could make a helper function that returns the same value as the *old* 
version of the 'server-stop-automatically' variable; either way has the 
same meaning. I could also keep the old variable around, possibly 
renamed to something like 'server-stop-automatically--kill-terminal', 
but I think a helper function would be cleaner.

>> +(defun server-apply-stop-automatically ()
>> +  "Apply the current value of `server-stop-automatically'.
>> +This function adds or removes the necessary helpers to manage
>> +stopping the Emacs server automatically, depending on the whether
>> +the server is running or not.  This function only applies when
>> +running Emacs as a daemon."
> 
> And why this significant refactoring of the original function? was there
> something wrong with it?

The previous implementation was limited in that you could call it once, 
but you couldn't necessarily call it a second time to change the 
setting. For example, this would put you in an inconsistent state:

  (server-stop-automatically 'delete-frame)
  (server-stop-automatically 'empty)

After this, the 'empty' setting would be enabled, and the 'delete-frame' 
setting would still be partially-enabled too.

That wasn't so much of a problem when you'd just call this function only 
once in your init file, but for the Customize interface, I think it's 
important to make sure that users can set the defcustom multiple times, 
e.g. if they change their minds while customizing it. The changes for 
'server-apply-stop-automatically' are to support that.

As you said, these changes are more extensive than "just" adding a 
defcustom, but all the other changes are there to support the Customize 
interface.

Message #16 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Thu, 1 Dec 2022 10:41:19 -0800

On 12/1/2022 10:33 AM, Jim Porter wrote:
> I could make a helper function that returns the same value as the *old* 
> version of the 'server-stop-automatically' variable; either way has the 
> same meaning.

Oops, the bit after the semicolon should be "this would do the same 
thing as the current conditional."

Message #19 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Thu, 1 Dec 2022 19:30:03 -0800

[Message part 1 (text/plain, inline)]

On 12/1/2022 9:08 AM, Eli Zaretskii wrote:
> I see no reason to make the function obsolete.  It does not harm to have
> both a variable and a function by the same name.

Ok, fixed. I've added back a 'server-stop-automatically' function that 
just sets the variable.

> This @itemize list should be converted to a @table, formatted like this:
> 
>    @item empty
>    This value caused the server to be stopped when...
> 
>    @item delete-frame
>    This value means that when the last client frame is deleted...

Done.

>> @@ -1780,7 +1784,8 @@ 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
>> +  (if (and (daemonp)
>> +           (memq server-stop-automatically '(kill-terminal delete-frame))
> 
> Why is this needed?  I guess I don't understand why non-trivial code changes
> are in a patch that was supposed to just add a defcustom?

Addressed in my other message, but I've added a defsubst to make this 
clearer (I hope).

[0001-Make-server-stop-automatically-into-a-defcustom.patch (text/plain, attachment)]

Message #22 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Fri, 02 Dec 2022 16:42:21 +0200

> Date: Thu, 1 Dec 2022 19:30:03 -0800
> Cc: 59668 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> @@ -1805,7 +1814,7 @@ server-save-buffers-kill-terminal
>  	    (t (error "Invalid client frame"))))))
>  
>  (defun server-stop-automatically--handle-delete-frame (frame)
> -  "Handle deletion of FRAME when `server-stop-automatically' is used."
> +  "Handle deletion of FRAME when `server-stop-automatically' is `delete-frame'."
>    (when server-stop-automatically
>      (if (if (and (processp (frame-parameter frame 'client))
>  		 (eq this-command 'save-buffers-kill-terminal))
> @@ -1828,7 +1837,7 @@ server-stop-automatically--handle-delete-frame
>  	  (delete-frame frame)))))

AFAIU, this delete-frame is called after save-buffers-kill-emacs, which is
strange: there will be no Emacs to perform this call after that.  What am I
missing?
> +          (const :tag "When empty" empty)

"When empty" doesn't explain itself well enough.  Can we come up with a
better description?

Otherwise LGTM, thanks.

Message #25 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Tue, 6 Dec 2022 17:39:52 -0800

[Message part 1 (text/plain, inline)]

On 12/2/2022 6:42 AM, Eli Zaretskii wrote:
>> Date: Thu, 1 Dec 2022 19:30:03 -0800
>> Cc: 59668 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> @@ -1805,7 +1814,7 @@ server-save-buffers-kill-terminal
>>   	    (t (error "Invalid client frame"))))))
>>   
>>   (defun server-stop-automatically--handle-delete-frame (frame)
>> -  "Handle deletion of FRAME when `server-stop-automatically' is used."
>> +  "Handle deletion of FRAME when `server-stop-automatically' is `delete-frame'."
>>     (when server-stop-automatically
>>       (if (if (and (processp (frame-parameter frame 'client))
>>   		 (eq this-command 'save-buffers-kill-terminal))
>> @@ -1828,7 +1837,7 @@ server-stop-automatically--handle-delete-frame
>>   	  (delete-frame frame)))))
> 
> AFAIU, this delete-frame is called after save-buffers-kill-emacs, which is
> strange: there will be no Emacs to perform this call after that.  What am I
> missing?

Thanks. I think that's just an oversight. I removed it in my patch for 
bug#51993, since this seems more closely-related to that bug.

>> +          (const :tag "When empty" empty)
> 
> "When empty" doesn't explain itself well enough.  Can we come up with a
> better description?

How about "When empty (no clients, unsaved files, or processes)"? That 
seems a bit clumsy to me, but I wanted to keep the word "empty" in there 
somehow, since that's the name of the option value.

Attached is an updated patch (rebased on top of my previous patches for 
bug#51993).

[0001-Make-server-stop-automatically-into-a-defcustom.patch (text/plain, attachment)]

Message #28 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Tue, 6 Dec 2022 17:49:34 -0800

[Message part 1 (text/plain, inline)]

On 12/6/2022 5:39 PM, Jim Porter wrote:
> Attached is an updated patch (rebased on top of my previous patches for 
> bug#51993).

Oops. Forgot to save the file after my last change.

[0001-Make-server-stop-automatically-into-a-defcustom.patch (text/plain, attachment)]

Message #31 received at 59668 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 59668 <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Wed, 07 Dec 2022 14:48:04 +0200

> Date: Tue, 6 Dec 2022 17:39:52 -0800
> Cc: 59668 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> >> +          (const :tag "When empty" empty)
> > 
> > "When empty" doesn't explain itself well enough.  Can we come up with a
> > better description?
> 
> How about "When empty (no clients, unsaved files, or processes)"?

Just leave the text inside the parentheses (but without the
parentheses themselves), there's no need to have the "empty part there.

> Attached is an updated patch (rebased on top of my previous patches for 
> bug#51993).

OK with the above change.  Thanks.

Message #36 received at 59668-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59668-done <at> debbugs.gnu.org
Subject: Re: bug#59668: 29.0.50; [PATCH] Make 'server-stop-automatically' into
 a defcustom
Date: Wed, 7 Dec 2022 22:02:39 -0800

On 12/7/2022 4:48 AM, Eli Zaretskii wrote:
>> Date: Tue, 6 Dec 2022 17:39:52 -0800
>> Cc: 59668 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> How about "When empty (no clients, unsaved files, or processes)"?
> 
> Just leave the text inside the parentheses (but without the
> parentheses themselves), there's no need to have the "empty part there.

Ok by me.

>> Attached is an updated patch (rebased on top of my previous patches for
>> bug#51993).
> 
> OK with the above change.  Thanks.

Thanks, merged to master as
153c67fa92eaad39410b1809ab9b125616bdc5c1. Closing this bug now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.