Skip to content

Scroll buffer after using cider-insert-X-in-repl #2590

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

Merged
merged 5 commits into from
Feb 25, 2019

Conversation

dpsutton
Copy link
Contributor

@dpsutton dpsutton commented Feb 16, 2019

As it stood, buffer would not scroll, even if you included a
(goto-char (point-max)) call at the end. Using solution from
https://stackoverflow.com/questions/13530025/emacs-scroll-automatically-when-inserting-text

Have to watch out if the buffer is visible or not. If it is, we can
scroll it with with-selected-window. Otherwise, the best we can do
is the current behavior. Note get-buffer-window returns nil if
something is not visible which is why we need to branch on it.

Currently

Using the cider-insert-[last-sexp|defun|region]-inrepl commands (C-c M-j [e,d,r]) were very convenient but would not scroll the buffer, limiting their usability.

before

Notice that the repl buffer is not scrolled. Simply adding a (goto-char (point-max)) in cider-insert-in-repl was not sufficient due. (see stack overflow post from commit message above).

After

By using with-selected-window we can manipulate the view of that buffer. So now we end up with this much more sensical view when using these commands:

after

@dpsutton dpsutton requested a review from arichiardi February 16, 2019 21:54
@dpsutton dpsutton force-pushed the feature/scroll-on-insert branch 2 times, most recently from c5b2c40 to 761d6bb Compare February 16, 2019 22:23
As it stood, buffer would not scroll, even if you included a
`(goto-char (point-max))` call at the end. Using solution from
https://stackoverflow.com/questions/13530025/emacs-scroll-automatically-when-inserting-text

Have to watch out if the buffer is visible or not. If it is, we can
scroll it with `with-selected-window`. Otherwise, the best we can do
is the current behavior. Note `get-buffer-window` returns nil if
something is not visible which is why we need to branch on it.
@dpsutton dpsutton force-pushed the feature/scroll-on-insert branch from 761d6bb to 7399bcc Compare February 16, 2019 22:36
@cichli
Copy link
Member

cichli commented Feb 16, 2019

We could switch to the REPL before inserting to make sure the buffer is visible, but cider-switch-to-repl-after-insert-p would have to be renamed.

@arichiardi
Copy link
Contributor

I am fine with the rename as seeing the output is more important ;)))

Thanks for opening this btw!

dan sutton added 2 commits February 16, 2019 18:25
makes it a bit nicer with scrolling if the buffer is visible. Possibly
need to rename the var `cider-switch-to-repl-after-insert-p`
To prevent this in the future we can just leave this var as
"on-insert" rather than constrain ourselves to before or after. The
remedy here is almost certainly more painful than the disease.
@dpsutton
Copy link
Contributor Author

I've marked obsolete and created a new defcustom

(defcustom cider-switch-to-repl-on-insert-p t
  "Whether to switch to the repl when inserting a form into the repl."
  :type 'boolean
  :group 'cider
  :package-version '(cider . "0.21.0"))

(define-obsolete-variable-alias
  'cider-switch-to-repl-after-insert-p
  'cider-switch-to-repl-on-insert-p
  "0.21.0")

I figure we can just say "on-insert" rather than before or after so hopefully we don't go through this again if it makes sense to focus in the buffer before/after insertion in the future.

Let me know if I didn't obsolete it correctly.

Copy link
Contributor

@arichiardi arichiardi left a comment

Choose a reason for hiding this comment

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

The code looked good but the I tried the PR and while the buffer now scrolls perfectly on insert. I do not see the output printed anymore (I have cider-switch-to-repl-on-insert-p to nil here btw which is what I like 😄).

@arichiardi
Copy link
Contributor

Oh it seems the problem could just be that it is not rebased on master.

@dpsutton
Copy link
Contributor Author

dpsutton commented Feb 17, 2019

@arichiardi do you have the correct settings? By default, CIDER doesn't eval the inserted forms automatically. use a prefix arg or set:

(setq cider-invert-insert-eval-p t)

@arichiardi
Copy link
Contributor

Ah lol maybe not :)

@dpsutton
Copy link
Contributor Author

also added fontlocking

with-fontlocking

cider-mode.el Outdated
@@ -223,6 +223,17 @@ With a prefix argument, prompt for function to run instead of -main."
:group 'cider
:package-version '(cider . "0.18.0"))

(defcustom cider-switch-to-repl-on-insert-p t
Copy link
Member

Choose a reason for hiding this comment

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

Defcustoms should not have predicate names IMO. I’ve never seen such naming for them.

:group 'cider
:package-version '(cider . "0.21.0"))

(define-obsolete-variable-alias
Copy link
Member

Choose a reason for hiding this comment

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

I don’t see the var that was made obsolete anywhere, so I guess that’s not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the var is right above the new one.

(defcustom cider-switch-to-repl-after-insert-p t
  "Whether to switch to the repl after inserting a form into the repl."
  :type 'boolean
  :group 'cider
  :package-version '(cider . "0.18.0"))

(defcustom cider-switch-to-repl-on-insert-p t
  "Whether to switch to the repl when inserting a form into the repl."
  :type 'boolean
  :group 'cider
  :package-version '(cider . "0.21.0"))

(define-obsolete-variable-alias
  'cider-switch-to-repl-after-insert-p
  'cider-switch-to-repl-on-insert-p
  "0.21.0")

The original had a -p so I mimic'ed it. I'll remove that from the new one.

@arichiardi
Copy link
Contributor

This looks very good to me 😄 Eager to have it in 😄

Thanks a lot @dpsutton!

@cichli cichli merged commit c35e862 into clojure-emacs:master Feb 25, 2019
@cichli
Copy link
Member

cichli commented Feb 25, 2019

Thanks for this! Sorry it's taken a while for me to get around to reviewing it.

@arichiardi
Copy link
Contributor

Awesome thank you!

@dpsutton dpsutton deleted the feature/scroll-on-insert branch October 26, 2019 16:07
dpsutton added a commit to dpsutton/inf-clojure that referenced this pull request Aug 6, 2022
When inserting forms into the repl, the buffer doesn't always scroll to
show the new input and result. This has been fixed in CIDER in a similar
way so bringing the same trick here.

clojure-emacs/cider#2590
bbatsov pushed a commit to clojure-emacs/inf-clojure that referenced this pull request Aug 7, 2022
When inserting forms into the repl, the buffer doesn't always scroll to
show the new input and result. This has been fixed in CIDER in a similar
way, so now I'm bringing the same trick here.

See clojure-emacs/cider#2590 for more details.
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.

4 participants