Skip to content

Commit

Permalink
[Fix #2126] Preserve the point position in cider-format-buffer
Browse files Browse the repository at this point in the history 
That's pretty important for people who want to apply the formatting
automatically on buffer save.

Unfortunately we can't use save-excursion here, because we're
deleting the location we want to preserve, so we have to get a bit
more creative.
  • Loading branch information
@bbatsov
bbatsov committed Dec 10, 2017
1 parent 254f17d commit c1a500d
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* [cider-nrepl#438](https://github.com/clojure-emacs/cider-nrepl/pull/438): Improve startup time by deferring loading CIDER's middleware until the first usage.
* [#2078](https://github.com/clojure-emacs/cider/pull/2078): Improve startup time by bundling together sync requests during startup.
* `cider-rotate-default-connection` will warn if you use it with only a single active connection.
* `cider-format-buffer` preserves the point position.

### Bugs Fixed

Expand Down
11 changes: 9 additions & 2 deletions cider-interaction.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1771,8 +1771,15 @@ of the buffer into a formatted string."
(let* ((original (substring-no-properties (buffer-string)))
(formatted (funcall formatter original)))
(unless (equal original formatted)
(erase-buffer)
(insert formatted))))
(let ((current-line (line-number-at-pos))
(current-column (current-column)))
(erase-buffer)
(insert formatted)
;; we have to preserve our point location in the buffer,
;; but save-excursion doesn't work, because of erase-buffer
(goto-char (point-min))
(forward-line (1- current-line))
(forward-char current-column)))))

(defun cider-format-buffer ()
"Format the Clojure code in the current buffer."
Expand Down

8 comments on commit c1a500d

@xiongtx
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right thing to do? If the buffer has n blank lines before point, cider-format-buffer would delete all but one, causing point to shift by n-1 lines.

@bbatsov
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I didn't think of this at all. My idea was to just remember the current position and restore it. Not sure what would be a good approach for the scenario you mentioned.

I'm not sure I want to go overboard with a diff-based solution like the ones linked from the issue.

@vspinu
Copy link
Contributor

@vspinu vspinu commented on c1a500d Dec 10, 2017

Choose a reason for hiding this comment

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

A slightly more precise measure would be to compute the proportion of the buffer before the erase and interpolate after the insertion.

@xiongtx
Copy link
Member

@xiongtx xiongtx commented on c1a500d Dec 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a pretty good heuristic:

(defun cider--format-buffer (formatter)
  "Format the contents of the current buffer.
Uses FORMATTER, a function of one argument, to convert the string contents
of the buffer into a formatted string."
  (let* ((original (substring-no-properties (buffer-string)))
         (formatted (funcall formatter original)))
    (unless (equal original formatted) 
      (let* ((pos (point))
             l 32
             (endp (> (+ pos l) (point-max))) 
             (snippet (thread-last (buffer-substring-no-properties pos (min (+ pos l) (point-max))) 
                        (replace-regexp-in-string "\n+" "\n+"))))
        (erase-buffer)
        (insert formatted)
        (goto-char (if endp (point-max) (point-min)))
        (funcall (if endp #'re-search-backward #'re-search-forward) snippet)
        (goto-char (or (match-beginning 0) (point-min)))
        (when (looking-at-p "\n")
          (forward-char))))))

We search for a snippet of text up to l chars long (here I've used 32) in the formatted code, replacing sequences of newlines w/ a regexp that matches one or more newlines. l need only be long enough so that it's highly unlikely to occur multiple times in the text. If point is very close to (point-max), such that the snippet may be very short, we search from the end instead.

I'm sure this can be cleaned up a little bit (e.g. to avoid the last looking-at-p "\n") but it's pretty good and much easier than a diff-based solution.

@vspinu
Copy link
Contributor

@vspinu vspinu commented on c1a500d Dec 10, 2017

Choose a reason for hiding this comment

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

need only be long enough so that it's highly unlikely to occur multiple times in the text.

Search it before and after same number of times?

@xiongtx
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean.

@bbatsov
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I was thinking of something along the same line after you flagged this commit @xiongtx, so I'm fine with this approach.

On a related note - to make this functionality more useful we should add the ability to format also region, defun and sexp I guess, to be a real alternative of what clojure-mode does.

@vspinu
Copy link
Contributor

@vspinu vspinu commented on c1a500d Dec 11, 2017

Choose a reason for hiding this comment

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

I meant that you search for the same regexp before till it gets to the current position and count the N times you did so. Then after the formatting you re-search again N times. This way you are sure that you arrived to he same position, no need for long or unique regexp.

Please sign in to comment.