Subject: Re: bug#77834: [PATCH] Improve help-fns-edit-variable for Lisp editing

From: Philip Kaludercic <[email-masked]>
Date: Thu, 17 Apr 2025 08:04:46 +0000

Spencer Baugh <[email-masked]> writes:

> Eli Zaretskii <[email-masked]> writes:
>>> Cc: [email-masked], Philip Kaludercic <[email-masked]>,
>>>  Lars Ingebrigtsen <[email-masked]>
>>> Date: Tue, 15 Apr 2025 17:18:37 -0400
>>> From:  Spencer Baugh via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <[email-masked]>
>>> 
>>> Before d50c82f3e98e ("Simplify
>>> 'help-enable-variable-value-editing' using 'string-edit'"),
>>> help-fns-edit-variable would open a buffer in emacs-lisp-mode
>>> and would not allow exiting that buffer with an invalid Lisp
>>> expression.  Restore that functionality by enhancing string-edit
>>> to allow choosing a major mode and allow passing a function to
>>> validate the buffer contents before returning.
>>
>> Why is it better to use emacs-lisp-mode in this case? what are the
>> advantages and disadvantages?
>
> It provides syntax highlighting and completion and all the other
> features of emacs-lisp-mode.  We're editing a Lisp form after all.
>
>> Philip, was this side-effect of commit d50c82f3e98e intentional or
>> just an oversight?  Maybe we should simply revert that commit instead?

It was an oversight.

> I don't think we should revert the commit - I think using string-edit is
> a good change since it's almost exactly what we want for
> help-fns-edit-variable.
>
> Plus, the new functionality I'm adding to string-edit is useful in other
> places too - I plan to also use it for other later changes.

I agree, this is an improvement we should make to string-edit regardless
of `help-fns-edit-variable'.

>>>  (cl-defun string-edit (prompt string success-callback
>>> -                              &key abort-callback)
>>> +                              &key abort-callback major-mode read)
>>>    "Switch to a new buffer to edit STRING.
>>> -When the user finishes editing (with \\<string-edit-mode-map>\\[string-edit-done]), SUCCESS-CALLBACK
>>> -is called with the resulting string.
>>> +When the user finishes editing (with `string-edit-done'),
>>> +SUCCESS-CALLBACK is called with the resulting string.  If READ is
>>> +non-nil, it is called on the string and its result is passed to
>>> +SUCCESS-CALLBACK instead.  If READ signals an error, the buffer remains
>>> +open, giving the user a chance to correct a typo.
>>>  
>>> -If the user aborts (with \\<string-edit-mode-map>\\[string-edit-abort]), ABORT-CALLBACK (if any) is
>>> +If the user aborts (with `string-edit-abort'), ABORT-CALLBACK (if any) is
>>>  called with no parameters.
>>>  
>>>  PROMPT will be inserted at the start of the buffer, but won't be
>>>  included in the resulting string.  If PROMPT is nil, no help text
>>>  will be inserted.
>>>  
>>> +MAJOR-MODE is called to set up the new buffer; if not passed,
>>> +`string-edit-mode' is used.
>>
>> This doc string, both its old parts and the new additions, are
>> replete with unnecessary uses of passive tense; please fix that as
>> part of the changeset.
>
> Will do so in the next version of the patch.
>
>>>  ;;;###autoload
>>> -(defun read-string-from-buffer (prompt string)
>>> +(cl-defun read-string-from-buffer (prompt string
>>> +                                          &key major-mode
>>> +                                          read)
>>
>> I don't see any reasons to make this a cl-defun, just to add 2 more
>> optional arguments.
>
> It's to match the existing cl-defun for string-edit.  string-edit takes
> keyword arguments - read-string-from-buffer probably should match it.
>
> Alternatively, I could just not change read-string-from-buffer and only
> change string-edit.  Then help-fns-edit-variable could call string-edit
> directly.  That would duplicate a bit more code though.
>
>>>    "Switch to a new buffer to edit STRING in a recursive edit.
>>> -The user finishes editing with \\<string-edit-mode-map>\\[string-edit-done], or aborts with \\<string-edit-mode-map>\\[string-edit-abort]).
>>> +
>>> +When the user finishes editing with `string-edit-done', this function
>>
>> Why did you remove the keymap and command markup?  That loses useful
>> features.
>>
>>> +If the user aborts with `string-edit-abort', this function signals an
>>> +error.
>>
>> Same here.
>
> I removed these because neither of these are user-facing functions; the
> docstrings are only read by Lisp hackers writing new packages using
> these functions.  And it seems to me that for a Lisp programmer it's
> more useful to know the name of the commands which the user uses to
> finish editing, rather than the bindings for those commands.
>
> But this is not an important change, if you prefer it to have the keymap
> markup instead.