About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el

[Tassilo Horn]

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?

Thanks,
  Tassilo

Re: 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.

Re: 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

Re: 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

Re: 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.

Re: 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

Re: 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

Re: 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

Re: 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

Re: 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

Re: 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