wandersoncferreira / code-review

Code Reviews in Emacs
https://wandersoncferreira.github.io/code-review/
GNU General Public License v3.0
462 stars 49 forks source link

C-c C-k deletes my window #253

Open aecepogluARUP opened 8 months ago

aecepogluARUP commented 8 months ago

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

  1. Create 2 windows side by side: a left_window and a right_window.
  2. Start a code review in the left_window
  3. Press Enter to add a new comment. It'll open in the right_window
  4. C-c C-k to give up
  5. right_window is deleted

Expected behavior right_window shouldn't be deleted. I Created it. code-review should not delete a window it did not create itself.

code-review-new-buffer-window-strategy is its default: switch-to-buffer-other-window .

Desktop (please complete the following information):

SqrtMinusOne commented 7 months ago

I fixed this like that:

(defun my/code-review-comment-quit ()
  "Quit the comment window."
  (interactive)
  (magit-mode-quit-window t)
  (with-current-buffer (get-buffer code-review-buffer-name)
    (goto-char code-review-comment-cursor-pos)
    (code-review-comment-reset-global-vars)))

(with-eval-after-load 'code-review
  (advice-add #'code-review-comment-quit :override #'my/code-review-comment-quit))

That way, C-c C-k behave the same as closing a magit-status buffer.

aecepogluARUP commented 7 months ago

So is it the intended behaviour or not? If not, then we can just open a PR to address it. If it is, then it would amke sense to patch it on my end like in your example @SqrtMinusOne

SqrtMinusOne commented 7 months ago

I think it should be up to the user to decide, but I wouldn't say the current solution is strictly wrong.

Feel free to make a PR if you want.

It's just that the repo has been inactive for the last two years, the package is pretty huge, it would take quite an amount of work to make the UX as good as Magit and I'm not sure if it's worth it. And I suspect the same is the case for the author.

I'll probably just target GitLab with more limited scope if I ever try to write something like that.