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

[Jim Porter <jporterbugs@gmail.com>]

On 12/1/2022 9:08 AM, Eli Zaretskii wrote:
>> Date: Mon, 28 Nov 2022 20:23:17 -0800
>> From: Jim Porter <jporterbugs@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.