bug#34160: json-pretty-print deletes everything after first JSON object

bug#34160: json-pretty-print deletes everything after first JSON object

[Albert Heinle]

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

Dear Emacs-dev team,

I have observed the following behavior of the command json-pretty-print
(http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740):

Write

{"a": 1}{"b": 2}

into any Emacs buffer. Then, mark the whole section and run M-x
json-pretty-print.
The line is afterwards altered as
{
  "a": 1
}

This means, that any string after the first completely parsed JSON-object
is being removed by this function. I would consider this unexpected,
because information gets lost after calling a function that is just
supposed to prettify things.

My Emacs version:
GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9) of 2017-09-20
on lcy01-07, modified by Debian

I also have an Arch-linux system at home where I could also reproduce this
behavior.

Let me know if you have any other questions.

Thank you very much,

Albert Heinle

[-- Attachment #2: Type: text/html, Size: 1373 bytes --]

bug#34160: json-pretty-print deletes everything after first JSON object

[Lars Ingebrigtsen]

Albert Heinle <albert.heinle@googlemail.com> writes:

> Write
>
> {"a": 1}{"b": 2}
>
> into any Emacs buffer. Then, mark the whole section and run M-x json-pretty-print.
> The line is afterwards altered as
> {
>   "a": 1
> }

I think I've now fixed this on the Emacs trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34160: json-pretty-print deletes everything after first JSON object

[Glenn Morris]

After the change, test-json-pretty-print-object fails.
Ref eg https://hydra.nixos.org/build/95978732

bug#34160: json-pretty-print deletes everything after first JSON object

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> After the change, test-json-pretty-print-object fails.
> Ref eg https://hydra.nixos.org/build/95978732

Thanks; should be fixed now...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Date: Wed, 31 Jul 2019 09:39:00 +0200
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 34160@debbugs.gnu.org
> 
> Hi Lars,
> 
> when fixing bug#34160 you've reverted my changes that made json pretty
> printing use replace-region-contents.  That had the major benefit that
> pretty printing the JSON object at point didn't move point.  I use that
> many times a week on large JSON objects using the following command.
> 
> --8<---------------cut here---------------start------------->8---
> (defun th/json-pretty-print-snippet-at-point (&optional minimize)
>   "Pretty-print the json snippet at point."
>   (interactive "P")
>   (save-excursion
>     (when-let ((beg (car (nth 9 (syntax-ppss)))))
>       (goto-char beg)
>       (forward-sexp)
>       (when (looking-back "\n" beg)
> 	(backward-char))
>       (json-pretty-print beg (point) minimize))))
> --8<---------------cut here---------------end--------------->8---
> 
> AFAICS, the problem in bug#34160 was not caused by my changes (the user
> used Emacs 24 and not a 27 snapshot) so I see no justification for
> removing my feature.
> 
> Could you please reinstall the feature or describe why it is not
> feasible to keep it?

Oops, sorry about that.

I think Lars is on vacation.  If he doesn't respond in a day or two, I
will revert the change until this issue is resolved.

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

Hi chaps,

> I think Lars is on vacation.  If he doesn't respond in a day or two, I
> will revert the change until this issue is resolved.

Thanks, Eli. If nobody is faster, I'll try to fix bug#34160 on the weekend 
in a way that satisfies all of us.

Bye,
  Tassilo

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Lars Ingebrigtsen]

Tassilo Horn <tsdh@gnu.org> writes:

> when fixing bug#34160 you've reverted my changes that made json pretty
> printing use replace-region-contents.  That had the major benefit that
> pretty printing the JSON object at point didn't move point.  I use that
> many times a week on large JSON objects using the following command.

[...]

> AFAICS, the problem in bug#34160 was not caused by my changes (the user
> used Emacs 24 and not a 27 snapshot) so I see no justification for
> removing my feature.

The user referred to
"http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740",
which doesn't look like Emacs 24?

> Could you please reinstall the feature or describe why it is not
> feasible to keep it?

As the bug in question described -- pretty-printing a JSON region would
silently delete everything but the first JSON object, which doesn't seem
like optimal behaviour for a pretty-printing function.

If there's a problem where point is moved unnecessarily, then that
should be fixed, of course.  Do you have a test case?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> CC: <emacs-devel@gnu.org>, <larsi@gnus.org>, <34160@debbugs.gnu.org>
> Date: Wed, 31 Jul 2019 20:40:00 +0200
> 
> > I think Lars is on vacation.  If he doesn't respond in a day or two, I
> > will revert the change until this issue is resolved.
> 
> Thanks, Eli. If nobody is faster, I'll try to fix bug#34160 on the weekend 
> in a way that satisfies all of us.

Great, thanks.

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

> The user referred to
> "http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740",
> which doesn't look like Emacs 24?

Ah, might be.

>> Could you please reinstall the feature or describe why it is not
>> feasible to keep it?
>
> As the bug in question described -- pretty-printing a JSON region would
> silently delete everything but the first JSON object, which doesn't seem
> like optimal behaviour for a pretty-printing function.

Obviously not. :-)

> If there's a problem where point is moved unnecessarily, then that
> should be fixed, of course.  Do you have a test case?

It's not just moving point. replace-region-contents also keeps marks, text 
properties and fontification intact. So we should definitely be using it here.

The loop over all json objects in the region you've added is correct. It's 
just that I beg you to drop the delete-region / insert in favor of 
replace-region-contents.

json-read advances point until the end of the read json. This can be used 
to give the right region (not the complete region as I did) to the repeated 
replace-region-contents calls.

Feel free to give it a try. Otherwise I'll do it on the weekend.

For a test case for point keeping its position in the json: use my command 
from my original mail and an arbitrary json file and invoke it while point 
is somewhere inside the json object.

Bye,
  Tassilo

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Lars Ingebrigtsen]

Tassilo Horn <tsdh@gnu.org> writes:

> It's not just moving point. replace-region-contents also keeps marks,
> text properties and fontification intact. So we should definitely be
> using it here.

Ah, I see.  I only gave that function a cursory look-over when fixing
this bug and I didn't quite understand what it was doing here, since I
couldn't recall any other pretty-printer doing anything similar.

Sorry for the confusion here; I've now restored the
replace-region-contents logic.  Or at least I think so; it works with
the test cases in this bug report, at least.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

>> It's not just moving point. replace-region-contents also keeps marks,
>> text properties and fontification intact. So we should definitely be
>> using it here.
>
> Ah, I see.  I only gave that function a cursory look-over when fixing
> this bug and I didn't quite understand what it was doing here, since I
> couldn't recall any other pretty-printer doing anything similar.

No worries!  You probably didn't see it more often because it's quite
new.  But pretty-printing is definitely a very good use-case for it.

> Sorry for the confusion here; I've now restored the
> replace-region-contents logic.  Or at least I think so; it works with
> the test cases in this bug report, at least.

Yes, it works again.  Thanks!

It can still be a bit improved in understandability and efficiency.

1. The function passed to replace-region-contents runs on the narrowed
   buffer anyway, so no need to narrow it yourself.
   
2. It would be better to create a temporary buffer, json-read repeatedly
   from the original buffer, json-encode/insert to the temp one, and
   then return the temp buffer.

The reason for point 2 is that if the function passed to
replace-region-contents returns a string, it'll put that in a temporary
buffer anyhow so that it can use replace-buffer-contents to perform the
actual replacement (replace-region-contents is just a wrapper around
that).

And we might want to cater for the situation where the region starts or
ends inside a json object by copying the buffer substring from (point)
to end to the temp buffer in case json-read fails.  I think right now,
we'd lose such half json objects and everything which follows them.

Bye,
  Tassilo

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Lars Ingebrigtsen]

Tassilo Horn <tsdh@gnu.org> writes:

> It can still be a bit improved in understandability and efficiency.
>
> 1. The function passed to replace-region-contents runs on the narrowed
>    buffer anyway, so no need to narrow it yourself.
>
> 2. It would be better to create a temporary buffer, json-read repeatedly
>    from the original buffer, json-encode/insert to the temp one, and
>    then return the temp buffer.
>
> The reason for point 2 is that if the function passed to
> replace-region-contents returns a string, it'll put that in a temporary
> buffer anyhow so that it can use replace-buffer-contents to perform the
> actual replacement (replace-region-contents is just a wrapper around
> that).

Sounds like a good idea; please go ahead.

> And we might want to cater for the situation where the region starts or
> ends inside a json object by copying the buffer substring from (point)
> to end to the temp buffer in case json-read fails.  I think right now,
> we'd lose such half json objects and everything which follows them.

Yes, that sounds like a good fix.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> It can still be a bit improved in understandability and efficiency.
>>
>> 1. The function passed to replace-region-contents runs on the narrowed
>>    buffer anyway, so no need to narrow it yourself.
>>
>> 2. It would be better to create a temporary buffer, json-read repeatedly
>>    from the original buffer, json-encode/insert to the temp one, and
>>    then return the temp buffer.
>>
>> The reason for point 2 is that if the function passed to
>> replace-region-contents returns a string, it'll put that in a temporary
>> buffer anyhow so that it can use replace-buffer-contents to perform the
>> actual replacement (replace-region-contents is just a wrapper around
>> that).
>
> Sounds like a good idea; please go ahead.

Will do at the weekend.

Bye,
  Tassilo

bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

Tassilo Horn <tsdh@gnu.org> writes:

Hi Lars,

>>> It can still be a bit improved in understandability and efficiency.
>>>
>>> 1. The function passed to replace-region-contents runs on the narrowed
>>>    buffer anyway, so no need to narrow it yourself.
>>>
>>> 2. It would be better to create a temporary buffer, json-read
>>>    repeatedly from the original buffer, json-encode/insert to the temp
>>>    one, and then return the temp buffer.
>>
>> Sounds like a good idea; please go ahead.
>
> Will do at the weekend.

Ok, I did it just now (10065010a6).  Seems to work very well.

Bye,
Tassilo