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

From: Spencer Baugh <[email-masked]>
Date: Wed, 16 Apr 2025 13:31:39 -0400

Eli Zaretskii <[email-masked]> writes:

>> From: Spencer Baugh <[email-masked]>
>> Cc: [email-masked],  [email-masked],  [email-masked],
>>    [email-masked]
>> 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.

>From 6ea71c8f516c6cc9ae3ec38dc8a331fd17afc3d3 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <[email-masked]>
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