~rjarry/aerc-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
25 7

[PATCH aerc] Allow not marking viewed messages as seen.

Details
Message ID
<20220705060618.68685-1-falsifian@falsifian.org>
DKIM signature
pass
Download raw message
Patch: +22 -6
Fixes: https://todo.sr.ht/~rjarry/aerc/48
Signed-off-by: James Cook <falsifian@falsifian.org>
---
Sending this out for feedback. I should probably test it at least a
little more; e.g. I haven't tried deleting a message to check that the
change to delete.go makes sense.

 commands/account/view.go | 2 +-
 commands/msg/delete.go   | 2 +-
 commands/msgview/next.go | 2 +-
 config/aerc.conf         | 6 ++++++
 config/config.go         | 2 ++
 doc/aerc-config.5.scd    | 5 +++++
 lib/messageview.go       | 7 +++++--
 widgets/msglist.go       | 2 +-
 8 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/commands/account/view.go b/commands/account/view.go
index 8537d33..04a1687 100644
--- a/commands/account/view.go
+++ b/commands/account/view.go
@@ -46,7 +46,7 @@ func (ViewMessage) Execute(aerc *widgets.Aerc, args []string) error {
		return nil
	}
	lib.NewMessageStoreView(msg, store, aerc.Crypto, aerc.DecryptKeys,
		func(view lib.MessageView, err error) {
		acct.UiConfig(), func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
				return
diff --git a/commands/msg/delete.go b/commands/msg/delete.go
index d26169f..aa129fa 100644
--- a/commands/msg/delete.go
+++ b/commands/msg/delete.go
@@ -69,7 +69,7 @@ func (Delete) Execute(aerc *widgets.Aerc, args []string) error {
				return nil
			}
			lib.NewMessageStoreView(next, store, aerc.Crypto, aerc.DecryptKeys,
				func(view lib.MessageView, err error) {
				acct.UiConfig(), func(view lib.MessageView, err error) {
					if err != nil {
						aerc.PushError(err.Error())
						return
diff --git a/commands/msgview/next.go b/commands/msgview/next.go
index 928b9fb..ca72715 100644
--- a/commands/msgview/next.go
+++ b/commands/msgview/next.go
@@ -43,7 +43,7 @@ func (NextPrevMsg) Execute(aerc *widgets.Aerc, args []string) error {
		return nil
	}
	lib.NewMessageStoreView(nextMsg, store, aerc.Crypto, aerc.DecryptKeys,
		func(view lib.MessageView, err error) {
		acct.UiConfig(), func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
				return
diff --git a/config/aerc.conf b/config/aerc.conf
index 2f7597c..2f7a3d9 100644
--- a/config/aerc.conf
+++ b/config/aerc.conf
@@ -18,6 +18,12 @@ pgp-provider=internal
unsafe-accounts-conf=false

[ui]
#
# Set the "seen" flag when a message is viewed.
#
# Default: true
auto-mark-read=true

#
# Describes the format for each row in a mailbox view. This field is compatible
# with mutt's printf-like syntax.
diff --git a/config/config.go b/config/config.go
index 8a243c3..b1ecc0d 100644
--- a/config/config.go
+++ b/config/config.go
@@ -32,6 +32,7 @@ type GeneralConfig struct {
}

type UIConfig struct {
	AutoMarkRead        bool          `ini:"auto-mark-read"`
	IndexFormat         string        `ini:"index-format"`
	TimestampFormat     string        `ini:"timestamp-format"`
	ThisDayTimeFormat   string        `ini:"this-day-time-format"`
@@ -689,6 +690,7 @@ func LoadConfigFromFile(root *string, logger *log.Logger) (*AercConfig, error) {
		},

		Ui: UIConfig{
			AutoMarkRead:       true,
			IndexFormat:        "%D %-17.17n %s",
			TimestampFormat:    "2006-01-02 03:04 PM",
			ThisDayTimeFormat:  "",
diff --git a/doc/aerc-config.5.scd b/doc/aerc-config.5.scd
index 518fe51..02a1514 100644
--- a/doc/aerc-config.5.scd
+++ b/doc/aerc-config.5.scd
@@ -97,6 +97,11 @@ These options are configured in the *[ui]* section of aerc.conf.
|  %Z
:  flags (O=old, N=new, r=answered, D=deleted, !=flagged, \*=marked)

*auto-mark-read*
	Set the "seen" flag when a message is viewed.

	Default: true

*timestamp-format*
	See time.Time#Format at https://godoc.org/time#Time.Format

diff --git a/lib/messageview.go b/lib/messageview.go
index a1797d5..0b7b5b9 100644
--- a/lib/messageview.go
+++ b/lib/messageview.go
@@ -9,6 +9,7 @@ import (
	"github.com/emersion/go-message"
	_ "github.com/emersion/go-message/charset"

	"git.sr.ht/~rjarry/aerc/config"
	"git.sr.ht/~rjarry/aerc/lib/crypto"
	"git.sr.ht/~rjarry/aerc/models"
	"git.sr.ht/~rjarry/aerc/worker/lib"
@@ -62,7 +63,7 @@ type MessageStoreView struct {

func NewMessageStoreView(messageInfo *models.MessageInfo,
	store *MessageStore, pgp crypto.Provider, decryptKeys openpgp.PromptFunction,
	cb func(MessageView, error)) {
	uiConfig *config.UIConfig, cb func(MessageView, error)) {

	msv := &MessageStoreView{messageInfo, store,
		nil, nil, messageInfo.BodyStructure}
@@ -97,7 +98,9 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
	} else {
		cb(msv, nil)
	}
	store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)
	if uiConfig.AutoMarkRead {
		store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)
	}
}

func (msv *MessageStoreView) MessageInfo() *models.MessageInfo {
diff --git a/widgets/msglist.go b/widgets/msglist.go
index ec14e79..949662c 100644
--- a/widgets/msglist.go
+++ b/widgets/msglist.go
@@ -305,7 +305,7 @@ func (ml *MessageList) MouseEvent(localX int, localY int, event tcell.Event) {
					return
				}
				lib.NewMessageStoreView(msg, store, ml.aerc.Crypto,
					ml.aerc.DecryptKeys,
					ml.aerc.DecryptKeys, acct.UiConfig(),
					func(view lib.MessageView, err error) {
						if err != nil {
							ml.aerc.PushError(err.Error())
-- 
2.36.1

[aerc/patches] build success

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CL7HYOF8FL6Y.UQR2BKMX2FU2@cirno>
In-Reply-To
<20220705060618.68685-1-falsifian@falsifian.org> (view parent)
DKIM signature
missing
Download raw message
aerc/patches: SUCCESS in 2m58s

[Allow not marking viewed messages as seen.][0] from [James Cook][1]

[0]: https://lists.sr.ht/~rjarry/aerc-devel/patches/33568
[1]: falsifian@falsifian.org

✓ #794947 SUCCESS aerc/patches/openbsd.yml     https://builds.sr.ht/~rjarry/job/794947
✓ #794946 SUCCESS aerc/patches/alpine-edge.yml https://builds.sr.ht/~rjarry/job/794946
Details
Message ID
<CLAGWLDXIV6M.2KH3WJUCNXDRM@TimBook-Arch>
In-Reply-To
<20220705060618.68685-1-falsifian@falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Tue Jul 5, 2022 at 1:06 AM CDT, James Cook wrote:
> Fixes: https://todo.sr.ht/~rjarry/aerc/48
> Signed-off-by: James Cook <falsifian@falsifian.org>

Thanks for the patch James! I tested this on imap, maildir, and notmuch
backends. It works as expected for maildir and notmuch.

The imap backend still marks the messages as read, however. The issue is
that imap servers set the \Seen flag when any part of the body is
fetched (see https://datatracker.ietf.org/doc/html/rfc3501#section-6.4.5)

I suspect we'll need to remove the flag after fetching for imap
backends.

The rest looked good to me!

Tim
Details
Message ID
<CLBLJEF0Y8M8.37UIZII0LE4TG@moth.falsifian.org>
In-Reply-To
<CLAGWLDXIV6M.2KH3WJUCNXDRM@TimBook-Arch> (view parent)
DKIM signature
pass
Download raw message
On Fri Jul 8, 2022 at 5:57 PM UTC, Tim Culverhouse wrote:
> On Tue Jul 5, 2022 at 1:06 AM CDT, James Cook wrote:
> > Fixes: https://todo.sr.ht/~rjarry/aerc/48
> > Signed-off-by: James Cook <falsifian@falsifian.org>
>
> Thanks for the patch James! I tested this on imap, maildir, and notmuch
> backends. It works as expected for maildir and notmuch.
>
> The imap backend still marks the messages as read, however. The issue is
> that imap servers set the \Seen flag when any part of the body is
> fetched (see https://datatracker.ietf.org/doc/html/rfc3501#section-6.4.5)
>
> I suspect we'll need to remove the flag after fetching for imap
> backends.
>
> The rest looked good to me!
>
> Tim

Thanks for testing it!

Looking at that IMAP RFC link, I think the right thing to do is to fetch
BODY.PEEK instead of BODY. Maybe aerc could always fetch BODY.PEEK
instead of BOD, so setting \Seen is always deliberate. I've never
written code that speaks IMAP so take this all with a grain of salt. (I
see "BODY.PEEK" appears in isync's source [0] though I haven't looked
carefully.)

FYI I've been using this patch with aerc's notmuch backend since I sent
it, and haven't had any trouble.

-- 
James

[0] https://sourceforge.net/p/isync/isync/ci/master/tree/src/drv_imap.c
Details
Message ID
<6010ddbb-acfb-4806-b7a3-b04f4c633024@beta.fastmail.com>
In-Reply-To
<CLBLJEF0Y8M8.37UIZII0LE4TG@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
> Looking at that IMAP RFC link, I think the right thing to do is to fetch
> BODY.PEEK instead of BODY. Maybe aerc could always fetch BODY.PEEK
> instead of BOD, so setting \Seen is always deliberate. I've never
> written code that speaks IMAP so take this all with a grain of salt. (I
> see "BODY.PEEK" appears in isync's source [0] though I haven't looked
> carefully.)

I think all that’s needed would be to set {section}.Peek = true in worker/imap/fetch.go [0].

You could add a field to the FetchBodyPart and FetchFullMessage messages which tells the worker what the users config value is for peek.

Good find!

https://git.sr.ht/~rjarry/aerc/tree/master/item/worker/imap/fetch.go
Details
Message ID
<CLBTUDQ29NM8.1Q41X548XI78B@Archetype>
In-Reply-To
<CLBLJEF0Y8M8.37UIZII0LE4TG@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sun Jul 10, 2022 at 3:48 AM CEST, James Cook wrote:
> Looking at that IMAP RFC link, I think the right thing to do is to fetch
> BODY.PEEK instead of BODY. Maybe aerc could always fetch BODY.PEEK
> instead of BOD, so setting \Seen is always deliberate.
I'd argue that BODY is the correct way to handle it and rather remove
the \Seen flag when requested. aerc is a mail client for which this
behaviour is the expected in most cases.

> I've never written code that speaks IMAP so take this all with a grain
> of salt. (I see "BODY.PEEK" appears in isync's source [0] though I
> haven't looked carefully.)
I think this is due to isync having a completely different purpose from
aerc. isync indiscriminately downloads all mail and would thereby mess
up the \Seen flags for all messages.

> FYI I've been using this patch with aerc's notmuch backend since I sent
> it, and haven't had any trouble.
I think having this flag only work for maildir and notmuch backends
would be fine. If IMAP is something you want to support, I'd suggest
removing the \Seen after fetching the mail.

-- 
Moritz Poldrack
https://moritz.sh
Details
Message ID
<CLC2LS6BW4PP.5YE3QHA8D6KD@moth.falsifian.org>
In-Reply-To
<CLBTUDQ29NM8.1Q41X548XI78B@Archetype> (view parent)
DKIM signature
pass
Download raw message
On Sun Jul 10, 2022 at 8:18 AM UTC, Moritz Poldrack wrote:
> On Sun Jul 10, 2022 at 3:48 AM CEST, James Cook wrote:
> > Looking at that IMAP RFC link, I think the right thing to do is to fetch
> > BODY.PEEK instead of BODY. Maybe aerc could always fetch BODY.PEEK
> > instead of BOD, so setting \Seen is always deliberate.
> I'd argue that BODY is the correct way to handle it and rather remove
> the \Seen flag when requested. aerc is a mail client for which this
> behaviour is the expected in most cases.

That may be more consistent with the intention behind \Seen, but here
are a few arguments for using BODY.PEEK nonetheless, at least if the
user has set the new auto-mark-read=false:

- It removes a failure case: if aerc crashes or the Internet connection
  is lost after fetching but before unsetting \Seen.

  I really don't relish the idea of accidentally marking an email as
  \Seen --- with my workflow, that means the email drops off my radar,
  and it might have been important.

- I would argue it's simpler. One request instead of two.

- I could imagine aerc may be modified later to pre-fetch messages ---
  for example, in case the user wanted to search message bodies.


Off-topic:

I have thought a few times of using an "Archive" folder instead of
read/unread status, because I feel like I'm working against the grain.
I haven't switched for two reasons: (a) I don't like the idea of
re-transferring a (potentially large) email a couple of times just
because I decided to Archive it (mbsync, at least, isn't smart enough to
move instead of copying); and (b) I don't think my Android email client
(K-9 mail) can be made to show a complete thread combining archived and
unarchived emails, and also indicate somehow which of them are archived
vs. not. Read/unread just happens to be one of a very few flags that
email clients are designed to display.

What I'd really like is better support for IMAP (custom) labels: both
with Maildir/mbsync and on some Android client. Then I'd simply use an
"archive" or "inbox" label and be done with it.

-- 
James
Details
Message ID
<CLC3W7D71TSY.WLIXVX3UO4SJ@Archetype>
In-Reply-To
<CLC2LS6BW4PP.5YE3QHA8D6KD@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sun Jul 10, 2022 at 5:10 PM CEST, James Cook wrote:
> - It removes a failure case: if aerc crashes or the Internet connection
>   is lost after fetching but before unsetting \Seen.
I mean… we do have quite a number of race conditions, but I really don't
think this is likely at all

>   I really don't relish the idea of accidentally marking an email as
>   \Seen --- with my workflow, that means the email drops off my radar,
>   and it might have been important.
I'd suggest using maildir in this case. Isync can – if I remember
correctly – sync the mails state back to the origin server.

> - I would argue it's simpler. One request instead of two.
The default is still it being off, in this case there would by default
be two requests body fetching and marking the mail, no?

> - I could imagine aerc may be modified later to pre-fetch messages ---
>   for example, in case the user wanted to search message bodies.
IMAP already searches bodies on the server, so I don't really see a
reason at the moment. In any case, prefetching mail is an entirely
different operation than opening a mail for reading.

> Off-topic:
>
> I have thought a few times of using an "Archive" folder instead of
> read/unread status, because I feel like I'm working against the grain.
> I haven't switched for two reasons:
> (a) I don't like the idea of re-transferring a (potentially large)
> email a couple of times just because I decided to Archive it (mbsync,
> at least, isn't smart enough to move instead of copying);
The IMAP backend (which is up for discussion here) is smart enough to
just tell the Server to move the messages, so for IMAP there's no
problem. For mbsync this patchset would just fix that issue for you,
wouldn't it?

> and (b) I don't think my Android email client (K-9 mail) can be made
> to show a complete thread combining archived and unarchived emails,
> and also indicate somehow which of them are archived vs. not.
> Read/unread just happens to be one of a very few flags that email
> clients are designed to display.
I'm afraid I don't get your use-case… wasn't it that you don't want to
see the "done" mails afterwards? In that case, just open your INBOX, if
you want a completed mail, go into the archive.

> What I'd really like is better support for IMAP (custom) labels: both
> with Maildir/mbsync and on some Android client. Then I'd simply use an
> "archive" or "inbox" label and be done with it.
I would love that too… but I really don't have the time to implement it…
I would also like to add urgencies to messages, but that's another thing
that'd take a bit of time.

Let me just describe my workflow here, maybe this can serve as
inspiration to you:
- Mail is received by server and client notifies me about it
- I read the mail
- if it's something I am supposed to act on, I either just do it, or
  leave the mail in my inbox (as read).
- after (and *only* after) whatever the mail's associated action is has
  been performed, I :archive year using a keybind and the mail is out of
  view.
- if for some reason my performed action was insufficient and I am
  notified by a method other than mail, I forward the mail to myself
  adding a note of what I am supposed to do (because I will not remember
  it the next day)
- cycle repeats

If you see any holes in that approach, feel free to also directly
contact me in order to keep the mailing list somewhat on topic. :)

-- 
Moritz Poldrack
https://moritz.sh
Details
Message ID
<CLC4DZQ6W1XU.3UDATWOY1HO25@moth.falsifian.org>
In-Reply-To
<CLC3W7D71TSY.WLIXVX3UO4SJ@Archetype> (view parent)
DKIM signature
pass
Download raw message
Thanks for your thoughts, Moritz. I'm happy to defer to you; in any case
I'm not using the IMAP backend.

I can try adding the change to unset \Seen myself, but probably won't
have time until next weekend, so if you or someone else wants to do the
work, please go ahead. (We could also just say the setting is not
supported with IMAP.)

> >   I really don't relish the idea of accidentally marking an email as
> >   \Seen --- with my workflow, that means the email drops off my radar,
> >   and it might have been important.
> I'd suggest using maildir in this case. Isync can – if I remember
> correctly – sync the mails state back to the origin server.

Indeed, I'm using the notmuch backend with maildir+isync.

> > Off-topic:
> >
> > I have thought a few times of using an "Archive" folder instead of
> > read/unread status, because I feel like I'm working against the grain.
> > I haven't switched for two reasons:
> > (a) I don't like the idea of re-transferring a (potentially large)
> > email a couple of times just because I decided to Archive it (mbsync,
> > at least, isn't smart enough to move instead of copying);
> The IMAP backend (which is up for discussion here) is smart enough to
> just tell the Server to move the messages, so for IMAP there's no
> problem. For mbsync this patchset would just fix that issue for you,
> wouldn't it?

Right, since I don't use the IMAP backend, it doesn't really matter to
me.

-- 
James
Details
Message ID
<CLC5HG2HGN45.22PPJNDTCDYNA@TimBook-Arch>
In-Reply-To
<CLC4DZQ6W1XU.3UDATWOY1HO25@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
Patch: +8 -0
On Sun Jul 10, 2022 at 11:34 AM CDT, James Cook wrote:
> I can try adding the change to unset \Seen myself, but probably won't
> have time until next weekend, so if you or someone else wants to do the
> work, please go ahead. (We could also just say the setting is not
> supported with IMAP.)

First, I think we should support the feature for all workers.

Second, I really think an approach like below is best. In both cases, it's only
one call to the imap server and is very few lines of code to implement.

diff --git a/worker/imap/fetch.go b/worker/imap/fetch.go
index 7d7b72f..5beb886 100644
--- a/worker/imap/fetch.go
+++ b/worker/imap/fetch.go
@@ -96,6 +96,9 @@ func (imapw *IMAPWorker) handleFetchMessageBodyPart(
		partBodySection.Specifier = imap.TextSpecifier
	}
	partBodySection.Path = msg.Part
	if msg.Peek {
		partBodySection.Peek = true
	}

	items := []imap.FetchItem{
		imap.FetchEnvelope,
@@ -150,6 +153,9 @@ func (imapw *IMAPWorker) handleFetchFullMessages(

	imapw.worker.Logger.Printf("Fetching full messages")
	section := &imap.BodySectionName{}
	if msg.Peek {
		section.Peek = true
	}
	items := []imap.FetchItem{
		imap.FetchEnvelope,
		imap.FetchFlags,
diff --git a/worker/types/messages.go b/worker/types/messages.go
index e303ade..67658c5 100644
--- a/worker/types/messages.go
+++ b/worker/types/messages.go
@@ -125,12 +125,14 @@ type FetchMessageHeaders struct {
type FetchFullMessages struct {
	Message
	Uids []uint32
	Peek bool
}

type FetchMessageBodyPart struct {
	Message
	Uid  uint32
	Part []int
	Peek bool
}

type FetchMessageFlags struct {
Details
Message ID
<CLC6BQBXPQVE.1SDWWMKH4GW7Q@moon3>
In-Reply-To
<CLC4DZQ6W1XU.3UDATWOY1HO25@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sun Jul 10, 2022 at 6:34 PM CEST, James Cook wrote:
> Thanks for your thoughts, Moritz. I'm happy to defer to you; in any case
> I'm not using the IMAP backend.
>
> I can try adding the change to unset \Seen myself, but probably won't
> have time until next weekend, so if you or someone else wants to do the
> work, please go ahead. (We could also just say the setting is not
> supported with IMAP.)
>
> > >   I really don't relish the idea of accidentally marking an email as
> > >   \Seen --- with my workflow, that means the email drops off my radar,
> > >   and it might have been important.
> > I'd suggest using maildir in this case. Isync can – if I remember
> > correctly – sync the mails state back to the origin server.
>
> Indeed, I'm using the notmuch backend with maildir+isync.

Sorry for jumping into this conversation but when you use notmuch, you
can already work with custom labels/tags that support your workflow I
believe.

What I do is use notmuch's post-new hook where I tag new messages with
whatever label I want. In the post-new-hook script I would have a line
like this:

notmuch tag +mycustomlabel -new -- tag:new

This will set "mycustomlabel" to all new messages in notmuch. 

In aerc you could then use ":cf tag:mycustomlabel" to see all those
tagged messages (or use the query map). 

With the :modify-labels command you can then remove this label and set
another one depending on your workflow, :modify-labels -mycustomlabel
+seen

Hope this helps.

>
> > > Off-topic:
> > >
> > > I have thought a few times of using an "Archive" folder instead of
> > > read/unread status, because I feel like I'm working against the grain.
> > > I haven't switched for two reasons:
> > > (a) I don't like the idea of re-transferring a (potentially large)
> > > email a couple of times just because I decided to Archive it (mbsync,
> > > at least, isn't smart enough to move instead of copying);
> > The IMAP backend (which is up for discussion here) is smart enough to
> > just tell the Server to move the messages, so for IMAP there's no
> > problem. For mbsync this patchset would just fix that issue for you,
> > wouldn't it?
>
> Right, since I don't use the IMAP backend, it doesn't really matter to
> me.
>
> -- 
> James
Details
Message ID
<CLC6R7U7HXHI.HWEUHR6XAYV5@marty>
In-Reply-To
<20220705060618.68685-1-falsifian@falsifian.org> (view parent)
DKIM signature
missing
Download raw message
James Cook, Jul 05, 2022 at 08:06:
> Fixes: https://todo.sr.ht/~rjarry/aerc/48
> Signed-off-by: James Cook <falsifian@falsifian.org>
> ---
> Sending this out for feedback. I should probably test it at least a
> little more; e.g. I haven't tried deleting a message to check that the
> change to delete.go makes sense.

Hey James,

The concept looks nice.

Having the option in UI is weird but it allows contextual configuration
(per-account, folder, etc.), so I guess I can live with it. I was never
fond of this dependency between aerc.conf and accounts.conf. Ideally
these two files should be completely independent. Any account/folder
specific stuff should be in accounts.conf.

Anyways, I agree with Tim about conditionally setting the Peek flag when
fetching the message body in IMAP when auto-mark-read=false. That way,
no need to worry about setting or un-setting that \Seen flag.

Cheers

notmuch workflow (Was [PATCH aerc] Allow not marking viewed messages as seen.)

Details
Message ID
<CLCIKSB3I4Q1.1ZC4D0I43583O@moth.falsifian.org>
In-Reply-To
<CLC6BQBXPQVE.1SDWWMKH4GW7Q@moon3> (view parent)
DKIM signature
pass
Download raw message
(List to bcc; my reply at bottom. If you're not interested in notmuch
workflows you can skip this.)

> Sorry for jumping into this conversation but when you use notmuch, you
> can already work with custom labels/tags that support your workflow I
> believe.
>
> What I do is use notmuch's post-new hook where I tag new messages with
> whatever label I want. In the post-new-hook script I would have a line
> like this:
>
> notmuch tag +mycustomlabel -new -- tag:new
>
> This will set "mycustomlabel" to all new messages in notmuch. 
>
> In aerc you could then use ":cf tag:mycustomlabel" to see all those
> tagged messages (or use the query map). 
>
> With the :modify-labels command you can then remove this label and set
> another one depending on your workflow, :modify-labels -mycustomlabel
> +seen
>
> Hope this helps.

Thank you. I already know notmuch can do this. The problem is
synchronizing the information so it's not just in my one computer's
notmuch database. Specifically:

0. I often check my email on my Android phone. I'd like to mark things as
   "done", and conversely see which messages are not yet "done".

1. I like to keep important information backed up, and that includes the
   "done" status of my emails.

2. I would like to be able to check my email from other devices (though
   in practice these days it's only phone and laptop).

I may be able to address 1 and 2 by synchronizing notmuch's tag
database, but it sounds like a mess, so I'm not doing that.

Because of these concerns, I've adopted a policy of only using notmuch
tags that are backed in some way by the backend (with limited exceptions
for short-lived tags). I do use a "spam" tag, but that's backed by
moving things in/out of a Spam folder. (I could do the same thing with
an "Inbox" or "Archive" tag, but see earlier in the thread for a couple
of reasons I don't do that.)

Since I brought it up, in case it's useful to anyone else, here are my
notmuch hooks for synchronizing the "spam" tag with being in the "Spam"
folder. The same technique could be used for an "Archive" tag. Probably
similar things have been posted to the notmuch list and wiki.

pre-new:

	#!/bin/sh
	set -e

	mail_root=$HOME/var/mbsync/falsifian.org

	# Copy spam status from spam tag to folder location. We do this in the "pre"
	# rather than "post" hook so that if mbsync moved a message in or out of Spam,
	# we don't see it yet, so we don't try to override it.

	# This hook may fail if a message's spam status was changed both externally
	# (mbsync moved it in or out of Spam) or internally ("spam" tag added or
	# removed), so don't do that.

	# Move spam to the Spam folder.
	notmuch search --output=files tag:spam not folder:Spam \
		| xargs mv_losing_uid_to -- "$mail_root/Spam/cur"

	# Move non-spam out of the Spam folder.
	notmuch search --output=files folder:Spam not tag:spam \
		| xargs mv_losing_uid_to -- "$mail_root/INBOX/cur"

	# This is a good time to run mbsync: after we've updated paths to match tags,
	# but before notmuch scans for changes. Running it at a different time should
	# be okay; the advantage of running it here is that everything should get synced
	# with just one run of "notmuch new".
	mbsync falsifian.org

post-new:

	#!/bin/sh
	set -e

	# Update the "spam" tag to match file locations.
	notmuch tag +spam folder:Spam not tag:spam
	notmuch tag -spam tag:spam not folder:Spam

And here's my "mv_losing_uid_to" script; I gather something like this is
needed to avoid confusing isync (mbsync):

	#!/usr/bin/env raku
	#
	# Usage:
	#     mv_losing_uid_to [--] dir file...
	#
	# Moves the files to dir, removing the ",U=..." part from their filenames if
	# present. Fails if the filenames don't look like Maildir filenames.
	#
	# Inspired by the safeMove function from
	# https://notmuchmail.org/pipermail/notmuch/2019/028956.html
	# except I don't want to lose the :info suffix, and I want to be extra-careful
	# by failing if it looks like we're being run on the wrong kind of file.

	sub die_note(Str $message) { note $message; exit 1; }

	shift @*ARGS if @*ARGS[0] eq '--';
	die_note 'Required first argument is missing.' if @*ARGS.elems == 0;
	my IO::Path $destination_dir := @*ARGS[0].IO;
	die_note 'Destination does not exist or is not a directory.'
		unless $destination_dir.d;

	# Compute all the destination paths first, so that if any of the source paths
	# don't match our expectations, we abort the whole operation.
	my Pair @renames =
	( for @*ARGS[1..*]
	{
		my regex filename_pattern
		{
			# Make sure the old filename matches our expectation. If it
			# doesn't, something has gone wrong, and we should not proceed.
			^
			$<before> = [ <-[,.:]> * '.' <-[,.:]> * '.' <-[,.:]> * ]
			[ ',U=' <[ 0..9 ]> + ]?  # UID to discard
			$<after> = [ ':' <-[:]> * ] ?
			$
		};
		die_note
			"Source $_ does not exist or is not a regular file. "
			~ 'No files were moved.'
			unless .IO.f;
		my $basename := .IO.basename;
		my $match := $basename ~~ &filename_pattern
			or die_note
			"{$basename.raku} doesn't look like a Maildir filename. "
			~ 'No files were moved.';
		$_ => $destination_dir.add("$match<before>$match<after>");
	});

	for @renames
	{
		# The destination should not exist already. Pass -i just in case. Even
		# if this is being run non-interactively, better to fail and have
		# useless "overwrite x?" messages appear than to overwrite.
		run <mv -i -->, $_.key, $_.value
			or die_note "Failed to move {.key} to {.value}.";
	}

-- 
James

[PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<20220820002218.43914-1-falsifian@falsifian.org>
In-Reply-To
<CLC6R7U7HXHI.HWEUHR6XAYV5@marty> (view parent)
DKIM signature
pass
Download raw message
Patch: +27 -7
This commit adds a new configuration option, auto-mark-read, which
defaults to true. When set to false, emails are not marked as seen when
viewed.

We also fetch BODY.PEEK[...] instead of BODY when fetching messages with
IMAP. This means messages are only marked as read when we deliberately
set the flag (which we automatically anyway, as long as
auto-mark-read=true).

I imagine before this change the export-mbox command caused all messages
in an IMAP folder to be marked as seen.

Fixes: https://todo.sr.ht/~rjarry/aerc/48
Signed-off-by: James Cook <falsifian@falsifian.org>

Squashed commit of the following:

commit 9ca6eda82438c0ee0836afb91ec2be9cdce2b393
Author: James Cook <falsifian@falsifian.org>
Date:   Fri Aug 19 23:51:26 2022 +0000

    Use PEEK with IMAP.

commit a49578363c989825fd2e1af1e404adcc8cf60d5a
Author: James Cook <falsifian@falsifian.org>
Date:   Fri Aug 19 22:55:48 2022 +0000

    Post-merge fix: Pass UI config.

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
---
 commands/account/view.go | 2 +-
 commands/msg/delete.go   | 1 +
 commands/msg/recall.go   | 2 +-
 commands/msgview/next.go | 2 +-
 config/aerc.conf         | 6 ++++++
 config/config.go         | 2 ++
 doc/aerc-config.5.scd    | 5 +++++
 lib/messageview.go       | 7 +++++--
 widgets/msglist.go       | 2 +-
 worker/imap/fetch.go     | 5 ++++-
 10 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/commands/account/view.go b/commands/account/view.go
index 8537d33..04a1687 100644
--- a/commands/account/view.go
+++ b/commands/account/view.go
@@ -46,7 +46,7 @@ func (ViewMessage) Execute(aerc *widgets.Aerc, args []string) error {
		return nil
	}
	lib.NewMessageStoreView(msg, store, aerc.Crypto, aerc.DecryptKeys,
		func(view lib.MessageView, err error) {
		acct.UiConfig(), func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
				return
diff --git a/commands/msg/delete.go b/commands/msg/delete.go
index ceb570b..4c3b7f0 100644
--- a/commands/msg/delete.go
+++ b/commands/msg/delete.go
@@ -63,6 +63,7 @@ func (Delete) Execute(aerc *widgets.Aerc, args []string) error {
						return
					}
					lib.NewMessageStoreView(next, store, aerc.Crypto, aerc.DecryptKeys,
						acct.UiConfig(),
						func(view lib.MessageView, err error) {
							if err != nil {
								aerc.PushError(err.Error())
diff --git a/commands/msg/recall.go b/commands/msg/recall.go
index 5fc3a26..578f910 100644
--- a/commands/msg/recall.go
+++ b/commands/msg/recall.go
@@ -137,7 +137,7 @@ func (Recall) Execute(aerc *widgets.Aerc, args []string) error {
		})
	}

	lib.NewMessageStoreView(msgInfo, store, aerc.Crypto, aerc.DecryptKeys,
	lib.NewMessageStoreView(msgInfo, store, aerc.Crypto, aerc.DecryptKeys, acct.UiConfig(),
		func(msg lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
diff --git a/commands/msgview/next.go b/commands/msgview/next.go
index c80e0ab..5114031 100644
--- a/commands/msgview/next.go
+++ b/commands/msgview/next.go
@@ -43,7 +43,7 @@ func (NextPrevMsg) Execute(aerc *widgets.Aerc, args []string) error {
		return nil
	}
	lib.NewMessageStoreView(nextMsg, store, aerc.Crypto, aerc.DecryptKeys,
		func(view lib.MessageView, err error) {
		acct.UiConfig(), func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
				return
diff --git a/config/aerc.conf b/config/aerc.conf
index fc6479a..0f03aa0 100644
--- a/config/aerc.conf
+++ b/config/aerc.conf
@@ -18,6 +18,12 @@ pgp-provider=internal
unsafe-accounts-conf=false

[ui]
#
# Set the "seen" flag when a message is viewed.
#
# Default: true
auto-mark-read=true

#
# Describes the format for each row in a mailbox view. This field is compatible
# with mutt's printf-like syntax.
diff --git a/config/config.go b/config/config.go
index 66c6dd1..cfabc44 100644
--- a/config/config.go
+++ b/config/config.go
@@ -33,6 +33,7 @@ type GeneralConfig struct {
}

type UIConfig struct {
	AutoMarkRead        bool          `ini:"auto-mark-read"`
	IndexFormat         string        `ini:"index-format"`
	TimestampFormat     string        `ini:"timestamp-format"`
	ThisDayTimeFormat   string        `ini:"this-day-time-format"`
@@ -710,6 +711,7 @@ func LoadConfigFromFile(root *string) (*AercConfig, error) {
		},

		Ui: UIConfig{
			AutoMarkRead:       true,
			IndexFormat:        "%D %-17.17n %s",
			TimestampFormat:    "2006-01-02 03:04 PM",
			ThisDayTimeFormat:  "",
diff --git a/doc/aerc-config.5.scd b/doc/aerc-config.5.scd
index aaf15b8..fe825de 100644
--- a/doc/aerc-config.5.scd
+++ b/doc/aerc-config.5.scd
@@ -97,6 +97,11 @@ These options are configured in the *[ui]* section of aerc.conf.
|  %Z
:  flags (O=old, N=new, r=answered, D=deleted, !=flagged, \*=marked)

*auto-mark-read*
	Set the "seen" flag when a message is viewed.

	Default: true

*timestamp-format*
	See time.Time#Format at https://godoc.org/time#Time.Format

diff --git a/lib/messageview.go b/lib/messageview.go
index e0e86ea..597e58d 100644
--- a/lib/messageview.go
+++ b/lib/messageview.go
@@ -9,6 +9,7 @@ import (
	"github.com/emersion/go-message"
	_ "github.com/emersion/go-message/charset"

	"git.sr.ht/~rjarry/aerc/config"
	"git.sr.ht/~rjarry/aerc/lib/crypto"
	"git.sr.ht/~rjarry/aerc/models"
	"git.sr.ht/~rjarry/aerc/worker/lib"
@@ -62,7 +63,7 @@ type MessageStoreView struct {

func NewMessageStoreView(messageInfo *models.MessageInfo,
	store *MessageStore, pgp crypto.Provider, decryptKeys openpgp.PromptFunction,
	cb func(MessageView, error),
	uiConfig *config.UIConfig, cb func(MessageView, error),
) {
	msv := &MessageStoreView{
		messageInfo, store,
@@ -99,7 +100,9 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
	} else {
		cb(msv, nil)
	}
	store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)
	if uiConfig.AutoMarkRead {
		store.Flag([]uint32{messageInfo.Uid}, models.SeenFlag, true, nil)
	}
}

func (msv *MessageStoreView) MessageInfo() *models.MessageInfo {
diff --git a/widgets/msglist.go b/widgets/msglist.go
index e431c2e..dfd6f34 100644
--- a/widgets/msglist.go
+++ b/widgets/msglist.go
@@ -307,7 +307,7 @@ func (ml *MessageList) MouseEvent(localX int, localY int, event tcell.Event) {
					return
				}
				lib.NewMessageStoreView(msg, store, ml.aerc.Crypto,
					ml.aerc.DecryptKeys,
					ml.aerc.DecryptKeys, acct.UiConfig(),
					func(view lib.MessageView, err error) {
						if err != nil {
							ml.aerc.PushError(err.Error())
diff --git a/worker/imap/fetch.go b/worker/imap/fetch.go
index f21c6e9..0d2bb16 100644
--- a/worker/imap/fetch.go
+++ b/worker/imap/fetch.go
@@ -91,6 +91,7 @@ func (imapw *IMAPWorker) handleFetchMessageBodyPart(
	partHeaderSection.Path = msg.Part

	var partBodySection imap.BodySectionName
	partBodySection.Peek = true
	if len(msg.Part) > 0 {
		partBodySection.Specifier = imap.EntireSpecifier
	} else {
@@ -150,7 +151,9 @@ func (imapw *IMAPWorker) handleFetchFullMessages(
	msg *types.FetchFullMessages,
) {
	logging.Infof("Fetching full messages: %v", msg.Uids)
	section := &imap.BodySectionName{}
	section := &imap.BodySectionName{
		Peek: true,
	}
	items := []imap.FetchItem{
		imap.FetchEnvelope,
		imap.FetchFlags,
-- 
2.37.2

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAFL4ZDR55J.17G3PA2UC6R2S@moth.falsifian.org>
In-Reply-To
<20220820002218.43914-1-falsifian@falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 12:22 AM UTC, James Cook wrote:
> This commit adds a new configuration option, auto-mark-read, which
> defaults to true. When set to false, emails are not marked as seen when
> viewed.
>
> We also fetch BODY.PEEK[...] instead of BODY when fetching messages with
> IMAP. This means messages are only marked as read when we deliberately
> set the flag (which we automatically anyway, as long as
> auto-mark-read=true).
>
> I imagine before this change the export-mbox command caused all messages
> in an IMAP folder to be marked as seen.
>
> Fixes: https://todo.sr.ht/~rjarry/aerc/48
> Signed-off-by: James Cook <falsifian@falsifian.org>

This patch unconditionally peeks, based on the patch Tim sent*. I chose
to make in unconditional for two reasons:

1. aerc already explicitly sets the Seen flag (now as long as
   auto-mark-read is false).

2. I started integrating Tim's suggested change which adds a "Peek"
   field to two structs, but I found it added a fair bit of plumbing,
   because it means whoever is calling the functions that generate those
   structs needs to pass an extra parameter saying whether Peek should
   be true. It felt unnecessarily complicated given that my patch is
   already separately plumbing through the auto-mark-read option.

See the comment about export-mbox in the commit message. I considered
updating the documentaton to export-mbox to explicitly say it won't
cause things to be marked as read, but it seems to me a reasonably user
would assume that behaviour anyway. (I didn't actually confirm that the
previous behaviour was to mark everything read.)

*I should have acknowledged Tim in the commit message; if you commit
this version, please add some acknowledgement like "Thanks to Tim
Culverhouse for the IMAP PEEK code" to the commit message.


-- 
James

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAOGU0XV5HK.AVX8CZ2GH384@Archetype>
In-Reply-To
<20220820002218.43914-1-falsifian@falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 2:22 AM CEST, James Cook wrote:
> This commit adds a new configuration option, auto-mark-read, which
> defaults to true. When set to false, emails are not marked as seen when
> viewed.
>
> We also fetch BODY.PEEK[...] instead of BODY when fetching messages with
> IMAP. This means messages are only marked as read when we deliberately
> set the flag (which we automatically anyway, as long as
> auto-mark-read=true).
>
> I imagine before this change the export-mbox command caused all messages
> in an IMAP folder to be marked as seen.
>
> Fixes: https://todo.sr.ht/~rjarry/aerc/48
> Signed-off-by: James Cook <falsifian@falsifian.org>
>
> Squashed commit of the following:
>
> commit 9ca6eda82438c0ee0836afb91ec2be9cdce2b393
> Author: James Cook <falsifian@falsifian.org>
> Date:   Fri Aug 19 23:51:26 2022 +0000
>
>     Use PEEK with IMAP.
>
> commit a49578363c989825fd2e1af1e404adcc8cf60d5a
> Author: James Cook <falsifian@falsifian.org>
> Date:   Fri Aug 19 22:55:48 2022 +0000
>
>     Post-merge fix: Pass UI config.
>
> 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?

>  commands/account/view.go | 2 +-
>  commands/msg/delete.go   | 1 +
>  commands/msg/recall.go   | 2 +-
>  commands/msgview/next.go | 2 +-
>  config/aerc.conf         | 6 ++++++
>  config/config.go         | 2 ++
>  doc/aerc-config.5.scd    | 5 +++++
>  lib/messageview.go       | 7 +++++--
>  widgets/msglist.go       | 2 +-
>  worker/imap/fetch.go     | 5 ++++-
>  10 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/worker/imap/fetch.go b/worker/imap/fetch.go
> index f21c6e9..0d2bb16 100644
> --- a/worker/imap/fetch.go
> +++ b/worker/imap/fetch.go
> @@ -91,6 +91,7 @@ func (imapw *IMAPWorker) handleFetchMessageBodyPart(
>  	partHeaderSection.Path = msg.Part
>  
>  	var partBodySection imap.BodySectionName
> +	partBodySection.Peek = true
>  	if len(msg.Part) > 0 {
>  		partBodySection.Specifier = imap.EntireSpecifier
>  	} else {
> @@ -150,7 +151,9 @@ func (imapw *IMAPWorker) handleFetchFullMessages(
>  	msg *types.FetchFullMessages,
>  ) {
>  	logging.Infof("Fetching full messages: %v", msg.Uids)
> -	section := &imap.BodySectionName{}
> +	section := &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.

> +	}
>  	items := []imap.FetchItem{
>  		imap.FetchEnvelope,
>  		imap.FetchFlags,
> -- 
> 2.37.2

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAOKVMBPH49.2P4GWZFH7H8NH@Archetype>
In-Reply-To
<20220820002218.43914-1-falsifian@falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 2:22 AM CEST, James Cook wrote:
> diff --git a/doc/aerc-config.5.scd b/doc/aerc-config.5.scd
> index aaf15b8..fe825de 100644
> --- a/doc/aerc-config.5.scd
> +++ b/doc/aerc-config.5.scd
> @@ -97,6 +97,11 @@ These options are configured in the *[ui]* section of aerc.conf.
>  |  %Z
>  :  flags (O=old, N=new, r=answered, D=deleted, !=flagged, \*=marked)
>  
> +*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

> +
> +	Default: true
> +
>  *timestamp-format*
>  	See time.Time#Format at https://godoc.org/time#Time.Format
>  

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAOP5EJ5ZFF.22H9I2LPREU3Y@Archetype>
In-Reply-To
<20220820002218.43914-1-falsifian@falsifian.org> (view parent)
DKIM signature
pass
Download raw message
Please also send your new revisions as a v2, v3, … vn :)
-- 
Moritz Poldrack
https://moritz.sh

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org>
In-Reply-To
<CMAOGU0XV5HK.AVX8CZ2GH384@Archetype> (view parent)
DKIM signature
pass
Download raw message
(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, … 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 := &imap.BodySectionName{}
> > +	section := &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,
		}
	 }
	 
	-func (store *MessageStore) FetchFull(uids []uint32, cb func(*types.FullMessage)) {
	+func (store *MessageStore) FetchFull(uids []uint32, peek bool, cb func(*types.FullMessage)) {
		// TODO: this could be optimized by pre-allocating toFetch and trimming it
		// at the end. In practice we expect to get most messages back in one frame.
		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 := msg.(*types.Error); ok {
					for _, uid := range toFetch {
	@@ -162,10 +163,11 @@ func (store *MessageStore) FetchFull(uids []uint32, cb func(*types.FullMessage))
		}
	 }
	 
	-func (store *MessageStore) FetchBodyPart(uid uint32, part []int, cb func(io.Reader)) {
	+func (store *MessageStore) FetchBodyPart(uid uint32, part []int, peek bool, cb func(io.Reader)) {
		store.worker.PostAction(&types.FetchMessageBodyPart{
			Uid:  uid,
			Part: part,
	+		Peek: peek,
		}, func(resp types.WorkerMessage) {
			msg, ok := 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=false 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=false 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.

-- 
James

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAX6JEI6EQM.3ASPP3JL5J3RK@guix-aspire>
In-Reply-To
<CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
(Sorry for butting in... :))

On Sat Aug 20, 2022 at 3:15 PM BST, James Cook wrote:
> Where should I write v2, v3, etc?

Git's `send-email` provides a `-v` flag:

---
# Send a v2 of the last 10 commits to blah@foobar.dev, with
# a cover letter preceding the patchset.
git send-email -10 --to=blah@foobar.dev -v2 --cover-letter
---

    -- (

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<B922166F-1253-460B-B8F5-AA6FAAA610C3@jarry.cc>
In-Reply-To
<CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On August 20, 2022 4:15:20 PM GMT+02:00, James Cook <falsifian@falsifian.org> wrote:
> 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.)

https://git.sr.ht/~rjarry/aerc/tree/master/item/CONTRIBUTING.md

Please read this. Everything you need to know is there.

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAXRYNX38L8.33IA8OO53PC7U@TimBook-Arch>
In-Reply-To
<CMAX3QJA8ATV.1NSRFYM2OEALH@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
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=false 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

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAXWJ5PG9HB.2IHRKOQBVLX6V@TimBook-Arch>
In-Reply-To
<CMAXRYNX38L8.33IA8OO53PC7U@TimBook-Arch> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 9:46 AM CDT, Tim Culverhouse wrote:
> 2. Add PEEK option to the worker message and all relevant function
> calls, with the fix to the ones that should be true

Actually, with this approach I think it should be called "SetSeenFlag"
or something to that effect. Peek is only a thing in IMAP and isn't very
clear what it's purpose is just from the name

Tim

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMAZSN3IS514.QRSX05V6IXT8@moth.falsifian.org>
In-Reply-To
<CMAXRYNX38L8.33IA8OO53PC7U@TimBook-Arch> (view parent)
DKIM signature
pass
Download raw message
> 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

I like this plan, except I wonder if #1 and #2 should be done in the
same patch. If I understand you right, after #1, every message fetch
would cause it to be marked as read, including e.g. running :pipe, so
non-IMAP backends would get a little bit broken until step #2 is
implemented.

-- 
James

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMB2TOHJV37C.1OV8OOXSWQB5N@Archetype>
In-Reply-To
<CMAZSN3IS514.QRSX05V6IXT8@moth.falsifian.org> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 6:21 PM CEST, James Cook wrote:
> > 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
>
> I like this plan, except I wonder if #1 and #2 should be done in the
> same patch.
I don't think that merging them makes a lot of sense… I think their
topics are different enough to warrant multiple commits.

> If I understand you right, after #1, every message fetch would cause
> it to be marked as read, including e.g. running :pipe, so non-IMAP
> backends would get a little bit broken until step #2 is implemented.
Not necessarily. While extracting you can already add a way to pass
whether to mark as read or not. Otherwise I don't think it matters if
one patch in between changes the default behaviour slightly when the
next commit already restores said standard.

Re: [PATCH aerc] Use IMAP PEEK and allow not marking messages read.

Details
Message ID
<CMB37LW8ACBY.16SJEJRIROCZH@TimBook-Arch>
In-Reply-To
<CMB2TOHJV37C.1OV8OOXSWQB5N@Archetype> (view parent)
DKIM signature
pass
Download raw message
On Sat Aug 20, 2022 at 1:44 PM CDT, Moritz Poldrack wrote:
> > I like this plan, except I wonder if #1 and #2 should be done in the
> > same patch.
> I don't think that merging them makes a lot of sense… I think their
> topics are different enough to warrant multiple commits.

I think it makes sense to combine...Not combining breaks between
commits, while combining implements the fix.

1. Move store.Flag to workers, and make it so that everything that
*shouldn't* mark as read, does so. Behavior the same for
maildir/notmuch/mbox, and a fix for imap.

2. Add config option to leave message unread when opening. Works the
same for all backends.
Reply to thread Export thread (mbox)