Window configuration changes on format

[ntBre]

I just updated rust-mode and noticed that calling rust-format-buffer is now changing my window configuration, namely deleting one of my windows. This appears to be a result of #476 because reverting that change fixes my issue.

Personally, I think that issue is better fixed by setting rust-format-show-buffer to nil, which prevents the extra window from being created in the first place.

Otherwise, if I'm following the code correctly, the kill-buffer-and-window call should only be in the (= ret 3) case right? I think the current code is assuming that the 0 return value is coming after a non-zero return, which is what created the window in the first place. So if you really want to keep the kill-buffer-and-window call, it needs to be gated behind a check for an existing error window, like in the Go code linked in the original issue (#475).

Replacing the kill-buffer call to one adapted from the Go version also seems to work:

(let ((win (get-buffer-window rust-rustfmt-buffername)))
  (if win
      (quit-window t win)
    (kill-buffer rust-rustfmt-buffername)))

[micl2e2]

@ntBre Hi!

I think we can discuss more details about your explanation above: >

Otherwise, if I'm following the code correctly, the kill-buffer-and-window call should only be in the (= ret 3) case right? I think the current code is assuming that the 0 return value is coming after a non-zero return, which is what created the window in the first place

The kill-buffer-and-window call aims to delete the window that is created when "rustfmt" program detects the error, that is, when the return value of "rustfmt" subprocess is any value except 0, including the situation of (= ret 3).

Adding this line in conditional branch of (zerop ret) is because when you got errors, most of time you are about to fix it, after that fix, the code in rust–format-call will run into (zerop ret) conditional branch, the previously created "*rustfmt*" buffer will still exist then. That's why we need to put the killing window code under the condition of (zerop ret) instead of (= ret 3).

[ntBre]

Thanks for the additional explanation! I guess I was off base on that speculation, but I did manage to submit a PR that fixed the issue I was having, so I'll close this too.

[micl2e2]

And about the issue you've mentioned:

calling rust-format-buffer is now changing my window configuration, namely deleting one of my windows.

In fact, I've tested the solution adopted by "go-mode", that is, do some checks before deleting the window, it turns out it is somewhat unnecessary.

This is why: the newly popped up window is dedicated for the error message displaying, except which there should be no other usage, if so, someone should create a seperate window by themselves, rather than reusing the dedicated window created by "rust-mode" package.

And in addition, that check comes with run-time cost, hence I think it's fine to delete the corresponding window directly, instead of like what "go-mode" does.

So, may I have more specific information about this issue? Or some configurations to reproduce the problematic situation? Thanks! @ntBre

[ntBre]

I don't have any other information. After your change in #475, every time I call rust-format-buffer, one of my windows (not related to rust-mode or rust-format, just any window I have open other than the current window) gets deleted. Before your change I never had this issue, and after incorporating the code from my PR, which was merged, I also don't have the issue. Personally I would be happy with a revert of your change and my fix to it, if you think this "run-time cost" is too high. And more generally I would prefer such changes not to get merged and disrupt my working configuration for no reason.

Clearly if you want to keep your delete-buffer-and-window call for some reason, you need to check that the current buffer is in fact the format error buffer, as my change does. Your code indiscriminately kills the active buffer, even if the user has rust-format-show-buffer set to nil as I do because I literally never want to see the format errors in a buffer.

Again, I think the actual fix to your original problem was setting rust-format-show-buffer to nil instead of breaking my working configuration with your PR. In that case, both of our changes should be reverted.

[micl2e2]

@ntBre: I never add code to indiscriminately kill the active buffer, kill-buffer already exists(link). Both of us were making the changes to the behavior of the buffer's corresponding window, not buffer itself.

The existence of kill-buffer in the code might has somehow shown the necessity of dealing with the existing "*rustfmt*" buffer after a successful rustfmt's format action, and the default-on variable rust-format-show-buffer might also has shown the necessity of the presence of the "*rustfmt*" buffer.

And it is rarely seen to modify a package-wide, user-customizable variable inside the package code(correct me if I was wrong), so I really think adding the line

(setq rust-format-show-buffer nil)

to rust-rustfmt.el is less than appropriate. And using a separate buffer to show errors might not fit your needs, but indeed fit others, otherwise would it be ON by default in other languages' mode as well? (for example)

I think we all are patient enough to wait for the information to reproduce the problem. I provided those information in #475, I'm sure you will do so as well in this thread. And this is not about keeping your lines or mine, but keeping the project open source as much as possible. I am very happy to see that someone other than me would propose a better solution than #476 or #479, as long as the changes is inclusive and open enough.

[ntBre]

Fine, I got loose with my buffer/window terminology. You indiscriminately kill the active window. Let me show you what happens with your version of the code:

This is recorded with emacs -Q and the tiny configuration (loading rust-mode) that you see on the right at the start. So this has nothing to do with my configuration. Actually your configuration must be the weird one if this actually works for you.

And it is rarely seen to modify a package-wide, user-customizable variable inside the package code(correct me if I was wrong), so I really think adding the line

(setq rust-format-show-buffer nil)

This part borders on trolling. Obviously I was suggesting that you should have set this in your config file instead of filing a PR in the first place.

I think we all are patient enough to wait for the information to reproduce the problem.

Clearly we're not patient enough. My change was already merged. That's the deeper problem I'm seeing here. I can't remember the last time I've had such a large issue from updating my packages, but my Rust development setup became unusable from your 1-line change, which obviously looked reasonable at first glance. If you're going to keep spamming tiny commits on this repository, maybe you should try to add a test suite to verify that you aren't breaking things for other users.

@brotzeit could you weigh in on this thread? I still vote for reverting both of our changes to the code that was already working in the first place. On a related note, what are the plans for migrating to rust-ts-mode? It seems the only thing I'm missing in rust-ts-mode is this formatting behavior we've been discussing here. Obviously I could paste the formatting code into my own config and delete rust-mode for good, but I was hoping there would be a smoother union between the two.

[micl2e2]

First thing first, now I got what your "breaking" means, according to your GIF, this is due to my previous change and indeed needs a fix, and go-mode's way is sufficient to fix it, so now I totally agree that your change is meaningful and needs not to be reverted.

Other than that, your wording makes me so disappointed. Maybe I should clarify again: I did not mean to be snide in any way, the very first comment I posted has mentioned that all I want is discussion, how could you treat it as internet trolling?

And I don't think Emacs has taught people to be impatient and rush to judge anyone, no matter you are patient enough, I still am anyway. Maybe something has triggered you, but I believe that's none of my business. If you think my intention to discussion is meaningless, I am fine to stop making any comments but listen, especially as you mentioned, listening to the comments from @brotzeit .

[ntBre]

I think my initial report was sufficient for discussion, and my fix was already merged, so I've been quite annoyed to receive further emails about this issue. I'm glad I finally convinced you that the fix was needed.

As for the trolling accusation, I just don't see how anyone could interpret a suggestion to set a variable as meaning inside the package itself. The variable is exposed so that you can set it in your own config. I thought I made this extra clear with phrasing like "if the user has rust-format-buffer set." In this same vein, I think your original change could have been handled as advice on rust-format-buffer. Something like

(advice-add 'rust-format-buffer :after
        (lambda (_)
          (kill-buffer-and-window)))

is what I would have tried. Emacs is extensible for a reason, and when I submit issues or PRs, I have usually exhausted my user-facing options. In fact, I was hoping to do that here, but it seemed silly just to run winner-undo after rust-format-buffer when I could trace the issue directly to your commit.

It also felt like you were ignoring the important parts of my comments and focusing on irrelevant details. For example, in the first comment, you discuss which cond case is entered, which I did mention in the original issue, before I dove into the code. However, if you look at my actual PR, you'll see that I didn't have to mess with the cases at all and must have figured that out on my own. As a second example, you zoomed in on my buffer/window mixup in the second part of that comment and ignored the most important part in the first sentence: "every time I call rust-format-buffer, one of my windows [...] gets deleted." I know a picture (GIF in this case) is worth a thousand words, but I really thought such a simple description in the first issue and that comment would suffice. I went to the trouble of reproducing it with emacs -Q, which I'm assuming you did not try.

Anyway, I didn't want to hurt your feelings. I was very annoyed when I updated rust-mode and my Rust development was immediately and painfully disrupted, and I was further annoyed when I received additional notifications on a closed issue for which a PR was already merged. If I were the maintainer of this package, I would have been more diplomatic throughout, but as a very minor contributor I didn't expect to have an in-depth design discussion over a four-line change and may have been harsher than necessary.