Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Sat, 20 Aug 2022 14:15:20 +0000
Message-Id: <CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org>
Subject: Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.
From: "James Cook" <falsifian@falsifian.org>
To: "Moritz Poldrack" <moritz@poldrack.dev>,
 <~rjarry/aerc-devel@lists.sr.ht>
References: <CLC6R7U7HXHI.HWEUHR6XAYV5@marty>
 <20220820002218.43914-1-falsifian@falsifian.org>
 <CMAOGU0XV5HK.AVX8CZ2GH384@Archetype>
In-Reply-To: <CMAOGU0XV5HK.AVX8CZ2GH384@Archetype>

(Note: I'm quoting from three different emails below, reordered; hope
you don't mind.)

On Sat Aug 20, 2022 at 7:29 AM UTC, Moritz Poldrack wrote:
(snip)
> > commit b13b7be79c85a408dcb6c509fb7b6284e29ff9ee
> > Merge: 8bfe752 1b91b68
> > Author: James Cook <falsifian@falsifian.org>
> > Date:   Fri Aug 19 22:50:05 2022 +0000
> >
> >     Merge branch 'master' into unread
> >
> > commit 8bfe752c2807efaf37507c87fe00a568239e23b7
> > Author: James Cook <falsifian@falsifian.org>
> > Date:   Tue Jul 5 05:38:38 2022 +0000
> > ---
> Thanks for your patchset, could you please remove what temporary commits
> made it from the commit message?

Sorry, I will remove them in the next revision.


> Please also send your new revisions as a v2, v3, =E2=80=A6 vn :)

Where should I write v2, v3, etc? As far as I can tell, if I want to add
that to the subject line, it will also end up in the commit message,
which doesn't make sense because in the end only one version will appear
in the commit history. (Sorry, I'm new to the git-send-email workflow.)


> > +*auto-mark-read*
> > +     Set the "seen" flag when a message is viewed.
> We should probably mention that this only has an effect when using IMAP

No, it affects other backends; at least, I tested it with notmuch. See:

-	store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)
+	if uiConfig.AutoMarkRead {
+		store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag,
true, nil)
+	}

In fact, I started with only that change (and related plumbing), and
only got into the IMAP stuff when someone pointed out it was not
sufficient for the IMAP backend.


> >  ) {
> >  	logging.Infof("Fetching full messages: %v", msg.Uids)
> > -	section :=3D &imap.BodySectionName{}
> > +	section :=3D &imap.BodySectionName{
> > +		Peek: true,
> Would it be useful to set this value (and the above
> partBodySection.Peek) to only peek when needed? This way we'd use the
> most appropriate method for getting the body.
>
> I fail to find a satisfactory answer in your follow up email.

I considered that that end then decided not to.

Here's an expanded reasoning, mostly following the two points in my
previous email.

1. Plumbing (point #2 in previous email):

When revising the patch, I started out with Tim's suggestion, which is
in line with what you're asking for: I added "Peek" fields to the
FetchFullMessages and FetchMessageBodyPart structs, so that we could
only peek as needed.

That's only useful if it gets populated. So, I modified the FetchFull
and FetchBodyPart methods in lib/msgstore.go to take an additional
parameter "peek". Something like the following change (indented so it
doesn't make the whole rest of the email look like a patch):

	--- a/lib/msgstore.go
	+++ b/lib/msgstore.go
	@@ -131,7 +131,7 @@ func (store *MessageStore) FetchHeaders(uids []uint32,
		}
	 }
	=20
	-func (store *MessageStore) FetchFull(uids []uint32, cb func(*types.FullMe=
ssage)) {
	+func (store *MessageStore) FetchFull(uids []uint32, peek bool, cb func(*t=
ypes.FullMessage)) {
		// TODO: this could be optimized by pre-allocating toFetch and trimming i=
t
		// at the end. In practice we expect to get most messages back in one fra=
me.
		var toFetch []uint32
	@@ -151,6 +151,7 @@ func (store *MessageStore) FetchFull(uids []uint32, cb=
 func(*types.FullMessage))
		if len(toFetch) > 0 {
			store.worker.PostAction(&types.FetchFullMessages{
				Uids: toFetch,
	+			Peek: peek,
			}, func(msg types.WorkerMessage) {
				if _, ok :=3D msg.(*types.Error); ok {
					for _, uid :=3D range toFetch {
	@@ -162,10 +163,11 @@ func (store *MessageStore) FetchFull(uids []uint32, =
cb func(*types.FullMessage))
		}
	 }
	=20
	-func (store *MessageStore) FetchBodyPart(uid uint32, part []int, cb func(=
io.Reader)) {
	+func (store *MessageStore) FetchBodyPart(uid uint32, part []int, peek boo=
l, cb func(io.Reader)) {
		store.worker.PostAction(&types.FetchMessageBodyPart{
			Uid:  uid,
			Part: part,
	+		Peek: peek,
		}, func(resp types.WorkerMessage) {
			msg, ok :=3D resp.(*types.MessageBodyPart)
			if !ok {

Now I need to actually pass that "peek" parameter. Based on some quick
grepping, store.FetchFull is called three times and store.FetchBodyPart
four times. Only one of those seven calls is a function actually updated
by this patch (NewMessageStorView in lib/messageview.go). For example,
the pipe command uses it (to get the data to be piped?).

This isn't the end of the world, but it's annoying, and it brings me
to my other point:

2. Having two mechanisms is clumsy and error-prone. Peek=3Dfalse is never
   needed.

Every time you view a message with aerc (before this patch), this line
gets called:

	store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)

This suffices to set the \Seen flag.

If we don't set Peek, or we optionally set Peek according to some state,
then we're doing the same thing two different ways. Not only is it
unnecssary, but in my opinion it's bound to cause confusion if another
person tries to modify the mark-as-read behaviour in the future, unless
they have taken note of the issue.

To add to this: in most of those seven calls I think "peek" I mentioned
above, I think Peek should be fixed to true. The only one where setting
Peek to false makes some sense is NewMessageStoreView in
lib/messageview.go. It could even be argued that the current code is
buggy due to not setting Peek in the other cases. As an example, I just
did the following experiment on master (without this patch):

- Opened an email with the IMAP backend. Of course it is marked as read.
- (For flavour: let's say I left aerc open on my desktop and home and
  went to work for the day.)
- With a different client on my phone, I marked the email as unread
  again. (Say, I realized it still needs some follow-up.)
- Back in aerc, I ran the :pipe command.

Result: the \Seen flag is set again. It's kind of a corner case, but
this seems like the wrong behaviour: I don't expect piping it through a
command to cause it to be marked as read again. Imagine the same for
forwarding or replying to an email.

Anyway, even if you think it's fine to set \Seen in those cases, my main
(sub-)point is Peek=3Dfalse is redundant. Making it configurable would
mean having two mechanisms for setting \Seen, and having to disable both
if you don't want to set it.

--=20
James