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

[Jim Porter]

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.