Potential bug in the logic of rmail-select-summary

Potential bug in the logic of rmail-select-summary

[Göktuğ Kayaalp]

Hello,

When both an Rmail buffer and it’s summary buffer are displayed
simultaneously, when something triggers rmail-show-message when
navigating inside the Rmail buffer, the Rmail buffer is replaced with
the summary buffer.  E.g., assume we have an mbox called ‘current’, and
‘X’ is the active cursor:

1. window setup: [ current-summary ] [ current X ]
2. hit ‘n’, i.e. rmail-next-undeleted-message
3. resulting window setup: [ current-summary ] [ current-summary X ]

This is not the case when ‘current’ is the sole buffer, when I navigate
messages the window keeps on displaying ‘current’ as expected.

rmail-next-undeleted-message calls rmail-show-message which triggers
rmail-select-summary, which has the following logic in it:

- if rmail-summary-displayed
  - rmail-pop-to-buffer rmail-summary-buffer ...
- else
  - with-current-buffer rmail-summary-buffer ...

Which IMHO is the opposite of what needs to happen.  If the summary
buffer is already displayed, why pop to it again?

When I flip the then and else clauses around like below, it behaves like
I expect it to.  Tho I’m not sure if the original behaviour is actually
wrong or not.  Am I not understanding what it’s supposed to do, or is
this a bug?

One caveat of my little ‘fix’ is the point is not updated in the summary
buffer, which results in the annoying behaviour where when I switch to
the summary buffer highlighted msg is not the one in the rmail buffer
but the last one I moved to in the summary buffer.

FWIW the exact thing I want to do in terms of usage is to just see the
rmail buffer and the associated summary buffer simultaneously.


Best,

        -Göktuğ.


(defmacro rmail-select-summary (&rest body)
  `(let ((total rmail-total-messages))
     (if (rmail-summary-displayed)
         (with-current-buffer rmail-summary-buffer
	   (let ((rmail-total-messages total))
	     ,@body))
       (let ((window (selected-window)))
	 (save-excursion
	   (unwind-protect
	       (progn
		 (rmail-pop-to-buffer rmail-summary-buffer)
		 ;; rmail-total-messages is a buffer-local var
		 ;; in the rmail buffer.
		 ;; This way we make it available for the body
		 ;; even tho the rmail buffer is not current.
		 (let ((rmail-total-messages total))
		   ,@body))
	     (select-window window)))))
     (rmail-maybe-display-summary)))

-- 
İ. Göktuğ Kayaalp / @cadadr / <https://www.gkayaalp.com/>
pgp:   024C 30DD 597D 142B 49AC 40EB 465C D949 B101 2427

Re: Potential bug in the logic of rmail-select-summary

[Eli Zaretskii]

> From: Göktuğ Kayaalp <self@gkayaalp.com>
> Date: Tue, 19 Jan 2021 15:23:02 +0300
> 
> When both an Rmail buffer and it’s summary buffer are displayed
> simultaneously, when something triggers rmail-show-message when
> navigating inside the Rmail buffer, the Rmail buffer is replaced with
> the summary buffer.  E.g., assume we have an mbox called ‘current’, and
> ‘X’ is the active cursor:
> 
> 1. window setup: [ current-summary ] [ current X ]
> 2. hit ‘n’, i.e. rmail-next-undeleted-message
> 3. resulting window setup: [ current-summary ] [ current-summary X ]

In what version of Emacs do you see that?

FWIW, I cannot reproduce this, neither in Emacs 27 nor in Emacs 28.
Does this happen in "emacs -Q"?  (I use Rmail all the time, and I
always have both the mbox file and its summary displayed, so if this
would be happening to me, I'd sure have noticed.)

Re: Potential bug in the logic of rmail-select-summary

[Göktuğ Kayaalp]

On 2021-01-19 16:50 +02, Eli Zaretskii <eliz@gnu.org> wrote:
> In what version of Emacs do you see that?

Build of 36d33776c21b3765b8a611f09ae7d86417abee8a, within last few days.

> FWIW, I cannot reproduce this, neither in Emacs 27 nor in Emacs 28.
> Does this happen in "emacs -Q"?  (I use Rmail all the time, and I
> always have both the mbox file and its summary displayed, so if this
> would be happening to me, I'd sure have noticed.)

Indeed, I should’ve known better and tested with -Q.  I’m sorry about
that.

So apparently the problem was caused by some interaction with the
following bit of config:

 display-buffer-alist
 '(("\\*Quail Completions" . (display-buffer-in-side-window))
   ("\\*.*Completions\\*" . (display-buffer-in-side-window . ((side . bottom))))
   ("\\*Help\\*" . (display-buffer-reuse-window))
   ("Checkdoc" . (display-buffer-pop-up-window))
   ("Calendar" . (display-buffer-in-side-window . ((side . bottom))))
   ("help\\[R\\]" . (display-buffer-pop-up-window))
   (".*" . (display-buffer-same-window)))

When I commented out that last line it worked.  FWIW, I added the
following advice which seems to help:

(define-advice rmail-show-message
    (:around (fn &rest args) inhibit-display-buffer-alist)
  (let ((display-buffer-alist nil))
    (apply fn args)))

Is this an expected interaction or a bug?  AFAIU rmail-pop-to-buffer
could use some form of display-buffer-* to avoid going through
display-buffer-alist.

-- 
İ. Göktuğ Kayaalp / @cadadr / <https://www.gkayaalp.com/>
pgp:   024C 30DD 597D 142B 49AC 40EB 465C D949 B101 2427

Re: Potential bug in the logic of rmail-select-summary

[Eli Zaretskii]

> From: Göktuğ Kayaalp <self@gkayaalp.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 19 Jan 2021 20:14:17 +0300
> 
>  display-buffer-alist
>  '(("\\*Quail Completions" . (display-buffer-in-side-window))
>    ("\\*.*Completions\\*" . (display-buffer-in-side-window . ((side . bottom))))
>    ("\\*Help\\*" . (display-buffer-reuse-window))
>    ("Checkdoc" . (display-buffer-pop-up-window))
>    ("Calendar" . (display-buffer-in-side-window . ((side . bottom))))
>    ("help\\[R\\]" . (display-buffer-pop-up-window))
>    (".*" . (display-buffer-same-window)))
> 
> When I commented out that last line it worked.  FWIW, I added the
> following advice which seems to help:
> 
> (define-advice rmail-show-message
>     (:around (fn &rest args) inhibit-display-buffer-alist)
>   (let ((display-buffer-alist nil))
>     (apply fn args)))
> 
> Is this an expected interaction or a bug?

Well, you in fact asked Emacs to use display-buffer-same-window for
displaying any buffer except the few you explicitly exempted, no?
Then I think it's what you asked for.

> AFAIU rmail-pop-to-buffer could use some form of display-buffer-* to
> avoid going through display-buffer-alist.

Yes, but I'm  not sure we should prevent users from customizing the
way Rmail buffers are displayed.

Perhaps Martin could comment on this?

Re: Potential bug in the logic of rmail-select-summary

[martin rudalics]

 > Well, you in fact asked Emacs to use display-buffer-same-window for
 > displaying any buffer except the few you explicitly exempted, no?
 > Then I think it's what you asked for.

Right.

 >> AFAIU rmail-pop-to-buffer could use some form of display-buffer-* to
 >> avoid going through display-buffer-alist.

An overriding action?  That's much too severe.

 > Yes, but I'm  not sure we should prevent users from customizing the
 > way Rmail buffers are displayed.

Rmail is free to do what it wants in this regard.  But

(".*" . (display-buffer-same-window))

is a much too strong customization IMO.  It conflates 'pop-to-buffer'
with its standard "pop to a window other than the selected one even if
the buffer is already displayed in the selected window" behavior with
'switch-to-buffer'.  Don't do that - it may miss too many cases.

martin

Re: Potential bug in the logic of rmail-select-summary

[Göktuğ Kayaalp]

On 2021-01-19 20:04 +01, martin rudalics <rudalics@gmx.at> wrote:
>  >> AFAIU rmail-pop-to-buffer could use some form of display-buffer-* to
>  >> avoid going through display-buffer-alist.
> An overriding action?  That's much too severe.
>  > Yes, but I'm  not sure we should prevent users from customizing the
>  > way Rmail buffers are displayed.
> Rmail is free to do what it wants in this regard.  But
>
> (".*" . (display-buffer-same-window))
>
> is a much too strong customization IMO.  It conflates 'pop-to-buffer'
> with its standard "pop to a window other than the selected one even if
> the buffer is already displayed in the selected window" behavior with
> 'switch-to-buffer'.  Don't do that - it may miss too many cases.

Martin and Eli, thanks for your help and sorry for this non-issue.  I
had this problem for more than a couple years now, and I was having some
problems with some Magit commands too, should’ve figured it out by now.

But also, it seems to me that how Rmail uses (rmail-)pop-to-buffer is
somewhat problematic regardless because Rmail uses it as part of its
logic sometimes and apparently that results in multiple calls to that
function per command.  I tested some configs and appended the results
below, tho I’m again not sure if it’s expected behaviour or some bug.


Best,

        -gk.



Appendix: Testing various configs with rmail-summary and display-buffer-alist.

This config in emacs -Q results in a vertical split, both windows
displaying the summary buffer, after hitting ‘h’ in an Rmail buffer:

(setq
 display-buffer-alist
 '(((lambda (b _) (eq (with-current-buffer b major-mode) 'rmail-summary-mode))
    .
    (display-buffer-below-selected))))

When I instead try this one:

(setq
 display-buffer-alist
 '(((lambda (b _) (eq (with-current-buffer b major-mode) 'rmail-summary-mode))
    .
    (display-buffer-at-bottom . ((inhibit-same-window . t))))))

(with or without the alist, summary buffer replaces the Rmail buffer.  A
more sophisticated config which is what I’d actually like to have
behaves a bit more weirdly:

(setq
 display-buffer-alist
 '(((lambda (b _) (eq (with-current-buffer b major-mode) 'rmail-summary-mode))
    .
    (gk-display-buffer-for-rmail))))

(defun gk-display-buffer-for-rmail (buffer _)
  (if (= (length (window-list)) 1)
      (display-buffer-in-direction buffer '((direction . left)))
    ;; If there are 1+ windows, use the top quarter of selected
    ;; window.
    (split-window-vertically (/ (window-height) 4))
    (switch-to-buffer buffer)))

if I have a vertical split, this happens:

[ some buffer ][ rmail ] ->  [ some buffer ][ rmail-summary ]
                                            [ rmail-summary ]
                                            [    rmail      ]

instead of expected

[ some buffer ][ rmail ] ->  [ some buffer ][ rmail-summary ]
                                            [    rmail      ]

and if rmail window was the only window, I get

[ rmail-summary ][ rmail-summary ]

This is apparently because rmail-summary (and rmail-quit, too) end up
calling rmail-pop-to-buffer multiple times, somehow.  I verified that
with calling toggle-debug-on-entry on the function: with rmail-summary I
need to hit ‘c’ in the debugger 4 times for the command to complete.

I tried the following more elaborate setup:

(require 'cl-lib)

(setq
 display-buffer-alist
 '(((lambda (b _) (eq (with-current-buffer b major-mode) 'rmail-summary-mode))
    .
    (gk-display-buffer-for-rmail))))

(defun gk-display-buffer-for-rmail (buffer _)
  ;; If the summary buffer is displayed, just go to it
  (if-let* ((sumw (car (cl-remove-if
                        #'null
                        (mapcar (lambda (w)
                                  (and (eq 'rmail-summary-mode
                                           (buffer-local-value 'major-mode (window-buffer w)))
                                       w))
                                (window-list))))))
      (select-window sumw)
    ;; Otherwise, if frame has only 1 window...
    (if (= (length (window-list)) 1)
        ;; (a) ...split it in two, otherwise...
        (split-window-horizontally (/ (window-width) 2))
      ;; (b) ...create a 1/4 split above the selected window, ...
      (split-window-vertically (/ (window-height) 4)))
    ;; ...eventually, display ‘buffer’.
    (switch-to-buffer buffer)))

This helps with the multiple summary windows problem, but while (b) case
works as expected, (a) results in a single window displaying the summary
buffer, which is weird.  I thought what rmail-pop-to-buffer does has
some effect here, but the following works fine:

(let (split-width-threshold)
   (split-window-horizontally (/ (window-width) 2)))

Re: Potential bug in the logic of rmail-select-summary

[martin rudalics]

 > A
 > more sophisticated config which is what I’d actually like to have
 > behaves a bit more weirdly:
 >
 > (setq
 >   display-buffer-alist
 >   '(((lambda (b _) (eq (with-current-buffer b major-mode) 'rmail-summary-mode))
 >      .
 >      (gk-display-buffer-for-rmail))))
 >
 > (defun gk-display-buffer-for-rmail (buffer _)
 >    (if (= (length (window-list)) 1)
 >        (display-buffer-in-direction buffer '((direction . left)))
 >      ;; If there are 1+ windows, use the top quarter of selected
 >      ;; window.
 >      (split-window-vertically (/ (window-height) 4))
 >      (switch-to-buffer buffer)))

While the 'switch-to-buffer' is not entirely kosher in this context
('display-buffer-in-direction' returns a window, 'switch-to-buffer' a
buffer) this function should do what you want when applied to some
arbitrary buffer.

However, it misses one important aspect of the original 'pop-to-buffer':
When a non-selected window showing the buffer already exists, it reuses
that window.  Your function doesn't do that and that causes the problems
you see when it is called multiple times in a row (as rmailsum.el does)
because it always tries to make a new window.

So I suggest you put a 'display-buffer-reuse-window' at the beginning of
'gk-display-buffer-for-rmail' and test how it behaves then.

martin

Re: Potential bug in the logic of rmail-select-summary

[Göktuğ Kayaalp]

On 2021-01-20 17:51 +01, martin rudalics <rudalics@gmx.at> wrote:
> So I suggest you put a 'display-buffer-reuse-window' at the beginning of
> 'gk-display-buffer-for-rmail' and test how it behaves then.

Thanks for the suggestion.  I tried to add it to the logic using an
if-let*, and I made sure the function returned a window, but I could not
improve the situation. I then tried to just skip dealing with the
situation where there’s only a single window:

(setq
 display-buffer-alist
 '(((lambda (b _) (and (not (one-window-p))
                       ;; also:
		       ;; (memq (with-current-buffer b major-mode)
			;;     '(rmail-mode rmail-summary-mode)))
		       (eq (with-current-buffer b major-mode)
			     'rmail-mode)))
    .
    ((lambda (buffer _)
       (split-window-vertically (/ (window-height) 4))
       ;; Select the buffer.
       (switch-to-buffer buffer)
       ;; Return the current window.
       (selected-window))))))

but still got the same behaviour.  I think this has something to do with
the following lines in rmailsum.el, in the definition of
‘rmail-new-summary’:

	    ;; If pop-to-buffer did not use that window, delete that
	    ;; window.  (This can happen if it uses another frame.)
	    (if (not (eq sumbuf (window-buffer (frame-first-window))))
		(delete-other-windows)))

Here the condition is for some reason true somewhere in the call chain,
and I don’t really understand.

I think I’ll give up on this as what Rmail does with buffers in
intermediary steps of a command is fairly complex, and Rmail has many
assumptions about what’s where when.  Maybe later I’ll try make Rmail
call pop-to-buffer no more than strictly necessary.  Till then I’ll give
it it’s own frame and call it a day :)

Again, thanks for bearing with me!

Best,

     -gk.

Re: Potential bug in the logic of rmail-select-summary

[martin rudalics]

 > 	    ;; If pop-to-buffer did not use that window, delete that
 > 	    ;; window.  (This can happen if it uses another frame.)
 > 	    (if (not (eq sumbuf (window-buffer (frame-first-window))))
 > 		(delete-other-windows)))
 >
 > Here the condition is for some reason true somewhere in the call chain,
 > and I don’t really understand.

Do you mean to say that this part is also executed in the multiple
windows case despite of the

       (if (and (one-window-p)
	       pop-up-windows
	       (not pop-up-frames))

guard?

 > I think I’ll give up on this as what Rmail does with buffers in
 > intermediary steps of a command is fairly complex, and Rmail has many
 > assumptions about what’s where when.  Maybe later I’ll try make Rmail
 > call pop-to-buffer no more than strictly necessary.  Till then I’ll give
 > it it’s own frame and call it a day :)

I'm afraid that Rmail is just not amenable to customizations via
'display-buffer'.  It probably would have to be rewritten from scratch
for that purpose.

martin