Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eval UX improvements #3148

Merged
merged 7 commits into from
Feb 11, 2022
Merged

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Feb 10, 2022

This is the first of a bunch of tweaks on my local copy of Cider that I've been using for almost 2 years now and never got around to submitting. I hope it's alright if I group several related changes together in a single PR, much of the friction comes from having to rebase commits and write multiple changelogs.

  • Overlay text takes on the face of the following text - this is especially noticeable with coloured parens and cider-result-overlay-position set to 'at-point'.
    Before (using cider-eval-last-sexp, the text takes on the yellow of the closing paren next to it):
    image
    After:
    image

  • When an error occurs and cider-show-error-buffer is set to nil, an overlay is created with an error indicating face. Currently there is no indication at all of the type or presence of error.
    I've been using this setting for some time now and find it much more responsive and less distracting than a stacktrace buffer popping up and needing to be dismissed. Most of the time a simple error message is enough information, and one can use cider-selector to bring up the stacktrace buffer if needed.

image

  • Similarly with cider-eval-to-comment commands, it helps (especially for teaching purposes) to have the error output to the comment:
    image

I don't think tests or user manual updates are needed here?


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@yuhan0 yuhan0 force-pushed the eval-overlay-improvements branch from 6ad52fb to cf72cab Compare February 10, 2022 14:59
@vemv
Copy link
Member

vemv commented Feb 10, 2022

The commits look reasonable to me!

As for the screenshots, have you tried having a variety of exceptions thrown, and seeing how they are rendered?

Some things I would expect for a smooth UX would be:

  • The final cause is displayed (might be under a chain of prior causes)
  • One or two lines are displayed at most
  • Width isn't excessive
  • Ideally, any irrelevant info would be trimmed away (e.g. Long is in module java.base...)

It's not a blocker but I'd certainly appreciate that experimentation. If something was needed from cider-nrepl side I could try providing it (maybe as a new k-v entry: shortened-error).

@yuhan0
Copy link
Contributor Author

yuhan0 commented Feb 11, 2022

The only thing it does is print out the error message exactly as sent to stderr, so naturally it handles all kinds of errors:

image

image

image

A slight visual nit is the trailing newline being rendered as a hanging background-coloured box - if string-trim-right is applied it would look like this:
image
I personally prefer the former for consistency of the extended faces.

  • The final cause is displayed (might be under a chain of prior causes)

This is only a few keystrokes away with cider-selector to bring up the full stacktrace of exceptions - I've found that the majority of errors can be identified by their type and message alone.

  • One or two lines are displayed at most

I don't see the advantage in truncating multiline error messages, after all the overlay can be dismissed easily.

  • Width isn't excessive

Linebreaks and indentation in the error message might be meaningful, so trying to fill-paragraph would run into undesired formatting. Within the window the width is naturally wrapped around or extended offscreen depending on truncate-line-mode.

  • Ideally, any irrelevant info would be trimmed away (e.g. Long is in module java.base...)

Again I think it's best to have as little "magic" as needed here - simply echoing the stderr instead of trying to determine what is relevant / irrelevant to show. (There are already external libraries like expound which simplify / pretty print error messages, which is a separate concern)

@bbatsov bbatsov merged commit f3309c3 into clojure-emacs:master Feb 11, 2022
@bbatsov
Copy link
Member

bbatsov commented Feb 11, 2022

I love the proposed changes and I don't think that any of them are controversial, so let's just merge this and polish it as we go based on whatever user feedback we receive. Thanks for tackling this! 🙇‍♂️

yuhan0 added a commit to yuhan0/cider that referenced this pull request Apr 29, 2022
yuhan0 added a commit to yuhan0/cider that referenced this pull request Apr 29, 2022
yuhan0 added a commit to yuhan0/cider that referenced this pull request Apr 29, 2022
bbatsov pushed a commit that referenced this pull request Apr 29, 2022
deleted by mistake, oops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants