Fix window scrolling after rustfmt is run

[apiraino]

Hello,

I tried investigating the window scrolling after rustfmt is run (this issue has been referenced in various forms in issues #268 #231 #251) as I am still experiencing it and imo it's pretty annoying.

I'm not an elisp expert, but I think a quick fix/workaround could be to simply nail the window start position and restore it after rustfmt has finished.

I think there may be still cases where the code reformatting can be a bit confusing (f.e. rustfmt'ing a big messed up file), but at least I'm not getting mad every time I compulsively save a buffer :-)

Can you let me know what you think of this little patch? I tested it a bit and it has greatly improved my coding experience with rustfmt.

Thanks for your time!

EDIT: I did some checks, but I'm not sure how to fix those warning that make the CI fail
EDIT2: maybe fixed (not sure if fix is correct though)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jjwest]

Works nicely for me. I'd suggest using a let binding instead of defvar (which creates a persistent global variable):

(let ((w-start (window-start)))
 .....
)

[apiraino]

@jjwest thanks, appreciated! I've pushed a patch to reflect your suggestion.

[Dushistov]

Issue #256 looks like also related

[Dushistov]

@apiraino With this change spontaneous still exists, but at now jump not to begining of buffer,
not at the end, as was in #256 before this change.

[drrlvn]

Isn't this is the reason that save-excursion exists? I think it should be possible to just use:

(save-excursion
    (rust--format-call (current-buffer)))

[sebastiencs]

@drrlvn save-excursion uses markers that might be deleted by rust--format-call, see https://emacs.stackexchange.com/a/7577/15635

[drrlvn]

@sebastiencs I see.

I tried looking at how clang-format does this as I've not had this issue in C++ code and it looks like the cursor position is passed as an argument and then the json output contains the location the cursor should be moved to. See the source.

Is there such an option in rustfmt, and if not would it maybe be possible to add it? I imagine every editor would need something like this.

[apiraino]

review @nikomatsakis? anyone? suggestions?

thanks

[brotzeit]

I think I finally found a solution. Instead of using stdin, I save the buffer, run rustfmt on the file and revert the buffer. It would be great if somebody could confirm this actually works with rustic.

[nikomatsakis]

This looks like a pretty simple patch :)

[apiraino]

@nikomatsakis I'm sorry I had lost track of this issue :-) you mean this patch can be pushed forward? I think I've been using it since.

[pnkfelix]

Ill try to take a look at this this week.

It would be interesting to try to figure out how to make a regression test for this ...

[nikomatsakis]

@apiraino seems ok to me to merge in this case. I don't really know how to judge if it will work, but I take your word for it. =) Would of course be nice to have a test, as @pnkfelix says.

[apiraino]

@nikomatsakis thanks for having a look at this patch. I can verify the effect of the patch on a sample Rust source file. However I'm afraid I can't provide an elisp test because my knowledge on that is pretty limited. If anyone is willing to help, I'd be really happy to provide a test case for the regression.

In any case here's how to reproduce:

1. Ensure you have installed rust-mode from melpa (currently rust-mode-20181008.1628.el)
2. Download this gist
3. Open the downloaded file in emacs
4. Follow the instructions in the Rust test file
5. You should experience that the buffer moves at the end of the current window

With my patch step (5) does not happen

Hope this helps

[brotzeit]

@apiraino You have to install rust and rustfmt in the travis config first. Then you can start with a simple test that formats a buffer. Afterwards you can write the regression test with save-some-buffers.
You can take a look at https://github.com/brotzeit/rustic/blob/master/.travis.yml.

[apiraino]

@brotzeit sorry for the long hiatus, got a lot going on.

So, I did a little reseach and tried writing a test for this PR. I believe what I need to test is the window-start value before and after running rust-format-buffer.

I tried allocating a temporary buffer as in other tests (with-temp-buffer) but window-start always return nil. I'm not sure I'm on the right path.
How should the test work in order to get meaningful values for window-start?

Here's a gist of what I am at this moment:
https://gist.github.com/apiraino/4c8abac8d23398b2bd6dcd5d3d54e1df

[nikomatsakis]

Sorry for long hiatus. I'm kind of overwhelmed with review requests and haven't really had time to try this out.

Maybe @mookid or @brotzeit can review?

[mookid]

Hi @apiraino,
Looks good.

About the test, the problem is with the let:

(defun rust-test-fn (code expected-result)
  (with-temp-buffer
    (rust-mode)
    (insert code)
    (message "%S" (selected-window))
    ;; check window before formatting
    (let ((w-start (window-start)))
      (message "before: %s" w-start))
    (rust-format-buffer)
    (should (equal expected-result (buffer-string)))

    ;; check window start after formatting
    (let ((w-start (window-start)))
      (message "%s" w-start))))

I will merge once the test is checked in.

[truchi]

Hi,

Any news on this ?

Thank you !

[mookid]

Integrated, thanks. Sorry for the long delay! :)