bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]


The main issue with this patch is that it drops the syntax highlighting
in the *string edit* buffer, but we could fix that in a second patch by
adding support to specify a major mode to inherit from when editing a
string.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Simplify 'help-enable-variable-value-editing' using --]
[-- Type: text/x-patch, Size: 3546 bytes --]

From fa1fff88e295bee0380665d623a6266842f91af6 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 5 Jun 2024 08:26:37 +0200
Subject: [PATCH] Simplify 'help-enable-variable-value-editing' using
 'string-edit'

* lisp/help-fns.el (help-fns--edit-variable): Remove variable
declaration, as 'string-edit' makes manually storing the
variable name unnecessary.
(help-fns-edit-variable): Use 'read-string-from-buffer' instead
of custom major mode.
(help-fns--edit-value-mode-map, help-fns--edit-value-mode)
(help-fns-edit-mode-done, help-fns-edit-mode-cancel):  Remove
functionality provided by 'string-edit'.
---
 lisp/help-fns.el | 55 ++++++------------------------------------------
 1 file changed, 7 insertions(+), 48 deletions(-)

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 1ffe1b16588..8ea5b301684 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1502,60 +1502,19 @@ help-fns--editable-variable
                      :parent button-map
                      "e" #'help-fns-edit-variable)))))
 
-(defvar help-fns--edit-variable)
-
 (put 'help-fns-edit-variable 'disabled t)
 (defun help-fns-edit-variable ()
   "Edit the variable under point."
   (declare (completion ignore))
   (interactive)
-  (let ((var (get-text-property (point) 'help-fns--edit-variable)))
-    (unless var
+  (let* ((val (thing-at-point 'sexp))
+         (var (get-text-property 0 'help-fns--edit-variable val)))
+    (unless val
       (error "No variable under point"))
-    (pop-to-buffer-same-window (format "*edit %s*" (nth 0 var)))
-    (prin1 (nth 1 var) (current-buffer))
-    (pp-buffer)
-    (goto-char (point-min))
-    (help-fns--edit-value-mode)
-    (insert (format ";; Edit the `%s' variable.\n" (nth 0 var))
-            (substitute-command-keys
-             ";; `\\[help-fns-edit-mode-done]' to update the value and exit; \
-`\\[help-fns-edit-mode-cancel]' to cancel.\n\n"))
-    (setq-local help-fns--edit-variable var)))
-
-(defvar-keymap help-fns--edit-value-mode-map
-  "C-c C-c" #'help-fns-edit-mode-done
-  "C-c C-k" #'help-fns-edit-mode-cancel)
-
-(define-derived-mode help-fns--edit-value-mode emacs-lisp-mode "Elisp"
-  :interactive nil)
-
-(defun help-fns-edit-mode-done (&optional kill)
-  "Update the value of the variable being edited and kill the edit buffer.
-If KILL (the prefix), don't update the value, but just kill the
-current buffer."
-  (interactive "P" help-fns--edit-value-mode)
-  (unless help-fns--edit-variable
-    (error "Invalid buffer"))
-  (goto-char (point-min))
-  (cl-destructuring-bind (variable _ buffer help-buffer)
-      help-fns--edit-variable
-    (unless (buffer-live-p buffer)
-      (error "Original buffer is gone; can't update"))
-    (unless kill
-      (let ((value (read (current-buffer))))
-        (with-current-buffer buffer
-          (set variable value))))
-    (kill-buffer (current-buffer))
-    (when (buffer-live-p help-buffer)
-      (with-current-buffer help-buffer
-        (revert-buffer)))))
-
-(defun help-fns-edit-mode-cancel ()
-  "Kill the edit buffer and cancel editing of the value.
-This cancels value editing without updating the value."
-  (interactive nil help-fns--edit-value-mode)
-  (help-fns-edit-mode-done t))
+    (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)))))
 
 (defun help-fns--run-describe-functions (functions &rest args)
   (with-current-buffer standard-output
-- 
2.45.1


[-- Attachment #3: Type: text/plain, Size: 37 bytes --]


-- 
	Philip Kaludercic on peregrine

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Wed, 05 Jun 2024 06:55:46 +0000
> 
> The main issue with this patch is that it drops the syntax highlighting
> in the *string edit* buffer, but we could fix that in a second patch by
> adding support to specify a major mode to inherit from when editing a
> string.

Thanks.  This is basically a cleanup, yes?  IOW, there's nothing wrong
with the current implementation, right?  If so, I'd prefer to wait
with this until after the emacs-30 branch is cut.

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Wed, 05 Jun 2024 06:55:46 +0000
>> 
>> The main issue with this patch is that it drops the syntax highlighting
>> in the *string edit* buffer, but we could fix that in a second patch by
>> adding support to specify a major mode to inherit from when editing a
>> string.
>
> Thanks.  This is basically a cleanup, yes?  IOW, there's nothing wrong
> with the current implementation, right?  If so, I'd prefer to wait
> with this until after the emacs-30 branch is cut.

Right, it just seemed like the kind of thing where it would be natural
to demonstrate string-edit.

-- 
	Philip Kaludercic on peregrine

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Stefan Kangas]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Date: Wed, 05 Jun 2024 06:55:46 +0000
>>>
>>> The main issue with this patch is that it drops the syntax highlighting
>>> in the *string edit* buffer, but we could fix that in a second patch by
>>> adding support to specify a major mode to inherit from when editing a
>>> string.
>>
>> Thanks.  This is basically a cleanup, yes?  IOW, there's nothing wrong
>> with the current implementation, right?  If so, I'd prefer to wait
>> with this until after the emacs-30 branch is cut.
>
> Right, it just seemed like the kind of thing where it would be natural
> to demonstrate string-edit.

Feel free to install on master now, thanks.

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Jeremy Bryant via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>> Date: Wed, 05 Jun 2024 06:55:46 +0000
>>>>
>>>> The main issue with this patch is that it drops the syntax highlighting
>>>> in the *string edit* buffer, but we could fix that in a second patch by
>>>> adding support to specify a major mode to inherit from when editing a
>>>> string.
>>>
>>> Thanks.  This is basically a cleanup, yes?  IOW, there's nothing wrong
>>> with the current implementation, right?  If so, I'd prefer to wait
>>> with this until after the emacs-30 branch is cut.
>>
>> Right, it just seemed like the kind of thing where it would be natural
>> to demonstrate string-edit.
>
> Feel free to install on master now, thanks.

Philip, has this been installed on master?  Can the bug be closed?

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

Jeremy Bryant <jb@jeremybryant.net> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>>> Date: Wed, 05 Jun 2024 06:55:46 +0000
>>>>>
>>>>> The main issue with this patch is that it drops the syntax highlighting
>>>>> in the *string edit* buffer, but we could fix that in a second patch by
>>>>> adding support to specify a major mode to inherit from when editing a
>>>>> string.
>>>>
>>>> Thanks.  This is basically a cleanup, yes?  IOW, there's nothing wrong
>>>> with the current implementation, right?  If so, I'd prefer to wait
>>>> with this until after the emacs-30 branch is cut.
>>>
>>> Right, it just seemed like the kind of thing where it would be natural
>>> to demonstrate string-edit.
>>
>> Feel free to install on master now, thanks.
>
> Philip, has this been installed on master?  Can the bug be closed?

I hadn't pushed the change, have done so now.  Closing the report.
Thanks for the reminder.

-- 
	Philip Kaludercic on peregrine

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

reopen 71374
quit

Hi Philip,

Philip Kaludercic <philipk@posteo.net> writes:

> I hadn't pushed the change, have done so now.  Closing the report.
> Thanks for the reminder.

I'm reopening this (hope you don't mind) to keep track of the minor
regressions I mentioned here:

https://lists.gnu.org/archive/html/emacs-devel/2024-07/msg01202.html


Thanks,

Eshel

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

Eshel Yaron <me@eshelyaron.com> writes:

> reopen 71374
> quit
>
> Hi Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> I hadn't pushed the change, have done so now.  Closing the report.
>> Thanks for the reminder.
>
> I'm reopening this (hope you don't mind) to keep track of the minor
> regressions I mentioned here:
>
> https://lists.gnu.org/archive/html/emacs-devel/2024-07/msg01202.html

Thanks, and my apologies for the late reply.

The issue seems to be that (thing-at-point 'sexp) returns nil if inside
a s-expression looking at whitespace:

(1 2     3 4 5)
     ^
     M-: (thing-at-point 'sexp) ;=> nil

One idea is to fall back on a defun at point, if no sexp is found:


[-- Attachment #2: Type: text/plain, Size: 1047 bytes --]

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 8ea5b301684..4d55ea8bbff 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1507,13 +1507,14 @@ help-fns-edit-variable
   "Edit the variable under point."
   (declare (completion ignore))
   (interactive)
-  (let* ((val (thing-at-point 'sexp))
-         (var (get-text-property 0 'help-fns--edit-variable val)))
+  (let ((val (or (thing-at-point 'sexp)
+                 (thing-at-point 'defun))))
     (unless val
-      (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)))))
+      (error "No value at point"))
+    (let* ((var (get-text-property 0 'help-fns--edit-variable val))
+           (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)))))
 
 (defun help-fns--run-describe-functions (functions &rest args)

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]


Additionally, I moved the `get-text-property' after the check, to ensure
that the indented error message is used.

Comments?

>
> Thanks,
>
> Eshel

-- 
	Philip Kaludercic on peregrine

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>> I'm reopening this (hope you don't mind) to keep track of the minor
>> regressions I mentioned here:
>>
>> https://lists.gnu.org/archive/html/emacs-devel/2024-07/msg01202.html
>
> Thanks, and my apologies for the late reply.

No problem at all.

> The issue seems to be that (thing-at-point 'sexp) returns nil if inside
> a s-expression looking at whitespace:
>
> (1 2     3 4 5)
>      ^
>      M-: (thing-at-point 'sexp) ;=> nil
>
> One idea is to fall back on a defun at point, if no sexp is found:
>
> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
> index 8ea5b301684..4d55ea8bbff 100644
> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -1507,13 +1507,14 @@ help-fns-edit-variable
>    "Edit the variable under point."
>    (declare (completion ignore))
>    (interactive)
> -  (let* ((val (thing-at-point 'sexp))
> -         (var (get-text-property 0 'help-fns--edit-variable val)))
> +  (let ((val (or (thing-at-point 'sexp)
> +                 (thing-at-point 'defun))))
>      (unless val
> -      (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)))))
> +      (error "No value at point"))
> +    (let* ((var (get-text-property 0 'help-fns--edit-variable val))
> +           (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)))))

Maybe I'm missing something, but why are val and the thing-at-point call
needed at all?  Simply grabbing the help-fns--edit-variable property at
point should be work as well, I think.


Eshel

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

Eshel Yaron <me@eshelyaron.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eshel Yaron <me@eshelyaron.com> writes:
>>
>>> I'm reopening this (hope you don't mind) to keep track of the minor
>>> regressions I mentioned here:
>>>
>>> https://lists.gnu.org/archive/html/emacs-devel/2024-07/msg01202.html
>>
>> Thanks, and my apologies for the late reply.
>
> No problem at all.
>
>> The issue seems to be that (thing-at-point 'sexp) returns nil if inside
>> a s-expression looking at whitespace:
>>
>> (1 2     3 4 5)
>>      ^
>>      M-: (thing-at-point 'sexp) ;=> nil
>>
>> One idea is to fall back on a defun at point, if no sexp is found:
>>
>> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
>> index 8ea5b301684..4d55ea8bbff 100644
>> --- a/lisp/help-fns.el
>> +++ b/lisp/help-fns.el
>> @@ -1507,13 +1507,14 @@ help-fns-edit-variable
>>    "Edit the variable under point."
>>    (declare (completion ignore))
>>    (interactive)
>> -  (let* ((val (thing-at-point 'sexp))
>> -         (var (get-text-property 0 'help-fns--edit-variable val)))
>> +  (let ((val (or (thing-at-point 'sexp)
>> +                 (thing-at-point 'defun))))
>>      (unless val
>> -      (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)))))
>> +      (error "No value at point"))
>> +    (let* ((var (get-text-property 0 'help-fns--edit-variable val))
>> +           (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)))))
>
> Maybe I'm missing something, but why are val and the thing-at-point call
> needed at all?  Simply grabbing the help-fns--edit-variable property at
> point should be work as well, I think.

No, I think you are right, we don't have to take the detour over
'thing-at-point', meaning that if I am not mistaken, the fix should
reduce itself to:


[-- Attachment #2: Type: text/plain, Size: 624 bytes --]

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 8a2ae79736f..97d054ce6db 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1507,9 +1507,8 @@ help-fns-edit-variable
   "Edit the variable under point."
   (declare (completion ignore))
   (interactive)
-  (let* ((val (thing-at-point 'sexp))
-         (var (get-text-property 0 'help-fns--edit-variable val)))
-    (unless val
+  (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))

[-- Attachment #3: Type: text/plain, Size: 47 bytes --]



> Eshel

-- 
	Philip Kaludercic on peregrine

bug#71374: [PATCH] Simplify 'help-enable-variable-value-editing' using

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Eshel Yaron <me@eshelyaron.com> writes:
>>>
>>>> I'm reopening this (hope you don't mind) to keep track of the minor
>>>> regressions I mentioned here:
>>>>
>>>> https://lists.gnu.org/archive/html/emacs-devel/2024-07/msg01202.html
>>>
>>> Thanks, and my apologies for the late reply.
>>
>> No problem at all.
>>
>>> The issue seems to be that (thing-at-point 'sexp) returns nil if inside
>>> a s-expression looking at whitespace:
>>>
>>> (1 2     3 4 5)
>>>      ^
>>>      M-: (thing-at-point 'sexp) ;=> nil
>>>
>>> One idea is to fall back on a defun at point, if no sexp is found:
>>>
>>> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
>>> index 8ea5b301684..4d55ea8bbff 100644
>>> --- a/lisp/help-fns.el
>>> +++ b/lisp/help-fns.el
>>> @@ -1507,13 +1507,14 @@ help-fns-edit-variable
>>>    "Edit the variable under point."
>>>    (declare (completion ignore))
>>>    (interactive)
>>> -  (let* ((val (thing-at-point 'sexp))
>>> -         (var (get-text-property 0 'help-fns--edit-variable val)))
>>> +  (let ((val (or (thing-at-point 'sexp)
>>> +                 (thing-at-point 'defun))))
>>>      (unless val
>>> -      (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)))))
>>> +      (error "No value at point"))
>>> +    (let* ((var (get-text-property 0 'help-fns--edit-variable val))
>>> +           (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)))))
>>
>> Maybe I'm missing something, but why are val and the thing-at-point call
>> needed at all?  Simply grabbing the help-fns--edit-variable property at
>> point should be work as well, I think.
>
> No, I think you are right, we don't have to take the detour over
> 'thing-at-point', meaning that if I am not mistaken, the fix should
> reduce itself to:
>
> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
> index 8a2ae79736f..97d054ce6db 100644
> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -1507,9 +1507,8 @@ help-fns-edit-variable
>    "Edit the variable under point."
>    (declare (completion ignore))
>    (interactive)
> -  (let* ((val (thing-at-point 'sexp))
> -         (var (get-text-property 0 'help-fns--edit-variable val)))
> -    (unless val
> +  (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))

I have pushed this change and am closing the report.

>> Eshel

-- 
	Philip Kaludercic on peregrine