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

Feature: multiple persistent eval overlays #3149

Merged
merged 5 commits into from
Feb 11, 2022

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Feb 10, 2022

This adds a third option 'change to cider-eval-result-duration, which persists eval overlays until there is a change to the buffer, allowing for easy comparisons between different forms.

A quick demo:
overlays

Note: This is how Calva behaves, see the demo at https://calva.io/try-first/


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.

cider-eval.el Outdated
@@ -897,7 +897,10 @@ arguments and only proceed with evaluation if it returns nil."
(start (car-safe bounds))
(end (car-safe (cdr-safe bounds))))
(when (and start end)
(remove-overlays start end 'cider-temporary t))
;; NOTE: `remove-overlays' splits and leaves behind partial overlays
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this comment could be more verbose?

e.g. NOTE: please don't use remove-overlays, as it splits and leaves behind partial overlays. See <source>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<source> referring to the function's docstring explaining this behaviour or the resulting issue?
Here's the problem that arises with multiple overlays and splitting:
o2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess to the issue, but I don't have a strong preference here, as long as it's clear what we're avoiding.

@bbatsov
Copy link
Member

bbatsov commented Feb 11, 2022

I like the proposed change. Let's just document this somewhere (I'm not sure if we've even documented the old two options) and illustrated the differences with a couple of animated gifs like the one you've used in the PR description.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Feb 11, 2022

Ok, I've updated the changelog and manual, also another commit that I accidentally missed out from the last PR.
I also realise that my title above was inaccurate, currently setting the variable to a number or nil currently does allow for multiple overlays on screen.

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

bbatsov commented Feb 11, 2022

Thanks!

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