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

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 77834 in the body.
You can then email your comments to 77834 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: Spencer Baugh <sbaugh <at> janestreet.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Improve help-fns-edit-variable for Lisp editing
Date: Tue, 15 Apr 2025 17:18:37 -0400

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

Tags: patch


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.

In GNU Emacs 30.1.50 (build 8, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2025-04-10 built on
 igm-qws-u22796a
Repository revision: 128bc06bfcc56a35d6b5555cc546faaf0d964df0
Repository branch: emacs-30
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --config-cache --with-x-toolkit=lucid --without-gpm
 --without-gconf --without-selinux --without-imagemagick --with-modules
 --with-gif=no --with-cairo --with-rsvg --without-compress-install
 --with-tree-sitter --with-native-compilation=aot'

[0001-Improve-help-fns-edit-variable-for-Lisp-editing.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
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 11:01:12 +0300

> 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?

Philip, was this side-effect of commit d50c82f3e98e intentional or
just an oversight?  Maybe we should simply revert that commit instead?

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

>  ;;;###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.

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

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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
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 20:00:54 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 77834 <at> debbugs.gnu.org,  larsi <at> gnus.org,  philipk <at> posteo.net,
>    azeng <at> janestreet.com
> Date: Wed, 16 Apr 2025 12:29:26 -0400
> 
> >> -(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.

But read-string-from-buffer is a public function, so any incompatible
changes in it should be avoided.

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

I think this will be better.  string-edit is already a cl-defun, so we
just adding new optional arguments to it.  But we also need a default
for the :read argument if it is omitted.

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

The original text shows both the command name and its binding.

> But this is not an important change, if you prefer it to have the keymap
> markup instead.

I'd prefer not to lose the information, indeed.

Thanks.

Message #17 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 13:31:39 -0400

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: 77834 <at> debbugs.gnu.org,  larsi <at> gnus.org,  philipk <at> posteo.net,
>>    azeng <at> janestreet.com
>> Date: Wed, 16 Apr 2025 12:29:26 -0400
>> 
>> >> -(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.
>
> But read-string-from-buffer is a public function, so any incompatible
> changes in it should be avoided.
>
>> 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.
>
> I think this will be better.  string-edit is already a cl-defun, so we
> just adding new optional arguments to it.

OK, reverted the changes to read-string-from-buffer and switched
help-fns-edit-variable to call string-edit directly.

> But we also need a default for the :read argument if it is omitted.

Good catch, done.

>> >>    "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.
>
> The original text shows both the command name and its binding.
>
>> But this is not an important change, if you prefer it to have the keymap
>> markup instead.
>
> I'd prefer not to lose the information, indeed.

OK, reverted.

Also updated the docstrings to not use passive voice.

[0001-Improve-help-fns-edit-variable-for-Lisp-editing.patch (text/x-patch, inline)]

From 6ea71c8f516c6cc9ae3ec38dc8a331fd17afc3d3 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Tue, 15 Apr 2025 17:17:27 -0400
Subject: [PATCH] Improve help-fns-edit-variable for Lisp editing

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.

* lisp/help-fns.el (help-fns-edit-variable): Call string-edit,
passing emacs-lisp-mode and read.
* lisp/textmodes/string-edit.el (string-edit--read): Add.
(string-edit): Add :major-mode and :read arguments and avoid
passive voice.
(read-string-from-buffer): Avoid passive voice in docs.
(string-edit-mode-map, string-edit-minor-mode-map)
(string-edit-mode, string-edit-minor-mode): Move string-edit
keybindings to a minor mode.
(string-edit-done): Call string-edit--read before exiting.
---
 lisp/help-fns.el              | 19 ++++++++---
 lisp/textmodes/string-edit.el | 62 +++++++++++++++++++++--------------
 2 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 0eb0a7a40be..7a0bc2908e9 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1561,11 +1561,20 @@ help-fns-edit-variable
   (let ((var (get-text-property (point) 'help-fns--edit-variable)))
     (unless var
       (error "No variable under point"))
-    (let ((str (read-string-from-buffer
-                (format ";; Edit the `%s' variable." (nth 0 var))
-                (prin1-to-string (nth 1 var)))))
-      (set (nth 0 var) (read str))
-      (revert-buffer))))
+    (string-edit
+     (format ";; Edit the `%s' variable." (nth 0 var))
+     (prin1-to-string (nth 1 var))
+     (lambda (edited)
+       (set (nth 0 var) edited)
+       (exit-recursive-edit))
+     :abort-callback
+     (lambda ()
+       (exit-recursive-edit)
+       (error "Aborted edit, variable unchanged"))
+     :major-mode #'emacs-lisp-mode
+     :read #'read)
+    (recursive-edit)
+    (revert-buffer)))
 
 (autoload 'shortdoc-help-fns-examples-function "shortdoc")
 
diff --git a/lisp/textmodes/string-edit.el b/lisp/textmodes/string-edit.el
index 3c76db202c7..c8fdffe5f4c 100644
--- a/lisp/textmodes/string-edit.el
+++ b/lisp/textmodes/string-edit.el
@@ -33,20 +33,25 @@ string-edit-prompt
 
 (defvar string-edit--success-callback)
 (defvar string-edit--abort-callback)
+(defvar string-edit--read)
 
 ;;;###autoload
 (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.
 
-If the user aborts (with \\<string-edit-mode-map>\\[string-edit-abort]), ABORT-CALLBACK (if any) is
-called with no parameters.
+Call MAJOR-MODE (defaulting to `string-edit-mode') to set up the new
+buffer, and insert PROMPT (defaulting to nothing) at the start of the
+buffer.
 
-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.
+When the user finishes editing (with \\<string-edit-minor-mode-map>\\[string-edit-done]), call
+READ (defaulting to `identity') on the resulting string, omitting PROMPT if any.
+
+If READ returns without an error, quit the buffer and call
+SUCCESS-CALLBACK on the result.
+
+If the user aborts (with \\<string-edit-minor-mode-map>\\[string-edit-abort]),
+call ABORT-CALLBACK (if any) with no parameters.
 
 Also see `read-string-from-buffer'."
   (with-current-buffer (generate-new-buffer "*edit string*")
@@ -74,26 +79,27 @@ string-edit
 
     (set-buffer-modified-p nil)
     (setq buffer-undo-list nil)
-    (string-edit-mode)
+    (funcall (or major-mode #'string-edit-mode))
+    (string-edit-minor-mode)
     (setq-local string-edit--success-callback success-callback)
     (setq-local string-edit--abort-callback abort-callback)
+    (setq-local string-edit--read read)
     (setq-local header-line-format
                 (substitute-command-keys
-                 "Type \\<string-edit-mode-map>\\[string-edit-done] when you've finished editing or \\[string-edit-abort] to abort"))
+                 "Type \\<string-edit-minor-mode-map>\\[string-edit-done] when you've finished editing or \\[string-edit-abort] to abort"))
     (message "%s" (substitute-command-keys
-                   "Type \\<string-edit-mode-map>\\[string-edit-done] when you've finished editing"))))
+                   "Type \\<string-edit-minor-mode-map>\\[string-edit-done] when you've finished editing"))))
 
 ;;;###autoload
 (defun read-string-from-buffer (prompt string)
   "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]).
 
-PROMPT will be inserted at the start of the buffer, but won't be
-included in the resulting string.  If nil, no prompt will be
-inserted in the buffer.
+Insert PROMPT at the start of the buffer.  If nil, no prompt is
+inserted.
 
-When the user exits recursive edit, this function returns the
-edited STRING.
+When the user exits recursive edit, return the contents of the
+buffer (without including PROMPT).
 
 Also see `string-edit'."
   (string-edit
@@ -108,11 +114,16 @@ read-string-from-buffer
   (recursive-edit)
   string)
 
-(defvar-keymap string-edit-mode-map
+(defvar-keymap string-edit-minor-mode-map
   "C-c C-c" #'string-edit-done
   "C-c C-k" #'string-edit-abort)
 
-(define-derived-mode string-edit-mode text-mode "String"
+(define-minor-mode string-edit-minor-mode
+  "Minor mode for editing strings"
+  :lighter "String"
+  :interactive nil)
+
+(define-derived-mode string-edit-mode text-mode "Text"
   "Mode for editing strings."
   :interactive nil)
 
@@ -120,13 +131,16 @@ string-edit-done
   "Finish editing the string and call the callback function.
 This will kill the current buffer."
   (interactive)
-  (goto-char (point-min))
-  ;; Skip past the help text.
-  (text-property-search-forward 'string-edit--prompt)
-  (let ((string (buffer-substring (point) (point-max)))
-        (callback string-edit--success-callback))
+  (let* ((string
+          (save-excursion
+            (goto-char (point-min))
+            ;; Skip past the help text.
+            (text-property-search-forward 'string-edit--prompt)
+            (buffer-substring (point) (point-max))))
+         (valid (funcall (or string-edit--read #'identity) string))
+         (callback string-edit--success-callback))
     (quit-window 'kill)
-    (funcall callback string)))
+    (funcall callback valid)))
 
 (defun string-edit-abort ()
   "Abort editing the current string."
-- 
2.39.3

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77834 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org,
 azeng <at> janestreet.com
Subject: Re: bug#77834: [PATCH] Improve help-fns-edit-variable for Lisp editing
Date: Thu, 17 Apr 2025 08:04:46 +0000

Spencer Baugh <sbaugh <at> janestreet.com> writes:

> 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?

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.

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77834 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, larsi <at> gnus.org,
 azeng <at> janestreet.com
Subject: Re: bug#77834: [PATCH] Improve help-fns-edit-variable for Lisp editing
Date: Thu, 17 Apr 2025 08:05:54 +0000

Spencer Baugh <sbaugh <at> janestreet.com> writes:


[...]

> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
> index 0eb0a7a40be..7a0bc2908e9 100644
> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -1561,11 +1561,20 @@ help-fns-edit-variable
>    (let ((var (get-text-property (point) 'help-fns--edit-variable)))
>      (unless var
>        (error "No variable under point"))
> -    (let ((str (read-string-from-buffer
> -                (format ";; Edit the `%s' variable." (nth 0 var))
> -                (prin1-to-string (nth 1 var)))))
> -      (set (nth 0 var) (read str))
> -      (revert-buffer))))
> +    (string-edit
> +     (format ";; Edit the `%s' variable." (nth 0 var))
> +     (prin1-to-string (nth 1 var))
> +     (lambda (edited)
> +       (set (nth 0 var) edited)
> +       (exit-recursive-edit))
> +     :abort-callback
> +     (lambda ()
> +       (exit-recursive-edit)
> +       (error "Aborted edit, variable unchanged"))
> +     :major-mode #'emacs-lisp-mode
> +     :read #'read)
> +    (recursive-edit)
> +    (revert-buffer)))

I don't remember the details here anymore, but why do you need the
`recursive-edit' and `exit-recursive-edit'?  Shouldn't `string-edit'
take care of this instead of the caller?

[...]

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

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

Philip Kaludercic <philipk <at> posteo.net> writes:

> Spencer Baugh <sbaugh <at> janestreet.com> writes:
>
>
> [...]
>
>> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
>> index 0eb0a7a40be..7a0bc2908e9 100644
>> --- a/lisp/help-fns.el
>> +++ b/lisp/help-fns.el
>> @@ -1561,11 +1561,20 @@ help-fns-edit-variable
>>    (let ((var (get-text-property (point) 'help-fns--edit-variable)))
>>      (unless var
>>        (error "No variable under point"))
>> -    (let ((str (read-string-from-buffer
>> -                (format ";; Edit the `%s' variable." (nth 0 var))
>> -                (prin1-to-string (nth 1 var)))))
>> -      (set (nth 0 var) (read str))
>> -      (revert-buffer))))
>> +    (string-edit
>> +     (format ";; Edit the `%s' variable." (nth 0 var))
>> +     (prin1-to-string (nth 1 var))
>> +     (lambda (edited)
>> +       (set (nth 0 var) edited)
>> +       (exit-recursive-edit))
>> +     :abort-callback
>> +     (lambda ()
>> +       (exit-recursive-edit)
>> +       (error "Aborted edit, variable unchanged"))
>> +     :major-mode #'emacs-lisp-mode
>> +     :read #'read)
>> +    (recursive-edit)
>> +    (revert-buffer)))
>
> I don't remember the details here anymore, but why do you need the
> `recursive-edit' and `exit-recursive-edit'?  Shouldn't `string-edit'
> take care of this instead of the caller?
>
> [...]

No - read-string-from-buffer takes care of those, but string-edit does
not.

I stopped using read-string-from-buffer on a suggestion from Eli.  We
could add some new function "string-edit-recursive" which handles the
recursive-edit details.

However: one nice property of using string-edit with its callbacks
directly is that if the user doesn't explicitly call string-edit-done,
then SUCCESS-CALLBACK isn't called, so (as I've written the code in my
patch) the variable's value is not set.

When using read-string-from-buffer, if the user manually exits the
recursive edit, read-string-from-buffer returns the original string, so
the variable would be unnecessarily set back to its original value, even
if it was changed by something else in the meantime.  For example, this
could happen if the user forgot about the string-edit buffer, changed
the variable some other way, and then later noticed they were in a
recursive-edit and ran exit-recursive-edit manually.  My code using
string-edit won't have this problem.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77834-done <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: Sat, 26 Apr 2025 14:55:46 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 77834 <at> debbugs.gnu.org,  larsi <at> gnus.org,  philipk <at> posteo.net,
>    azeng <at> janestreet.com
> Date: Wed, 16 Apr 2025 13:31:39 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh <at> janestreet.com>
> >> Cc: 77834 <at> debbugs.gnu.org,  larsi <at> gnus.org,  philipk <at> posteo.net,
> >>    azeng <at> janestreet.com
> >> Date: Wed, 16 Apr 2025 12:29:26 -0400
> >> 
> >> >> -(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.
> >
> > But read-string-from-buffer is a public function, so any incompatible
> > changes in it should be avoided.
> >
> >> 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.
> >
> > I think this will be better.  string-edit is already a cl-defun, so we
> > just adding new optional arguments to it.
> 
> OK, reverted the changes to read-string-from-buffer and switched
> help-fns-edit-variable to call string-edit directly.
> 
> > But we also need a default for the :read argument if it is omitted.
> 
> Good catch, done.
> 
> >> >>    "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.
> >
> > The original text shows both the command name and its binding.
> >
> >> But this is not an important change, if you prefer it to have the keymap
> >> markup instead.
> >
> > I'd prefer not to lose the information, indeed.
> 
> OK, reverted.
> 
> Also updated the docstrings to not use passive voice.

Thanks, now installed on the master branch, and closing the bug.

Previous Next

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