[Fix #1859] In message log insert large objects on request only

[vspinu]

Instead of removing the pp logic I have moved it to a different place. The pp print of big dicts, lists and strings now happens on requests only, i.e. when the user clicks the expansion button. This completely removes logger overhead. In my tests I don't see nrepl-message-log in the profiler tree anymore.

Not closing the #1758 as there seem to be much more going on there.

[bbatsov]

Probably constants like this one should be moved to defcustoms (as some people might want to tune them).

[vspinu]

It looks rather useless option to me. Even though it's arbitrary I don't see why would anyone care to change that.

If we add config here, we should probably add a config for the minimal size of the dict that is buttonized. Currently I am buttonizing all non-empty dicts. One option would be to have same constant for both dicts and lists, but that would mean printing potentially deeply nested objects if at each level there is a small list.

Also strings are truncated if they are longer than 80chars. Do you want an option for that as well? :)

[vspinu]

BTW, there is a now a defunct nrepl-dict-max-message-size which I forgot to remove. IMO, unfolding dicts is an unnecessary overhead. People can always click when needed and it saves quite some space.

One slight drawback of this design that you cannot copy-paste or save the log directly because there is no text in those buttons in the first place. We can have a small utility nrepl-log-unfold-all-buttons to handle this.

[bbatsov]

If we add config here, we should probably add a config for the minimal size of the dict that is buttonized. Currently I am buttonizing all non-empty dicts. One option would be to have same constant for both dicts and lists, but that would mean printing potentially deeply nested objects if at each level there is a small list.

Oh, I've missed this. I guess top-level dicts shouldn't be folded, though, as you'll get all your context about a request/response.

I generally don't like magic numbers flowing around. I'm fine with not using defcustoms, but even consts make it easier to read the code. You can spruce up some comments here and there in the folding logic, as some decision might not be obvious to everyone.

[vspinu]

I guess top-level dicts shouldn't be folded,

The top dicts are not folded, only 1st level dicts are (aka those which display with (dict ...). It looks like this:

(--> 
  op  "describe"
  session  "67aafc4f-8ba7-4b41-9a20-8f418ca0da8c"
  id  "8"
)
(<-- 
  aux  (dict ...)
  id  "8"
  ops  (dict ...)
  session  "67aafc4f-8ba7-4b41-9a20-8f418ca0da8c"
  status  ("done")
  versions  (dict ...)
)

You can spruce up some comments here and there in the folding logic, as some decision might not be obvious to everyone.

Ok. I will get all these into a local let. You can extract them into variables if it will ever be a need for it.

[bbatsov]

Sounds good to me.

[bbatsov]

Guess a better name would be nrepl--log-insert-button or something like this.

[bbatsov]

One slight drawback of this design that you cannot copy-paste or save the log directly because there is no text in those buttons in the first place. We can have a small utility nrepl-log-unfold-all-buttons to handle this.

Sounds reasonable to me.

[vspinu]

Guess a better name would be nrepl--log-insert-button or something like this.

Then all others should be renamed for consistency: nrepl--expand-button, nrepl--message-color, nrepl--expand-button-mouse, nrepl--pp. There is no consistency with respect to nrepl-log-.. and nrepl-message-.. prefix either.

[bbatsov]

Yeah, ideally we should renamed all the related stuff in some consistent manner, but that's not super important. If you'd like to tackle it - that'd be great, otherwise I can do it down the road.

[vspinu]

Ok. I have parameterized the constants and renamed the main logging functions with nrepl-log- prefix.

I also fixed the keymap problems on buttons on this ocassion. Fix in #1568, 814692d used local-map property, which completely overwrites the major-mode map. So things like TAB, n,p and now e/E wouldn't work on buttons.

[bbatsov]

Thanks!