Package: emacs;
Reported by: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Tue, 15 Apr 2025 21:20:02 UTC
Severity: normal
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
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: 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.