save-excursion doesn't restore point with json-pretty-print

save-excursion doesn't restore point with json-pretty-print

[Tassilo Horn]

Hi all,

I have this small command in my ~/.emacs:

--8<---------------cut here---------------start------------->8---
(defun th/json-pretty-print-snippet-at-point ()
  "Pretty-print the json snippet at point."
  (interactive)
  (save-excursion
    (when (beginning-of-defun)
      (let ((beg (point)))
	(end-of-defun)
	(when (looking-back "\n")
	  (backward-char))
	(json-pretty-print beg (point))))))
--8<---------------cut here---------------end--------------->8---

It works pretty well except that point is not restored.  No matter where
I execute it, after reformatting the snippet, point will sit on the
character where `beginning-of-defun' moved it.

For example, when point is on the 1 below,

{"foo": 1, "bar":27}

I'll end up with

{
  "foo": 1,
  "bar": 27
}

and point is on the {.  I'd expect it to still sit on the 1.

Obviously, `save-excursion' works just fine in all other places.  It
seems like it's just not playing well with `json-pretty-print', and I
don't know why.

Bye,
Tassilo

Re: save-excursion doesn't restore point with json-pretty-print

[tomas]

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

On Fri, Feb 01, 2019 at 10:32:20AM +0100, Tassilo Horn wrote:
> Hi all,
> 
> I have this small command in my ~/.emacs:

[...]

> For example, when point is on the 1 below,
> 
> {"foo": 1, "bar":27}
> 
> I'll end up with
> 
> {
>   "foo": 1,
>   "bar": 27
> }
> 
> and point is on the {.  I'd expect it to still sit on the 1.
> 
> Obviously, `save-excursion' works just fine in all other places.  It
> seems like it's just not playing well with `json-pretty-print', and I
> don't know why.

AFAIK the mechanism for save-excursion is to set a special marker
to return to after the excursion.

Now the pretty print probably replaces the whole region in the
process: the marker can't be at the same place where it was. If
you're lucky, it'll be at one of both region's borders.

A more careful pretty-print process which is capable of "floating"
existing markers around sounds like an interesting challenge :-)

(Note that this is just armchair analysis, and I could be totally
wrong).

Cheers
-- tomás

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: save-excursion doesn't restore point with json-pretty-print

[Eli Zaretskii]

> Date: Fri, 1 Feb 2019 10:55:16 +0100
> From: <tomas@tuxteam.de>
> 
> AFAIK the mechanism for save-excursion is to set a special marker
> to return to after the excursion.
> 
> Now the pretty print probably replaces the whole region in the
> process: the marker can't be at the same place where it was. If
> you're lucky, it'll be at one of both region's borders.

Yes, erasing the buffer makes all the markers point to BOB, and then
save-excursion won't work as expected.

Perhaps json-pretty-print could use the new replace-buffer-contents
function?  Although I'm not sure this will cure the problem, at least
not in all cases.

Re: save-excursion doesn't restore point with json-pretty-print

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 1 Feb 2019 10:55:16 +0100
>> From: <tomas@tuxteam.de>
>> 
>> AFAIK the mechanism for save-excursion is to set a special marker
>> to return to after the excursion.
>> 
>> Now the pretty print probably replaces the whole region in the
>> process: the marker can't be at the same place where it was. If
>> you're lucky, it'll be at one of both region's borders.
>
> Yes, erasing the buffer makes all the markers point to BOB, and then
> save-excursion won't work as expected.
>
> Perhaps json-pretty-print could use the new replace-buffer-contents
> function?  Although I'm not sure this will cure the problem, at least
> not in all cases.

replace-buffer-contents would be a lot easier to use in this context
if it could take a string. Otherwise you have to create a buffer,
select it, insert a string, then pass that buffer to
replace-buffer-contents, delete the buffer.

Robert

Re: save-excursion doesn't restore point with json-pretty-print

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: help-gnu-emacs@gnu.org
> Date: Fri, 01 Feb 2019 11:33:23 +0100
> 
> > Perhaps json-pretty-print could use the new replace-buffer-contents
> > function?  Although I'm not sure this will cure the problem, at least
> > not in all cases.
> 
> replace-buffer-contents would be a lot easier to use in this context
> if it could take a string. Otherwise you have to create a buffer,
> select it, insert a string, then pass that buffer to
> replace-buffer-contents, delete the buffer.

json-read-from-string uses a temporary buffer anyway, so a couple of
trivial changes in json-pretty-print should do the trick.

IOW, we have buffer text to begin with, so going through strings is
the part that should be eliminated, not the other way around, IMO.

Re: save-excursion doesn't restore point with json-pretty-print

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: help-gnu-emacs@gnu.org
>> Date: Fri, 01 Feb 2019 11:33:23 +0100
>> 
>> > Perhaps json-pretty-print could use the new replace-buffer-contents
>> > function?  Although I'm not sure this will cure the problem, at least
>> > not in all cases.
>> 
>> replace-buffer-contents would be a lot easier to use in this context
>> if it could take a string. Otherwise you have to create a buffer,
>> select it, insert a string, then pass that buffer to
>> replace-buffer-contents, delete the buffer.
>
> json-read-from-string uses a temporary buffer anyway, so a couple of
> trivial changes in json-pretty-print should do the trick.

Is this what you have in mind?  Seems to work well.  I'm just not sure
if the save-excursion should be in `json-pretty-print' itself.  I'd
always want to have it but maybe there are use-cases where it would be
wrong.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/json.el b/lisp/json.el
index 26cd48f41d..966b2e680c 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -740,14 +740,23 @@ json-pretty-print-buffer
 (defun json-pretty-print (begin end)
   "Pretty-print selected region."
   (interactive "r")
-  (atomic-change-group
-    (let ((json-encoding-pretty-print t)
-          ;; Distinguish an empty objects from 'null'
-          (json-null :json-null)
-          ;; Ensure that ordering is maintained
-          (json-object-type 'alist)
-          (txt (delete-and-extract-region begin end)))
-      (insert (json-encode (json-read-from-string txt))))))
+  (save-excursion
+    (save-restriction
+      (narrow-to-region begin end)
+      (goto-char (point-min))
+      (atomic-change-group
+        (let ((json-encoding-pretty-print t)
+              ;; Distinguish an empty objects from 'null'
+              (json-null :json-null)
+              ;; Ensure that ordering is maintained
+              (json-object-type 'alist)
+              (obj (json-read))
+              (orig-buffer (current-buffer)))
+          (with-temp-buffer
+            (insert (json-encode obj))
+            (let ((tmp-buffer (current-buffer)))
+              (set-buffer orig-buffer)
+              (replace-buffer-contents tmp-buffer))))))))
 
 (defun json-pretty-print-buffer-ordered ()
   "Pretty-print current buffer with object keys ordered."
--8<---------------cut here---------------end--------------->8---

Oh, one part where that's not completely right is undoing.  That is,
pretty-printing keeps point on the character it has been before but
undoing the pretty-printing will place point at the end of the json.

Bye,
Tassilo

Re: save-excursion doesn't restore point with json-pretty-print

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: help-gnu-emacs@gnu.org
> Date: Fri, 01 Feb 2019 19:02:23 +0100
> 
> > json-read-from-string uses a temporary buffer anyway, so a couple of
> > trivial changes in json-pretty-print should do the trick.
> 
> Is this what you have in mind?  Seems to work well.

Yes.

> I'm just not sure if the save-excursion should be in
> `json-pretty-print' itself.  I'd always want to have it but maybe
> there are use-cases where it would be wrong.

Good question.  I don't know the answer either.

Re: save-excursion doesn't restore point with json-pretty-print

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> > json-read-from-string uses a temporary buffer anyway, so a couple of
>> > trivial changes in json-pretty-print should do the trick.
>> 
>> Is this what you have in mind?  Seems to work well.
>
> Yes.

Ok, great.  Do you think it would make sense to extract the mechanics of
narrowing, extracting from the narrowed source buffer, and injecting
(transformed) into the temporary buffer with which to replace the source
region into some function

  (replace-region-contents beg end extract-fn inject-fn)

That way, `json-pretty-print' would look like.

--8<---------------cut here---------------start------------->8---
(defun json-pretty-print (begin end)
  "Pretty-print selected region."
  (interactive "r")
  (atomic-change-group
    (replace-region-contents
     begin end
     (lambda ()
       (let ((json-encoding-pretty-print t)
	     ;; Distinguish an empty objects from 'null'
	     (json-null :json-null)
	     ;; Ensure that ordering is maintained
	     (json-object-type 'alist))
	 (json-read)))
     (lambda (json-obj)
       (json-encode json-obj)))))
--8<---------------cut here---------------end--------------->8---

If so, where should I put it?  subr.el?

>> I'm just not sure if the save-excursion should be in
>> `json-pretty-print' itself.  I'd always want to have it but maybe
>> there are use-cases where it would be wrong.
>
> Good question.  I don't know the answer either.

Then let's keep it the current way until someone complains which will
probably never happen.

Bye,
Tassilo

Re: save-excursion doesn't restore point with json-pretty-print

[Tassilo Horn]

Tassilo Horn <tsdh@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> > json-read-from-string uses a temporary buffer anyway, so a couple of
>>> > trivial changes in json-pretty-print should do the trick.
>>> 
>>> Is this what you have in mind?  Seems to work well.
>>
>> Yes.
>
> Ok, great.  Do you think it would make sense to extract the mechanics of
> narrowing, extracting from the narrowed source buffer, and injecting
> (transformed) into the temporary buffer with which to replace the source
> region into some function
>
>   (replace-region-contents beg end extract-fn inject-fn)
>
> That way, `json-pretty-print' would look like.

[...]

Nah, but almost.  This is the actual working code which I'd happily
commit if you agree:

--8<---------------cut here---------------start------------->8---
;; in subr.el (or wherever you please)
(defun replace-region-contents (beg end extract-fn inject-fn)
  "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.

The current buffer is narrowed to the region between BEG and END,
then EXTRACT-FN is called in order to extract some value.
Thereafter, INJECT-FN is called with that value in a temporary
buffer which it should populate.

Finally, the region in the source buffer is replaced with the
contents of the temporary buffer prepared by INJECT-FN using
`replace-buffer-contents'."
  (save-excursion
    (save-restriction
      (narrow-to-region beg end)
      (goto-char (point-min))
      (atomic-change-group
	(let ((source-buffer (current-buffer))
	      (extracted (funcall extract-fn)))
	  (with-temp-buffer
	    (funcall inject-fn extracted)
	    (let ((tmp-buffer (current-buffer)))
              (set-buffer source-buffer)
              (replace-buffer-contents tmp-buffer))))))))

;; in json.el
(defun json-pretty-print (begin end)
  "Pretty-print selected region."
  (interactive "r")
  (replace-region-contents
   begin end
   (lambda ()
     (let ((json-null :json-null)
	   ;; Ensure that ordering is maintained
	   (json-object-type 'alist))
       (json-read)))
   (lambda (json-obj)
     (let ((json-encoding-pretty-print t)
	   ;; Distinguish an empty objects from 'null'
	   (json-null :json-null))
       (insert (json-encode json-obj))))))
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

Re: save-excursion doesn't restore point with json-pretty-print

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: help-gnu-emacs@gnu.org
> Date: Fri, 01 Feb 2019 21:00:04 +0100
> 
> Nah, but almost.  This is the actual working code which I'd happily
> commit if you agree:

LGTM. but maybe propose this on emacs-devel, in case someone else
would like to comment.

> ;; in subr.el (or wherever you please)

subr-x.el, I think.

Thanks.

Re: save-excursion doesn't restore point with json-pretty-print

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: help-gnu-emacs@gnu.org
>> Date: Fri, 01 Feb 2019 21:00:04 +0100
>> 
>> Nah, but almost.  This is the actual working code which I'd happily
>> commit if you agree:
>
> LGTM. but maybe propose this on emacs-devel, in case someone else
> would like to comment.

Will do.

Thanks,
Tassilo