code-review icon indicating copy to clipboard operation
code-review copied to clipboard

C-c C-k deletes my window

Open aecepogluARUP opened this issue 1 year ago • 3 comments

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):

  • code-review 20230724.2927 on Emacs 29.1

aecepogluARUP avatar Dec 18 '23 15:12 aecepogluARUP

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.

SqrtMinusOne avatar Jan 15 '24 22:01 SqrtMinusOne

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

aecepogluARUP avatar Jan 17 '24 16:01 aecepogluARUP

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.

SqrtMinusOne avatar Jan 18 '24 23:01 SqrtMinusOne