clojure-emacs / clojure-mode

Sponsor

Sponsor clojure-emacs/clojure-mode
Watch 55
Star 783
Fork 218

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

Jump to bottom

Remove calls to `redisplay` from `clojure-sort-ns` #579

Merged

bbatsov merged 1 commit into clojure-emacs:master from tomdl89:master

Nov 26, 2020

Contributor

tomdl89 commented Nov 26, 2020 •

edited

This avoids unwanted flicker when jumping from current location
to ns form and back. Especially useful when clojure-sort-ns is
called from before-save-hook, and would otherwise distract the user.

Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

The commits are consistent with our contribution guidelines.
You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
You've run M-x checkdoc and fixed any warnings in the code you've written.
You've updated the changelog (if adding/changing user-visible functionality).
You've updated the readme (if adding/changing user-visible functionality).

Thanks!

Member

bbatsov commented Nov 26, 2020

Won't some save-excursion also tackle this?

Especially useful when clojure-sort-ns is
called from before-save-hook, and would otherwise distract the user.

Probably you should document this somewhere, otherwise you'll probably be the only user of the new functionality. :-)

Contributor Author

tomdl89 commented Nov 26, 2020

The (redisplay) that jumps us up to the ns occurs within a save-excursion (see new ln 1829) so I'm not sure I understand how adding more save-excursions would fix this. Good point on the documentation. I should add that to the docstring.

Member

bbatsov commented Nov 26, 2020

The (redisplay) that jumps us up to the ns occurs within a save-excursion (see new ln 1829) so I'm not sure I understand how adding more save-excursions would fix this.

I wrote this without really reading the code. :-) I'm not quite sure what's the nature of the flicker you experience. From the redisplay's docstring it seems that the cursor shouldn't jump around, but I'm not familiar with the function.

Contributor Author

tomdl89 commented Nov 26, 2020

The cursor jumps around because of the goto-char command (and then again because of the forward-sexp etc commands). Usually, save-excursion allows all of this to be done without redisplaying (so the user never sees the movement), however, I guess whoever wrote this code thought that the user would be reassured seeing the cursor jump up to the ns form and then back down again. I guess it confirms that something is happening? I added the suppress-redisplay? arg to allow the user to opt out. I would be fine with removing the redisplay calls, but presumably some users like to see the jumps? (When I wrote "flicker" I was just referring to fact that the jumps are so fast, btw).

Member

bbatsov commented Nov 26, 2020

I'd just remove the redisplay if it's not adding anything meaningful. I always prefer to delete code rather than add more code to deal with the existing code. :-)

Contributor Author

tomdl89 commented Nov 26, 2020

Ok :) that was my preference all along, so suits me! Thanks

tomdl89 force-pushed the tomdl89:master branch from 7dddb5b to d952c76

bbatsov reviewed

clojure-mode.el

  
      @@ -1843,7 +1842,6 @@ content) are considered part of the preceding sexp."
    
                  (if (looking-at (regexp-quote ns))

                      (message "ns form is already sorted")

                    (sleep-for 0.1)

                    (redisplay)

bbatsov Nov 26, 2020
Member

I think you shoudl also kill those surrounding sleeps, as I can't see why they would be needed.

bbatsov Nov 26, 2020
Member

I see one before and after the redisplay.


          Add `suppress-redisplay?` optional arg to `clojure-sort-ns`

330cbcc

This avoids unwanted flicker when jumping from current location
to ns form and back. Espeically useful when `clojure-sort-ns` is
called from `before-save-hook`, and would otherwise distract the user.

tomdl89 force-pushed the tomdl89:master branch from d952c76 to 330cbcc

tomdl89 changed the title ~~Add `suppress-redisplay?` optional arg to `clojure-sort-ns`~~ Remove calls to `redisplay` from `clojure-sort-ns`

bbatsov merged commit 53ef8ac into clojure-emacs:master

0 of 4 checks passed

0 of 4 checks passed

ci/circleci: test-emacs-25 Your tests failed on CircleCI

Details

ci/circleci: test-emacs-26 Your tests failed on CircleCI

Details

ci/circleci: test-emacs-27 Your tests failed on CircleCI

Details

ci/circleci: test-emacs-master Your tests failed on CircleCI

Details

Member

bbatsov commented Nov 26, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.