[PATCH] emacs/show: Only recenter interactively

[PATCH] emacs/show: Only recenter interactively

[Damien Cassou]

Several notmuch-show commands recenter the window by calling
`notmuch-show-message-adjust`. This is fine when these commands are
called interactively but not when used inside non-interactive
functions. An example of the problem is when `which-func-mode` is
activated: each time the user moves the point to a different line, the
window is recentered. This happens because `which-func-mode` calls
`imenu` which calls `notmuch-show-imenu-prev-index-position-function`
which calls `notmuch-show-previous-message` which recenters the
window.

This patch makes sure that commands only invoke
`notmuch-show-message-adjust` when they have been called
interactively.
---
 emacs/notmuch-show.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 14e3c698..c08375b8 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1535,7 +1535,7 @@ (defun notmuch-show-goto-message (msg-id)
 		   until (not (notmuch-show-goto-message-next)))
     (goto-char (point-min))
     (message "Message-id not found."))
-  (notmuch-show-message-adjust))
+  (when (interactive-p) (notmuch-show-message-adjust)))
 
 (defun notmuch-show-apply-state (state)
   "Apply STATE to the current buffer.
@@ -2052,7 +2052,7 @@ (defun notmuch-show-rewind ()
       (when (<= (count-screen-lines (window-start) start-of-message)
 		next-screen-context-lines)
 	(goto-char (notmuch-show-message-top))
-	(notmuch-show-message-adjust))
+	(when (interactive-p) (notmuch-show-message-adjust)))
       ;; Move to the top left of the window.
       (goto-char (window-start)))
      (t
@@ -2109,7 +2109,7 @@ (defun notmuch-show-next-message (&optional pop-at-end)
 thread, navigate to the next thread in the parent search buffer."
   (interactive "P")
   (if (notmuch-show-goto-message-next)
-      (notmuch-show-message-adjust)
+      (when (interactive-p) (notmuch-show-message-adjust))
     (if pop-at-end
 	(notmuch-show-next-thread)
       (goto-char (point-max)))))
@@ -2120,7 +2120,7 @@ (defun notmuch-show-previous-message ()
   (if (= (point) (notmuch-show-message-top))
       (notmuch-show-goto-message-previous)
     (notmuch-show-move-to-message-top))
-  (notmuch-show-message-adjust))
+  (when (interactive-p) (notmuch-show-message-adjust)))
 
 (defun notmuch-show-next-open-message (&optional pop-at-end)
   "Show the next open message.
@@ -2134,7 +2134,7 @@ (defun notmuch-show-next-open-message (&optional pop-at-end)
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-message-visible-p))))
     (if r
-	(notmuch-show-message-adjust)
+	(when (interactive-p) (notmuch-show-message-adjust))
       (if pop-at-end
 	  (notmuch-show-next-thread)
 	(goto-char (point-max))))
@@ -2147,7 +2147,7 @@ (defun notmuch-show-next-matching-message ()
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-get-prop :match))))
     (if r
-	(notmuch-show-message-adjust)
+	(when (interactive-p) (notmuch-show-message-adjust))
       (goto-char (point-max)))))
 
 (defun notmuch-show-open-if-matched ()
@@ -2176,7 +2176,7 @@ (defun notmuch-show-previous-open-message ()
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-message-adjust))
+  (when (interactive-p) (notmuch-show-message-adjust)))
 
 (defun notmuch-show-view-raw-message ()
   "View the original source of the current message."
-- 
2.47.0

Re: [PATCH] emacs/show: Only recenter interactively

[Damien Cassou]

Hi,

I've used this patch for several weeks now and I'm happy about it. Does
anyone want to give me a review?

Thank you

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

Re: [PATCH] emacs/show: Only recenter interactively

[Pengji Zhang]

Damien Cassou <damien@cassou.me> writes:

> Several notmuch-show commands recenter the window by calling
> `notmuch-show-message-adjust`. This is fine when these commands are
> called interactively but not when used inside non-interactive
> functions. An example of the problem is when `which-func-mode` is
> activated: each time the user moves the point to a different line, the
> window is recentered. This happens because `which-func-mode` calls
> `imenu` which calls `notmuch-show-imenu-prev-index-position-function`
> which calls `notmuch-show-previous-message` which recenters the
> window.
>
> This patch makes sure that commands only invoke
> `notmuch-show-message-adjust` when they have been called
> interactively.

I think this is a reasonable change. Thanks! I am not a maintainer, but
here is my review of your patch.

> ---
>  emacs/notmuch-show.el | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 14e3c698..c08375b8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1535,7 +1535,7 @@ (defun notmuch-show-goto-message (msg-id)
>  		   until (not (notmuch-show-goto-message-next)))
>      (goto-char (point-min))
>      (message "Message-id not found."))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))

'notmuch-show-goto-message' is not a command. So it is always called
non-interactively.

>  (defun notmuch-show-apply-state (state)
>    "Apply STATE to the current buffer.
> @@ -2052,7 +2052,7 @@ (defun notmuch-show-rewind ()
>        (when (<= (count-screen-lines (window-start) start-of-message)
>  		next-screen-context-lines)
>  	(goto-char (notmuch-show-message-top))
> -	(notmuch-show-message-adjust))
> +	(when (interactive-p) (notmuch-show-message-adjust)))
>        ;; Move to the top left of the window.
>        (goto-char (window-start)))
>       (t
> @@ -2109,7 +2109,7 @@ (defun notmuch-show-next-message (&optional pop-at-end)
>  thread, navigate to the next thread in the parent search buffer."
>    (interactive "P")
>    (if (notmuch-show-goto-message-next)
> -      (notmuch-show-message-adjust)
> +      (when (interactive-p) (notmuch-show-message-adjust))
>      (if pop-at-end
>  	(notmuch-show-next-thread)
>        (goto-char (point-max)))))
> @@ -2120,7 +2120,7 @@ (defun notmuch-show-previous-message ()
>    (if (= (point) (notmuch-show-message-top))
>        (notmuch-show-goto-message-previous)
>      (notmuch-show-move-to-message-top))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))
>  
>  (defun notmuch-show-next-open-message (&optional pop-at-end)
>    "Show the next open message.
> @@ -2134,7 +2134,7 @@ (defun notmuch-show-next-open-message (&optional pop-at-end)
>      (while (and (setq r (notmuch-show-goto-message-next))
>  		(not (notmuch-show-message-visible-p))))
>      (if r
> -	(notmuch-show-message-adjust)
> +	(when (interactive-p) (notmuch-show-message-adjust))
>        (if pop-at-end
>  	  (notmuch-show-next-thread)
>  	(goto-char (point-max))))
> @@ -2147,7 +2147,7 @@ (defun notmuch-show-next-matching-message ()
>      (while (and (setq r (notmuch-show-goto-message-next))
>  		(not (notmuch-show-get-prop :match))))
>      (if r
> -	(notmuch-show-message-adjust)
> +	(when (interactive-p) (notmuch-show-message-adjust))
>        (goto-char (point-max)))))
>  
>  (defun notmuch-show-open-if-matched ()
> @@ -2176,7 +2176,7 @@ (defun notmuch-show-previous-open-message ()
>  		  (notmuch-show-goto-message-previous)
>  		(notmuch-show-move-to-message-top))
>  	      (not (notmuch-show-message-visible-p))))
> -  (notmuch-show-message-adjust))
> +  (when (interactive-p) (notmuch-show-message-adjust)))
>  
>  (defun notmuch-show-view-raw-message ()
>    "View the original source of the current message."

'interactive-p' is a long obsolete function. I think we should at least
use 'called-interactively-p'. Even better, as suggested in the
documentation of 'called-interactively-p', we may add an optional
argument and make use of the interactive form. E.g.,

--8<---------------cut here---------------start------------->8---
-(defun notmuch-show-previous-open-message ()
+(defun notmuch-show-previous-open-message (&optional adjust)
   "Show the previous open message."
-  (interactive)
+  (interactive (list t))
   (while (and (if (= (point) (notmuch-show-message-top))
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-message-adjust))
+  (when adjust (notmuch-show-message-adjust)))
--8<---------------cut here---------------end--------------->8---

Best,
Pengji

Re: [PATCH] emacs/show: Only recenter interactively

[Damien Cassou]

Hi,

thank you very much for your feedback. I have a question though:

Pengji Zhang <me@pengjiz.com> writes:
> 'notmuch-show-goto-message' is not a command. So it is always called
> non-interactively.

That makes sense. This function is, for example, called by
`notmuch-show-apply-state' which is itself called by
`notmuch-show--build-buffer' which is itself called by
`notmuch-show-refresh-view'. If I go with the idea of adding an `adjust`
parameter as you (and Emacs maintainers) suggest, does it mean that I
should add this parameter to all these functions? Or maybe should I move
the call to `notmuch-show-message-adjust' from
`notmuch-show-goto-message' to interactive functions that (possibly
indirectly) make use of it (e.g., `notmuch-show-refresh-view' and
`notmuch-show-filter-thread'). The second solution seems to make the
most sense to me because it is only in the interactive function that the
developer really know what should be shown to the user and why.

Do you have another idea or a preference?

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

Re: [PATCH] emacs/show: Only recenter interactively

[Pengji Zhang]

Damien Cassou <damien@cassou.me> writes:

> [...]
>
> That makes sense. This function is, for example, called by
> `notmuch-show-apply-state' which is itself called by
> `notmuch-show--build-buffer' which is itself called by
> `notmuch-show-refresh-view'. If I go with the idea of adding an
> `adjust` parameter as you (and Emacs maintainers) suggest, does it
> mean that I should add this parameter to all these functions? Or maybe
> should I move the call to `notmuch-show-message-adjust' from
> `notmuch-show-goto-message' to interactive functions that (possibly
> indirectly) make use of it (e.g., `notmuch-show-refresh-view' and
> `notmuch-show-filter-thread'). The second solution seems to make the
> most sense to me because it is only in the interactive function that
> the developer really know what should be shown to the user and why.
>
> Do you have another idea or a preference?

I like the second solution better as well. To me, it generally feels
cleaner if we constrain the user-visible side effects in interactive
commands. However, I am afraid it would be involved, and I am not sure
if it is worth the effort.

Besides, do you have other use cases for calling those commands
non-interactively? If not, IMO this time it might be better to only
change 'notmuch-show-previous-message' (or
'notmuch-show-imenu-prev-index-position-function'), so to fix the issue
you had with 'which-function-mode'. After a closer look, I have an
impression that those commands are meant to be interactive only.

Regards,
Pengji

Re: [PATCH] emacs/show: Only recenter interactively

[Damien Cassou]

Pengji Zhang <me@pengjiz.com> writes:
> Besides, do you have other use cases for calling those commands
> non-interactively? If not, IMO this time it might be better to only
> change 'notmuch-show-previous-message' (or
> 'notmuch-show-imenu-prev-index-position-function'), so to fix the issue
> you had with 'which-function-mode'. After a closer look, I have an
> impression that those commands are meant to be interactive only.

This is a brilliant idea. I submitted a new patch:
https://nmbug.notmuchmail.org/nmweb/show/20241217192919.889035-1-damien%40cassou.me


-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill