search

djcb / mu

maildir indexer/searcher + emacs mail client + guile bindings
http://www.djcbsoftware.nl/code/mu
GNU General Public License v3.0
1.59k stars 384 forks source link

mu4e-message-kill-buffer doesn't restore previous window configuration #2600

Open thierryvolpiatto opened 7 months ago

thierryvolpiatto commented 7 months ago

Describe the bug

When killing composition buffer (C-c C-k) previous window configuration is not restored.

How to Reproduce

1) Open an Email from mu4e headers buffer. You have now two windows, the headers window and the view buffer where your mail is. 2) Hit "W" or "R" to reply, you have now the headers window (same as before) and in place of the view buffer the composition buffer. 3) Change your mind and hit C-c C-k. You have now only the headers window. This because mu4e-message-kill-buffer set the view buffer before killing the composition buffer and also it check for windows that are no more existing.

What is expected is to be back to 1) when hitting C-c C-k.

The following patch is fixing it:

diff --git a/mu4e/mu4e-compose.el b/mu4e/mu4e-compose.el
index 7ceb365b..836d2072 100644
--- a/mu4e/mu4e-compose.el
+++ b/mu4e/mu4e-compose.el
@@ -461,14 +461,14 @@ It attempts to restore some mu4e window layout after killing the
 compose-buffer."
   (interactive)
   (let ((win (selected-window))
-        (view (mu4e-get-view-buffer))
+        ;; `mu4e-get-view-buffer' makes the view buffer current.
+        (view (save-selected-window (mu4e-get-view-buffer)))
         (hdrs (mu4e-get-headers-buffer)))
     (message-kill-buffer)
-    (when (window-live-p win) (delete-window win))
     ;; try to go back to some mu window if its live; otherwise do nothing.
-    (if (and (buffer-live-p view) (window-live-p (get-buffer-window view)))
+    (if (buffer-live-p view)
         (switch-to-buffer view)
-      (when (and (buffer-live-p hdrs) (window-live-p (get-buffer-window hdrs)))
+      (when (buffer-live-p hdrs)
         (switch-to-buffer hdrs)))))
 
 ;;; Crypto

Environment

LinuxMint and Emacs-29.1

Checklist

Please make sure you all items in the checklist are set/met before filing the ticket. Thank you!

thierryvolpiatto commented 7 months ago

I guess there is a similar issue as well when sending mail (instead of C-c C-k), but this time I always end up with 3 windows: The headers window, a window handling previous buffer used before opening Mu4e, and the view buffer. Didn't find how to fix it yet.

thierryvolpiatto commented 7 months ago

I guess there is a similar issue as well when sending mail (instead of C-c C-k), but this time I always end up with 3 windows: The headers window, a window handling previous buffer used before opening Mu4e, and the view buffer.

This happen after tag 1.11.24, I will try to bissect further to find the offending commit.

thierryvolpiatto commented 7 months ago

Culprit is commit 8abf0983

djcb commented 7 months ago

Ah, thanks for checking.

There's a bit of messy massaging around the way message does its thing, and there is still some more to do...

djcb commented 7 months ago

Ah, hadn't even noticed the new behavior. Anyway, the proposed change seems to do the job, I'll push a fix soon. Thanks!

thierryvolpiatto commented 6 months ago

"Dirk-Jan C. Binnema" @.***> writes:

Ah, thanks for checking.

There's a bit of messy massaging around the way message does its thing, and there is still some more to do...

Perhaps we could open a new bug report about this?

To resume, I have on my main frame two buffers, one is the mu4e-headers buffer and the other the mu4e-view buffer, then I reply to this email, I have now the mu4e-headers buffer and the mu4e-compose buffer. When I send my email I expect the mu4e-compose buffer to be killed once sent and my previous window configuration with mu4e-headers and mu4e-view buffers to be restored, instead I end up with 3 windows, the mu4e-headers on top, a previous buffer just under it (scratch here) and the mu4e-view at bottom.

I could fix it temporarily with the patch below but it is really bad and may work only for my usage (windows) as it seems people are using many different window configs (frames, only one window etc...). Thus the function mu4e--compose-setup is overriding several message-*-actions hook which prevent users to fix these issues themselves without modifying source as I did. Also I had to delay with a timer the windows restoration. I send you here the patch which at least is good to localize the problem and fix it in a better way.

Thanks.

diff --git a/mu4e/mu4e-compose.el b/mu4e/mu4e-compose.el index 4a91a10d..1512c62d 100644 --- a/mu4e/mu4e-compose.el +++ b/mu4e/mu4e-compose.el @@ -748,20 +748,15 @@ Is this address yours?" (set-buffer-modified-p nil) (undo-boundary))

-(defun mu4e--maybe-delete-frame ()

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.*Message ID: @.***>

-- Thierry

djcb commented 6 months ago

I'll reopen this one...

djcb commented 4 months ago

FWIW, things work fine for me when sending / cancelling a message composition if I have just the headers / view message (ie., the canceled composition is replaced with the message-view I started with)

However, when there's another window, cancelling the composition strangely changes the window to the headers view (so I have two now). Have to find what causes the difference.

thierryvolpiatto commented 4 months ago

"Dirk-Jan C. Binnema" @.***> writes:

  1. ( ) text/plain (*) text/html

FWIW, things work fine for me when sending / cancelling a message composition if I have just the headers / view message (ie., the canceled composition is replaced with the message-view I started with)

Same for me but in addition a third window is created between header and view windows. This window is displaying the last buffer used before staring Mu4e.

However, when there's another window, cancelling the composition strangely changes the window to the headers view (so I have two now). Have to find what causes the difference.

Here what I am using since 1 or 2 months now (but it is still not the right fix IMO even if working fine for me, see FIXME below):

commit 7040590b2a8b62e3f509000c7c1ed201b3633483 Author: Thierry Volpiatto @.***> Date: Sun Jan 7 14:57:54 2024 +0100

Restore frame config after sending email

diff --git a/mu4e/mu4e-compose.el b/mu4e/mu4e-compose.el index 4a91a10d..6377f11c 100644 --- a/mu4e/mu4e-compose.el +++ b/mu4e/mu4e-compose.el @@ -748,21 +748,11 @@ Is this address yours?" (set-buffer-modified-p nil) (undo-boundary))

-(defun mu4e--maybe-delete-frame ()

@@ -782,23 +772,21 @@ Optionally, SWITCH determines how to find a buffer for the message (mu4e-message-at-point))) (mu4e-compose-parent-message parent) (mu4e-compose-type compose-type)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.*Message ID: @.***>

-- Thierry

djcb commented 4 months ago

Hmm, can't reproduce the erroneous behavior today. Perhaps something changed in emacs itself?

Ie., I have a headers-view window and an unrelated window. In the headers-view, I view, then reply to some message R; then close it C-c C-k. And we're back at the beginning. Not sure what changed.

There's still some issue with deleting frames though.

djcb commented 2 months ago

Following the steps:

How to Reproduce

    Open an Email from mu4e headers buffer.
    You have now two windows, the headers window and the view buffer where your mail is.
    Hit "W" or "R" to reply, you have now the headers window (same as before) and in place of the view buffer the composition buffer.
    Change your mind and hit C-c C-k.
    You have now only the headers window. This because mu4e-message-kill-buffer set the view buffer before killing the composition buffer and also it check for windows that are no more existing.

this works fine for me (with 1.12.4). For you too?

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

Following the steps:

How to Reproduce

Open an Email from mu4e headers buffer.
You have now two windows, the headers window and the view buffer where your mail is.
Hit "W" or "R" to reply, you have now the headers window (same as before) and in place of the view buffer the composition buffer.
Change your mind and hit C-c C-k.
You have now only the headers window. This because mu4e-message-kill-buffer set the view buffer before killing the composition buffer and also it check for windows that are no more existing.

this works fine for me (with 1.12.4). For you too?

No it doesn't, always the same behavior.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

thierryvolpiatto commented 2 months ago

Thierry Volpiatto @.***> writes:

"Dirk-Jan C. Binnema" @.***> writes:

Following the steps:

How to Reproduce

Open an Email from mu4e headers buffer.
You have now two windows, the headers window and the view buffer where your mail is.
Hit "W" or "R" to reply, you have now the headers window (same as before) and in place of the view buffer the composition buffer.
Change your mind and hit C-c C-k.
You have now only the headers window. This because

mu4e-message-kill-buffer set the view buffer before killing the composition buffer and also it check for windows that are no more existing.

this works fine for me (with 1.12.4). For you too?

No it doesn't, always the same behavior.

Further more, I now endup with three windows after replying, Mu4e is now only usable with patch https://github.com/thierryvolpiatto/mu/commit/94b7666e0f58a917e3fe7de2f499573ed2faa708 applied (probably this patch doesn't apply anymore on top of master without resolving conflict).

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

djcb commented 2 months ago

Hmm, trying to repro the original "How to reproduce" scenario here.

Start emacs/mu4e (change paths as needed):

 emacs -Q --eval "(progn (add-to-list 'load-path \"~/Sources/mu/build/mu4e\") (setq mu4e-mu-binary \"~/Sources/mu/build/mu/mu\") (require 'mu4e) (mu4e))"

bu to go to some unread mails. Press R. Press C-c C-k works as expected.

I tried with emacs master and emacs 28.2; both work for me.

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

  1. ( ) text/plain (*) text/html

Hmm, trying to repro the original "How to reproduce" scenario here.

Start emacs/mu4e (change paths as needed):

emacs -Q --eval "(progn (add-to-list 'load-path \"~/Sources/mu/build/mu4e\") (setq mu4e-mu-binary \"~/Sources/mu/build/mu/mu\") (require 'mu4e) (mu4e))"

bu to go to some unread mails.

RET on a unreaded mail, now you have two windows, and only then press R or W.

Press R. Press C-c C-k works as expected.

I tried with emacs master and emacs 28.2; both work for me.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

djcb commented 2 months ago

Yeah, same in that case; two windows, I press R, the view window is replaced by the composition window. I press C-c C-k, and the composition goes away and we're back at the view window, just like before pressing R.

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

  1. ( ) text/plain (*) text/html

Yeah, same in that case; two windows, I press R, the view window is replaced by the composition window. I press C-c C-k, and the composition goes away and we're back at the view window, just like before pressing R.

I tried your recipe and indeed C-c C-k is restoring the previous window conf, so I guess something in my setting is modifying this behavior, I checked and didn't find why, I have no code or variable directly modifying the window configuration behavior but because we are relaying on Gnus, I believe modifying either gnus- or message- or mm-* vars is modifying some piece of gnus code hence the behavior I have. This mean anyway that the current code is fragile and may fail in any other corner cases. I suggest we determine how you are currently fixing such issue in your code to be able to fix it, seems it is unclear currently.

Also I opened a new issue (#2676) that concern not only window restoration after C-c C-k but also after sending, and with your recipe it is still not working, I endup with three windows after sending, so I guess you should either consider this in #2600 (seems you don't) or reopen #2676. The two issues are related, the good way to fix this IMO is to have common code that fix both issues.

Hopefully, with last master I can advice mu4e--compose-setup without having to maintain a working branch. I still think saving and restoring frame/window conf is the right way to do.

Without such advice mu4e is not usable for me actually (really annoying issue).

(defun tv:advice-mu4e--compose-setup (compose-type compose-func &optional switch)
  (cl-assert (member compose-type '(reply forward edit new)))
  (unless (mu4e-running-p) (mu4e 'background)) ;; start if needed
  (let* ((parent
          (when (member compose-type '(reply forward edit))
            (mu4e-message-at-point)))
         (mu4e-compose-parent-message parent)
         (mu4e-compose-type compose-type)
         (frameconf (current-frame-configuration)))
    (advice-add 'message-is-yours-p :around #'mu4e--message-is-yours-p)
    (run-hooks 'mu4e-compose-pre-hook) ;; run the pre-hook. Still useful?
    (mu4e--context-autoswitch parent mu4e-compose-context-policy)
    (with-current-buffer
        (mu4e--compose-setup-buffer compose-type compose-func parent)
      (unless (eq compose-type 'edit)
        (set-visited-file-name ;; make it a draft file
         (mu4e--draft-message-path (mu4e--message-basename) parent)))
      (mu4e--compose-setup-post compose-type parent)
      (funcall (or switch (mu4e--compose-switch-function)) (current-buffer))
      (let ((actions (list
                      (lambda ()
                        (set-frame-configuration frameconf)))))
        ;; handle closing of frames.
        (setq-local ;;message-kill-actions actions
         message-return-actions actions
         message-send-actions actions
         message-kill-actions actions))
      (current-buffer))))

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

djcb commented 2 months ago

Both sending and cancelling messages work fine for me, if it were "really annoying" surely I'd notice.

mu4e is at the mercy of what gnus gives up and the abstraction is not fully watertight, so perhaps you found something that influences things in a way that bubbles up.

I'm planning to rework the composition code a bit, so whatever your advising likely will go away. But hopefully it's not necessary anymore.

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

  1. ( ) text/plain (*) text/html

Both sending and cancelling messages work fine for me, if it were "really annoying" surely I'd notice.

Not necessarily, we have all our own workflow and we sometimes miss obvious bugs or misfeatures.

mu4e is at the mercy of what gnus gives up and the abstraction is not fully watertight, so perhaps you found something that influences things in a way that bubbles up.

Exactly.

I'm planning to rework the composition code a bit, so whatever your advising likely will go away. But hopefully it's not necessary anymore.

Great, looking forward for this.

Thanks.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

theophilusx commented 2 months ago

Another data point regarding new behaviour in latest version.

I'm often seeing a read/view buffer pop back up without being requested. I think I've narrowed down the exact steps required to reproduce.

  1. Open my inbox (SPC j i in my case)
  2. Move to some message in the header view
  3. Hit on the message to read it
  4. Hit 'q' whil in the message buffer to exist and return to the header view
  5. Mark the message in some way i.e. 'd' to delete it
  6. Hit 'x' to apply marks

at this point, the view buffer for the previously viewed message pops up again.

I then hit 'q' to close the view buffer and then hit 'q' again to exit the headers view and return to the main mu4e window.

I'm running Emacs 30.0.50 on Fedora 39 under either hyprland (Wayland) or i3 (X). I use meow.el, so modal editing. I have no frame or window settings in Emacs and don't use anything like winnum or any other window management packages. Essentially, I start emacs as a daemon and then use emacsclient full scren frame. I have a dedicatged virtual desktop for mu4e. When I want to do other stuff, I move desktop workspace (virtual desktop) and open a new frame.

The popping up of the view buffer after applying marks is a little irritating, but no big deal. It is behaviour I only noticed fairly recently. However, as I run both Emacs and mu4e from git HEAD, really don't know if this new behaviour is due to changes in Emacs or changes in mu4e.

Note that I see the same behaviour with Emacs build with standard GTK (for X) and with the pure or pgtk variant (for Wayland).

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

  1. ( ) text/plain (*) text/html

Both sending and cancelling messages work fine for me, if it were "really annoying" surely I'd notice.

mu4e is at the mercy of what gnus gives up and the abstraction is not fully watertight, so perhaps you found something that influences things in a way that bubbles up.

I'm planning to rework the composition code a bit, so whatever your advising likely will go away. But hopefully it's not necessary anymore.

I think I found the cause of the bug; Basically, this is because at one point you are using switch-to-buffer to make a buffer current inside several with-current-buffer and even from a with-temp-buffer which return another buffer!!! (instead of the temp buffer!), this shuffle the window history and after each operation emacs lost what was the underlying buffer. Thus this make the code really hard to read, debug and follow, even more because message/gnus functions are involved from closures (and sometimes advised) and the reader has to follow this.

Probably once this will be cleaned we will see clearly what's wrong.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry

djcb commented 2 months ago

Yeah, I'm trying to make that convoluted path a bit less so; however, fundamentally mu4e handles buffer display, and it takes that task over from the Gnus code. That way, it can display buffers the way that mu4e does that, and support display-buffer etc.

Anyway, let's see how things work after changes.

thierryvolpiatto commented 2 months ago

"Dirk-Jan C. Binnema" @.***> writes:

Yeah, I'm trying to make that convoluted path a bit less so; however, fundamentally mu4e handles buffer display, and it takes that task over from the Gnus code. That way, it can display buffers the way that mu4e does that, and support display-buffer etc.

Anyway, let's see how things work after changes.

Seems better now with mu4e-compose-post-hook (44405940).

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Thierry