clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.52k stars 643 forks source link

Limit the maximum string size to be displayed in echo area #3661

Closed alexander-yakushev closed 2 months ago

alexander-yakushev commented 2 months ago

Although there is still work to be done (and thoughts to be thought) in cider-nrepl or nREPL regarding how much we want to print, it makes sense to solve such an issue on CIDER side too. Currently, if cider-use-overlays is not t, the standard cider-interactive-eval-handler will try to display the whole printed result in the minibuffer (echo area). For some reason, pushing huge strings through the echo area is much slower than printing it to the REPL. E.g., doing (println (range 200000)) is almost instant while evaluating (range 200000) in the source buffer takes ~3 seconds on my machine to display the truncated result. Emacs would obviously still truncate the echo area output but not before printing the whole response to *Messages*.

This PR proposes to truncate the long output ahead of time. We can also add the similar message that we currently show in overlays, something like "Result truncated. Type M-x cider-inspect-last-result to inspect it. if it is deemed necessary.

Example of how the truncated result would look like:

image
alexander-yakushev commented 2 months ago

I guess this intersects with #3635 (only spotted it now).

While I think that PR solves a more general problem, it may make sense to restrict it here as well. Dunno, let's discuss.

alexander-yakushev commented 2 months ago

I've looked through https://github.com/clojure-emacs/cider/pull/3635 and now I think that my PR is unnecessary. While with that PR, the echo area would still receive more than it could handle (e.g., 10k characters instead of ~1500 with this PR), the difference in perceived latency is not noticeable. On the plus side, we won't be doing similar work in two places.

If we can fast forward #3635 – that will be great.

vemv commented 2 months ago

While I think that PR solves a more general problem, it may make sense to restrict it here as well. Dunno, let's discuss.

I don't mind about having both in place - this can be a last-resource measure.

A drawback being that users (/ maintainers) have to understand/tweak more stuff (e.g. let's imagine that each of the solutions has a different defcustom, or in any case, a different algo).

Maybe for the sake of containing complexity (i.e. favoring existing tech, namely the "print quotas"), let's go for https://github.com/clojure-emacs/cider/pull/3635 . I can polish it later this week or if it feels like a blocker, I'd be OK with you taking over it.

vemv commented 2 months ago

If we can fast forward https://github.com/clojure-emacs/cider/pull/3635 – that will be great.

You beat me to it ^^

as mentioned, I can go back to it soon, else you can take over.

alexander-yakushev commented 2 months ago

Actually, let me reopen.

I just understood that because an overflowing message in the echo area truncates the beginning:

image

then, after #3635 is applied (which truncates the end), an overflowing message will show the middle of printed value, which is going to be quite ugly and probably quite confusing. Currently, at least, the user gets to see the end of the value and understand the return type (here – a list), but when shown the middle of the message they won't even get that.

Also, arguably the beginning of the value is more useful then the end of it.

So, I think, we'll still need this PR, and perhaps even more so after #3635.

vemv commented 2 months ago

If a small enough quota was requested for ops which output goes to the echo area, why would there be truncating in the beginning?

alexander-yakushev commented 2 months ago

That implies that different quota is used when the output is going to the minibuffer vs the REPL vs somewhere else. From #3635 I gathered that it isn't going to be the case.

Besides, this goes against how cider--display-interactive-eval-result is implemented (at least, currently). The function receives a single result, and then sends it to two destinations (if cider-use-overlays = 'both). I guess, the echo area limit is still larger than the overlay limit (the overlay one is now at 3*frame-width), but making the presentation depend on that feels kinda flaky to me.

alexander-yakushev commented 2 months ago

Addressed the comments.

A drawback being that users (/ maintainers) have to understand/tweak more stuff (e.g. let's imagine that each of the solutions has a different defcustom, or in any case, a different algo).

I don't think we would need a defcustom for this one. Here, we are lucky to get "help" from the minibuffer implementation by being kinda "broken" currently since it can't go over (max-mini-window-lines). So, in my opinion, any solution that we provide is better than status quo, and hence, we aren't going to break any current UX for people to miss.

vemv commented 2 months ago

That implies that different quota is used when the output is going to the minibuffer vs the REPL vs somewhere else. From https://github.com/clojure-emacs/cider/pull/3635 I gathered that it isn't the case.

You're partially right, but in that PR some parts were intentionally unchanged to not tighten the quota.

So indeed the 'overlay' and 'echo area' limits would be coupled.

Honestly I believe that it would be a very minor problem / edge case, because:

So my thinking is that the echo area can help as a quick confirmation that evaluation succeeded - its rough value.

Because it's 'transient', it only has relative value as it can easily go away with keypresses or other events.

So, tldr I don't exactly oppose this PR but it only seems useful for making very specific things like (range 20000) nicer. For most exprs there isn't a huge difference or a problem to begin with.

I'd prefer to keep things simple and not add further moving pieces, since they only address edge cases in the less favored of the available UIs.

alexander-yakushev commented 2 months ago

I agree with you that the problem is not a huge one. But I'd still prefer to see the beginning of the value in the minibuffer if the value is too large. At least, it will be consistent with how we trim values everywhere else.

(range 20000) is an example of the value that we will only show the middle of after #3636 is merged. But for any value over (max-mini-window-lines), we'll still show the tail instead of the head, and seeing the head at a quick glance, in my opinion, gives much better information about the structure of the response, even if the tail is truncated.

alexander-yakushev commented 2 months ago

since they only address edge cases in the less favored of the available UIs.

To your point about the UI being less favored – it is true that the default for cider-use-overlays is 'both, however:

  1. Both means we still print to minibuffer by default, so it is still visible to the user, even if the same value is also shown as an overlay. So we can assume that most users see the minibuffer output.
  2. While I may be in the minority of users who disable overlays and only print the result to the minibuffer, the minibuffer is still the primary communication vessel in Emacs, so I would avoid writing it off as redundant or obsolete.

So, I think, if we can improve the UX of it with small costs, then it is reasonable to do so.

alexander-yakushev commented 2 months ago

Thanks for the review!

vemv commented 2 months ago

Cheers.

Frankly I'd review the default of cider-use-overlays at some point - overlays have gotten better and better over the last year, so showing duplicated info (and that can often look worse for the echo area part) makes little sense.