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

Integrate overlays with interactive evaluation #1201

Merged
merged 1 commit into from
Jul 13, 2015
Merged

Integrate overlays with interactive evaluation #1201

merged 1 commit into from
Jul 13, 2015

Conversation

Malabarba
Copy link
Member

Closes #1196

@bbatsov
Copy link
Member

bbatsov commented Jul 13, 2015

Just one comment from me - won't it better to have the option select either minibuffer, overlays or both as the result's display scheme. Generally I'd never opt to use them both at the same time, as dislike information redundancy. I guess others might share my feelings on the subject.

@Malabarba
Copy link
Member Author

Just one comment from me - won't it better to have the option select either minibuffer, overlays or both as the result's display scheme

Done. I also set the default duration of overlays to be "until next command" which mimics the echo-area.
I also ended up rewriting some stuff and moving some stuff out of cider-interaction, so see if you agree with it.

bbatsov added a commit that referenced this pull request Jul 13, 2015
Integrate overlays with interactive evaluation
@bbatsov bbatsov merged commit 6b2e5af into clojure-emacs:master Jul 13, 2015
@bbatsov
Copy link
Member

bbatsov commented Jul 13, 2015

👍 Looks good to me. Not sure about the default value for cider-user-overlays, but let's merge it as is and see what the users say about it.

@arrdem
Copy link
Contributor

arrdem commented Jul 13, 2015

👍 yey more pretty!

@Malabarba
Copy link
Member Author

👍

And I'm not sure about the default value either.

@Malabarba
Copy link
Member Author

Here's a data point for the default-value discussion. When you get a result of several lines, having it print in both places will completely take up the window.

@bbatsov
Copy link
Member

bbatsov commented Jul 14, 2015

@Malabarba Maybe we should just leave the old version as the default and add a note and a screenshot to the README, regarding the overlays. I'm totally fine with making them the default as well, I'm just worried about potential bugs, as this hasn't been tested that extensively yet.

@Malabarba
Copy link
Member Author

@bbatsov Yes, I've been using it and uncovering a few corner cases. There'll be a PR related to that soon.
It might be good to turn this off before release (but maybe keep it on until then?).

@bbatsov
Copy link
Member

bbatsov commented Jul 15, 2015

You're a mind reader - this is exactly what I've been thinking about. It makes a lot of sense to solicit feedback about the feature right now, so let's leave it on until the release.

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