mu icon indicating copy to clipboard operation
mu copied to clipboard

Possible bug in buffer switching behaviour in mu4e~headers-search-execute

Open andersjohansson opened this issue 5 years ago • 5 comments

Expected or desired behavior

Hooks added to mu4e-headers-search-hook should always be executed in the headers buffer. At least this has been my assumption about what mu4e~headers-search-execute attempts to do.

Actual behavior

For some window and frame setups, this is not the case. The buffer switching on in lines 1234-1235 in mu4e~headers-search-execute fails to put us in the headers buffer if the buffer is already visible in some window or frame. The intent of the comment doesn’t seem to match what the code does.

;; when the buffer is already visible, select it; otherwise,
;; switch to it.
 (unless (get-buffer-window buf 0)
   (switch-to-buffer buf))

Steps to reproduce

Eval:

(defun aj/mu4e-headers-hook-test (query)
  (unless (string= (buffer-name) mu4e~headers-buffer-name)
    (error "mu4e-headers: Not in headers buffer but in: %s" (buffer-name)))
  (message "It worked!"))

(add-hook 'mu4e-headers-search-hook #'aj/mu4e-headers-hook-test)

Display a mu4e headers view in one buffer. Keep this buffer visible in a window, go to another buffer in some other window and launch a search mu4e-headers-search

Versions of mu, mu4e/emacs, operating system etc.

GNU Emacs 26.2 (build 5, x86_64-pc-linux-gnu, X toolkit) of 2019-04-15 built on ajeb

mu from git at c30b9fa4

Any other detail

I tried switching to:

(if (get-buffer-window buf 0)
        (set-buffer buf)
      (switch-to-buffer buf))

which seemed to be what the comment implied would be the right thing. But this gave me some problems (double display of headers buffer) when calling (mu4e-view-message-with-message-id msgid) (through org-mu4e). However this is not encountered when just doing a search with mu4e-headers-search. I haven’t looked more deeply into this yet.

I suppose I could easily work around this by doing a with-current-buffer switch in any hook I add to mu4e-headers-search-hook. For example, mu4e~headers-clear, called a few lines below, seems to do this.

The question is: What is really supposed to happen there? Should hook functions called through mu4e-headers-search-hook not assume that they are executed in the headers buffer?

andersjohansson avatar Jun 04 '19 14:06 andersjohansson

I see the same problem, and it is still here in 1.4.1. The code in mu4e~headers-found-handler is responsible when it is using set-window-point, because that function does not affect the mu4e-header buffer in the other frame:

(if (eq (current-buffer) (window-buffer))
            (mu4e-headers-goto-message-id mu4e~headers-msgid-target)
          (let* ((pos (mu4e-headers-goto-message-id mu4e~headers-msgid-target)))
            (when pos
              (set-window-point (get-buffer-window) pos))))

If the mu4e-header buffer is visible in another frame, this ends up in moving point in the current frame's window (which displays another buffer, e.g. the org-file in which I am doing C-o on a link). The subsequent mu4e-headers-view-message then changes the current frame's windows to the mu4e-views. So, it has two problems for me

  • does not use the frame displaying the visible mu4e-headers buffer, but changes current frame's layout
  • modified point in the buffer from which I am coming

I can fix it for myself by changing mu4e-headers-found-handler to use select-window. Since I also want the frame to get the focus, I use select-frame-input-focus in addition. But I guess that this is not a generic enough solution, so I do not put it in a pull request.

(if (eq (current-buffer) (window-buffer))
            (mu4e-headers-goto-message-id mu4e~headers-msgid-target)
          (let* ((pos (mu4e-headers-goto-message-id mu4e~headers-msgid-target))
		 (win (get-buffer-window (mu4e-get-headers-buffer) 0)))
            (when pos
	      (select-frame-set-input-focus (window-frame win))
	      (select-window win)
	      (goto-char pos)
	      )))

I work with emacs workgroups and I always have one frame dedicated to a workgroup displaying mu4e. This bug had made it tough working with links, and I am happy to now have the workaround.

A big thanks for mu/mu4e... this has revolutionized my mail interaction in the last few years.

dfeich avatar Apr 21 '20 16:04 dfeich

@andersjohansson: The comment in the code is misleading indeed, but mu4e-headers-search-hook doesn't really need the current buffer to be the in-progress headers-buffer, or? There's nothing it can reasonably do there.

@dfeich: let me try & play a bit with that patch.

djcb avatar May 20 '20 09:05 djcb

@andersjohansson: The comment in the code is misleading indeed, but mu4e-headers-search-hook doesn't really need the current buffer to be the in-progress headers-buffer, or? There's nothing it can reasonably do there.

@djcb, I should have described my real use-case from the beginning, about the things I reasonably (or unreasonably?) wanted to do in the headers buffer :slightly_smiling_face:. I wanted to be able to set different headers for different maildirs (some of my accounts don’t use tags for example). For this reason I added this function to mu4e-headers-search-hook:

(defun aj/mu4e-set-headers-fields (query)
  "Set ‘mu4e-headers-fields’ depending on which maildir is viewed."
  (when (buffer-live-p (mu4e-get-headers-buffer))
    (with-current-buffer (mu4e-get-headers-buffer)
      (setq mu4e-headers-fields
            (plist-get
             aj/mu4e-headers-fields
             (when-let* ((mdm
                          (s-match-strings-all
                           "maildir:\\(?2:\\\"\\(?1:[^\\\"]+\\)\\\"\\|\\(?1:[^[:space:]]+\\)\\)"
                           query))
                         (maildirs (mapcar #'cadr mdm)))
               (cond
                ((--all? (string-match-p "^/work" it) maildirs)
                 :work)
                ((--all? (string-match-p "^/\\(gmail1\\|gmail2\\)" it) maildirs)
                 :gmail))))
            header-line-format (mu4e~header-line-format)))))

Where aj/mu4e-headers-fields is plist with some different header field configs for different maildirs (with the default config for multi-maildir-searches stored under nil, so that's returned when the other conditions fail). Apparently I added the lines

(when (buffer-live-p (mu4e-get-headers-buffer))
    (with-current-buffer (mu4e-get-headers-buffer)

as a workaround after reporting this bug.

andersjohansson avatar May 20 '20 14:05 andersjohansson

Ah, but setting mu4e-headers-fields can happen before actually to the headers view, or? Did you get any errors?

djcb avatar May 20 '20 14:05 djcb

Yes, that’s done in mu4e-headers-mode, which is activated earlier in mu4e~headers-search-execute. Maybe my solution has never worked... But I do think it did some year back 🤔. I don’t really use it much now, since I mostly only use one account on my current computer and setup.

Anyway. I shouldn’t expect to be able to do that thing there. Maybe there is a case to be made for the possibility to dynamically change the used headers. Maybe in another earlier hook mu4e-headers-search-pre-hook :slightly_smiling_face:? But I’m not arguing strongly for this, if you want to avoid the complexity.

andersjohansson avatar May 20 '20 15:05 andersjohansson

Okay thanks... not currently planniing anything for this, closing this..

djcb avatar Jan 17 '23 16:01 djcb