unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] emacs: notmuch-tree: mark the initial message at point as read
@ 2021-02-16  0:01 Jonas Bernoulli
  2021-02-25 20:52 ` Tomi Ollila
  2021-07-25 18:30 ` David Bremner
  0 siblings, 2 replies; 9+ messages in thread
From: Jonas Bernoulli @ 2021-02-16  0:01 UTC (permalink / raw)
  To: notmuch

When moving between message in a tree or show buffer, the message at
point is marked as read.  Likewise when creating such a buffer, then
the message that is initially at point is supposed to be marked as
read as well.

The latter worked for `notmuch-show' but not for `notmuch-tree'.
Press "RET" or "M-RET" in a search buffer to observe these behaviors.

In both cases the marking is supposed to be done by the function
`notmuch-show-command-hook'.  In the case of `notmuch-show' that
function is added directly to `post-command-hook'.

`notmuch-tree' instead adds the function `notmuch-tree-command-hook'
to `post-command-hook' and that calls `notmuch-show-command-hook',
in the respective show buffer, but of course only if that exists.

Because the tree buffer is created asynchronously, the show buffer
doesn't exist yet by the time the `post-command-hook' is run, so
we have to explicitly run `notmuch-tree-command-hook' once the
show buffer exists.

The show buffer is created when `notmuch-tree-goto-and-insert-msg'
calls `notmuch-tree-show-message-in'.  `notmuch-tree-process-filter'
is what finally brings us here.
---
 emacs/notmuch-tree.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 13007a13..d9265fd5 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -949,7 +949,8 @@ (defun notmuch-tree-goto-and-insert-msg (msg)
       (goto-char (point-max))
       (forward-line -1)
       (when notmuch-tree-open-target
-	(notmuch-tree-show-message-in)))))
+	(notmuch-tree-show-message-in)
+	(notmuch-tree-command-hook)))))
 
 (defun notmuch-tree-insert-tree (tree depth tree-status first last)
   "Insert the message tree TREE at depth DEPTH in the current thread.
-- 
2.30.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-02-16  0:01 [PATCH] emacs: notmuch-tree: mark the initial message at point as read Jonas Bernoulli
@ 2021-02-25 20:52 ` Tomi Ollila
  2021-02-27  0:18   ` Jonas Bernoulli
  2021-07-25 18:30 ` David Bremner
  1 sibling, 1 reply; 9+ messages in thread
From: Tomi Ollila @ 2021-02-25 20:52 UTC (permalink / raw)
  To: Jonas Bernoulli, notmuch

On Tue, Feb 16 2021, Jonas Bernoulli wrote:

> When moving between message in a tree or show buffer, the message at
> point is marked as read.  Likewise when creating such a buffer, then
> the message that is initially at point is supposed to be marked as
> read as well.
>
> The latter worked for `notmuch-show' but not for `notmuch-tree'.
> Press "RET" or "M-RET" in a search buffer to observe these behaviors.
>
> In both cases the marking is supposed to be done by the function
> `notmuch-show-command-hook'.  In the case of `notmuch-show' that
> function is added directly to `post-command-hook'.
>
> `notmuch-tree' instead adds the function `notmuch-tree-command-hook'
> to `post-command-hook' and that calls `notmuch-show-command-hook',
> in the respective show buffer, but of course only if that exists.
>
> Because the tree buffer is created asynchronously, the show buffer
> doesn't exist yet by the time the `post-command-hook' is run, so
> we have to explicitly run `notmuch-tree-command-hook' once the
> show buffer exists.
>
> The show buffer is created when `notmuch-tree-goto-and-insert-msg'
> calls `notmuch-tree-show-message-in'.  `notmuch-tree-process-filter'
> is what finally brings us here.

Long message, small change (which is good). I try to understand
whether adding notmuch-tree-command-hook to post-command-hook in
notmuch-tree.el (not in this change) actually have any effect. 

Tomi

> ---
>  emacs/notmuch-tree.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
> index 13007a13..d9265fd5 100644
> --- a/emacs/notmuch-tree.el
> +++ b/emacs/notmuch-tree.el
> @@ -949,7 +949,8 @@ (defun notmuch-tree-goto-and-insert-msg (msg)
>        (goto-char (point-max))
>        (forward-line -1)
>        (when notmuch-tree-open-target
> -	(notmuch-tree-show-message-in)))))
> +	(notmuch-tree-show-message-in)
> +	(notmuch-tree-command-hook)))))
>  
>  (defun notmuch-tree-insert-tree (tree depth tree-status first last)
>    "Insert the message tree TREE at depth DEPTH in the current thread.
> -- 
> 2.30.1
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-02-25 20:52 ` Tomi Ollila
@ 2021-02-27  0:18   ` Jonas Bernoulli
  2021-02-27  0:28     ` Jonas Bernoulli
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Bernoulli @ 2021-02-27  0:18 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

> I try to understand whether adding notmuch-tree-command-hook to
> post-command-hook in notmuch-tree.el (not in this change) actually
> have any effect.

You're right, that's unnecessary, and it gets worse.

`notmuch-tree-command-hook' is unnecessary too and if it weren't,
then it would fail because its added to the buffer-local value of
`post-command-hook', but that buffer is not the current buffer
when the hook is run.

Using `post-command-hook' in any form for this is questionable in the
first place.  The idiomatic approach would be to use the `point-entered'
text property hook.  Or `notmuch-tree-{previous,next}-message' could
just do the marking explicitly.

It's also not okay that `notmuch-tree-show-message-in' keeps recreating
the window.

`notmuch-show-command-hook' forces redisplay to update values that pass
along, but don't and shouldn't use.  There is more than one function to
mark a message as read and they don't all end up doing the same thing.

What effectively does the marking is

(defun notmuch-tree-show-message-in ()
  ...
  (when notmuch-show-mark-read-tags
    (notmuch-tree-tag-update-display notmuch-show-mark-read-tags))
  ...)

I am going to refactor this mess, but that will take a while.

     Jonas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-02-27  0:18   ` Jonas Bernoulli
@ 2021-02-27  0:28     ` Jonas Bernoulli
  2021-06-05 11:23       ` David Bremner
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Bernoulli @ 2021-02-27  0:28 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

Hm, when no tree buffer is involved, then `notmuch-show-command-hook'
probably is required.  Anyway, there seems to be some undead code and
rethinking all this would be a good idea.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-02-27  0:28     ` Jonas Bernoulli
@ 2021-06-05 11:23       ` David Bremner
  2021-07-19 11:12         ` Jonas Bernoulli
  0 siblings, 1 reply; 9+ messages in thread
From: David Bremner @ 2021-06-05 11:23 UTC (permalink / raw)
  To: Jonas Bernoulli, Tomi Ollila, notmuch

Jonas Bernoulli <jonas@bernoul.li> writes:

> Hm, when no tree buffer is involved, then `notmuch-show-command-hook'
> probably is required.  Anyway, there seems to be some undead code and
> rethinking all this would be a good idea.

I'm not sure I followed the discussion, but I'm marking
id:20210216000138.19625-1-jonas@bernoul.li as "notmuch::moreinfo",
meaning we're expecting another version from Jonas. If that's wrong,
please let me know.

d

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-06-05 11:23       ` David Bremner
@ 2021-07-19 11:12         ` Jonas Bernoulli
  2021-07-19 23:37           ` David Bremner
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Bernoulli @ 2021-07-19 11:12 UTC (permalink / raw)
  To: David Bremner, Tomi Ollila, notmuch

David Bremner <david@tethera.net> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> Hm, when no tree buffer is involved, then `notmuch-show-command-hook'
>> probably is required.  Anyway, there seems to be some undead code and
>> rethinking all this would be a good idea.
>
> I'm not sure I followed the discussion, but I'm marking
> id:20210216000138.19625-1-jonas@bernoul.li as "notmuch::moreinfo",
> meaning we're expecting another version from Jonas. If that's wrong,
> please let me know.
>
> d

Please merge the proposed fix; IMO it is sound.  My follow-up, the reply
to Tomi, seems quite confused though.  But just because we are uncertain
whether there is further room for improvement, that shouldn't keep us
from fixing the bug.  Maybe in the future we can improve upon this fix
but I probably won't investigate any time soon.

     Jonas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-07-19 11:12         ` Jonas Bernoulli
@ 2021-07-19 23:37           ` David Bremner
  2021-07-23 14:01             ` Jonas Bernoulli
  0 siblings, 1 reply; 9+ messages in thread
From: David Bremner @ 2021-07-19 23:37 UTC (permalink / raw)
  To: Jonas Bernoulli, Tomi Ollila, notmuch

Jonas Bernoulli <jonas@bernoul.li> writes:

>
> Please merge the proposed fix; IMO it is sound.  My follow-up, the reply
> to Tomi, seems quite confused though.  But just because we are uncertain
> whether there is further room for improvement, that shouldn't keep us
> from fixing the bug.  Maybe in the future we can improve upon this fix
> but I probably won't investigate any time soon.
>
>      Jonas

Apologies, I should have experimented / asked before. I can't seem to
duplicate the problem being fixed here. Is it non-deterministic, or
should I expect it to always happen? If the latter, can you give me a
further hint / instructions how to reproduce? For me, whether in
tree-mode or search-mode, if I hit return the message at point is marked
unread.

d

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-07-19 23:37           ` David Bremner
@ 2021-07-23 14:01             ` Jonas Bernoulli
  0 siblings, 0 replies; 9+ messages in thread
From: Jonas Bernoulli @ 2021-07-23 14:01 UTC (permalink / raw)
  To: David Bremner, Tomi Ollila, notmuch

David Bremner <david@tethera.net> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>>
>> Please merge the proposed fix; IMO it is sound.  My follow-up, the reply
>> to Tomi, seems quite confused though.  But just because we are uncertain
>> whether there is further room for improvement, that shouldn't keep us
>> from fixing the bug.  Maybe in the future we can improve upon this fix
>> but I probably won't investigate any time soon.
>>
>>      Jonas
>
> Apologies, I should have experimented / asked before. I can't seem to
> duplicate the problem being fixed here. Is it non-deterministic, or
> should I expect it to always happen? If the latter, can you give me a
> further hint / instructions how to reproduce? For me, whether in
> tree-mode or search-mode, if I hit return the message at point is marked
> unread.
>
> d

I can always reproduce it:

1. Perform a search.

2. Visit the tree for a thread that you have previously not
   seen using notmuch-tree-from-search-thread. The default
   binding for that command is M-RET.  You mentioned that you
   pressed RET so maybe that's why you couldn't reproduce
   (i.e. you invoked notmuch-search-show-thread, which
   doesn't have this issue).

3. Press n (notmuch-tree-next-message) a few times until
   you reach the last message in the tree.  Do NOT press
   p (notmuch-tree-prev-message) to move back to the first
   message.

4. All messages should now be shown as "going to be marked
   as read".  In my case that is visualized by the word
   "unread" being striked out.  This DOES include the first
   message, but in this case it's not true.

5. Press q (notmuch-tree-quit) twice to return to the
   search view.

6. Press g (notmuch-refresh-this-buffer).  You would expect
   that to cause the thread at point to be marked read, but
   it is not.

7. Visit the tree for the same thread again using
   notmuch-tree-from-search-thread.  The second and subsequent
   messages are in the read state, the word "unread" is completely
   absent.  The first message however is shown as being in the
   "going to be marked as read", i.e. the word "unread" is shown
   and struck out.

The work around for this is to press RET (notmuch-tree-show-message)
at this point.  The word "unread" is still struck out, but if you
exit now (using q twice), then this change is actually applied.

Note that notmuch-search-show-thread does NOT behave as described
for notmuch-tree-from-search-thread above.  Applying my patch causes
the former to behave like the latter.

     Jonas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] emacs: notmuch-tree: mark the initial message at point as read
  2021-02-16  0:01 [PATCH] emacs: notmuch-tree: mark the initial message at point as read Jonas Bernoulli
  2021-02-25 20:52 ` Tomi Ollila
@ 2021-07-25 18:30 ` David Bremner
  1 sibling, 0 replies; 9+ messages in thread
From: David Bremner @ 2021-07-25 18:30 UTC (permalink / raw)
  To: Jonas Bernoulli, notmuch

Jonas Bernoulli <jonas@bernoul.li> writes:

> When moving between message in a tree or show buffer, the message at
> point is marked as read.  Likewise when creating such a buffer, then
> the message that is initially at point is supposed to be marked as
> read as well.

Applied to master. Thanks for patiently helping me reproduce the issue.

d

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-25 18:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16  0:01 [PATCH] emacs: notmuch-tree: mark the initial message at point as read Jonas Bernoulli
2021-02-25 20:52 ` Tomi Ollila
2021-02-27  0:18   ` Jonas Bernoulli
2021-02-27  0:28     ` Jonas Bernoulli
2021-06-05 11:23       ` David Bremner
2021-07-19 11:12         ` Jonas Bernoulli
2021-07-19 23:37           ` David Bremner
2021-07-23 14:01             ` Jonas Bernoulli
2021-07-25 18:30 ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).