Feedback-ID: i3ad947a1:Fastmail
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Sat, 20 Aug 2022 09:46:59 -0500
Message-Id: <CMAXRYNX38L8.33IA8OO53PC7U@TimBook-Arch>
To: "James Cook" <falsifian@falsifian.org>, "Moritz Poldrack"
 <moritz@poldrack.dev>, <~rjarry/aerc-devel@lists.sr.ht>
Subject: Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.
From: "Tim Culverhouse" <tim@timculverhouse.com>
References: <CLC6R7U7HXHI.HWEUHR6XAYV5@marty>
 <20220820002218.43914-1-falsifian@falsifian.org>
 <CMAOGU0XV5HK.AVX8CZ2GH384@Archetype>
 <CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org>
In-Reply-To: <CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org>

Thanks for the updates, James! Comments below:

On Sat Aug 20, 2022 at 9:15 AM CDT, James Cook wrote:
> 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):
> 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?).

I agree it's annoying, but I do think it's the right way to do it. More
below.

> 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.

I think that the right way to implement would be to move this piece to
the backend. Since the backends behave different, the UI should tell the
worker what it wants, and the worker should do it. If they all behaved
the same, I'd argue it doesn't matter - but they don't, so we should
allow each to operate as efficiently as possible.

In this case, the UI wants to open an email and mark it as read. The
worker can accomplish this in whatever way is most efficient for that
worker.

In other cases, the UI wants to fetch a message but not mark it as read.

> 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):

I agree with this.

What if this were implemented in several patches:

1. Move store.Flag to worker because it's currently redundant for IMAP,
but needed for other workers

2. Add PEEK option to the worker message and all relevant function
calls, with the fix to the ones that should be true

3. Add the option to set peek on opening message

This approach, at least to me, makes the imap worker more efficient than
it currently is, fixes some bugs, and gets this feature in.

Tim