GNU bug report logs - #77834
[PATCH] Improve help-fns-edit-variable for Lisp editing

Previous Next

Full log

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77834 <at> debbugs.gnu.org, larsi <at> gnus.org, philipk <at> posteo.net,
 azeng <at> janestreet.com
Subject: Re: bug#77834: [PATCH] Improve help-fns-edit-variable for Lisp editing
Date: Wed, 16 Apr 2025 12:29:26 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> Cc: azeng <at> janestreet.com, Philip Kaludercic <philipk <at> posteo.net>,
>>  Lars Ingebrigtsen <larsi <at> gnus.org>
>> 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" <bug-gnu-emacs <at> gnu.org>
>> 
>> 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?

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.

>>  (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.

Previous Next

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