~sircmpwn/aerc

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

maildir: add recent handling

Details
Message ID
<20200914051045.1726954-1-reto@labrat.space>
DKIM signature
pass
Download raw message
This should fix some of the issues with my first patch set, in that the counts
didn't get properly updated in the msglist.

It also includes the count fix from Galen.

If this works, I'd suggest we squash the 3 commits, would you mind Galen?

Galen Abell (1):
  maildir: Don't add recents to unread and msg count

Reto Brunner (2):
  maildir: track the recent flag correctly
  maildir: fix updating of recent counts

 worker/maildir/container.go | 40 ++++++++++++++-----
 worker/maildir/worker.go    | 77 ++++++++++++++++++++++---------------
 2 files changed, 77 insertions(+), 40 deletions(-)

-- 
2.28.0

[PATCH 1/3] maildir: track the recent flag correctly

Details
Message ID
<20200914051045.1726954-2-reto@labrat.space>
In-Reply-To
<20200914051045.1726954-1-reto@labrat.space> (view parent)
DKIM signature
pass
Download raw message
Patch: +55 -31
In the maildir worker we manually need to track the Recent flag in order for the
notification command etc to work.

Push that responsibility to the container, we must make sure to manually add the
flag though if one grabs the message info.
---
 worker/maildir/container.go | 40 ++++++++++++++++++++++++--------
 worker/maildir/worker.go    | 46 +++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/worker/maildir/container.go b/worker/maildir/container.go
index 14815c9..1bdc4e7 100644
--- a/worker/maildir/container.go
+++ b/worker/maildir/container.go
@@ -15,9 +15,10 @@ import (
// A Container is a directory which contains other directories which adhere to
// the Maildir spec
type Container struct {
	dir  string
	log  *log.Logger
	uids *uidstore.Store
	dir        string
	log        *log.Logger
	uids       *uidstore.Store
	recentUIDS map[uint32]struct{} // used to set the recent flag
}

// NewContainer creates a new container at the specified directory
@@ -34,7 +35,8 @@ func NewContainer(dir string, l *log.Logger) (*Container, error) {
	if !s.IsDir() {
		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
	}
	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}, nil
	return &Container{dir: dir, uids: uidstore.NewStore(), log: l,
		recentUIDS: make(map[uint32]struct{})}, nil
}

// ListFolders returns a list of maildir folders in the container
@@ -72,17 +74,26 @@ func (c *Container) ListFolders() ([]string, error) {
	return folders, err
}

// SyncNewMail adds emails from new to cur, tracking them
func (c *Container) SyncNewMail(dir maildir.Dir) error {
	keys, err := dir.Unseen()
	if err != nil {
		return err
	}
	for _, key := range keys {
		uid := c.uids.GetOrInsert(key)
		c.recentUIDS[uid] = struct{}{}
	}
	return nil
}

// OpenDirectory opens an existing maildir in the container by name, moves new
// messages into cur, and registers the new keys in the UIDStore.
func (c *Container) OpenDirectory(name string) (maildir.Dir, error) {
	dir := c.Dir(name)
	keys, err := dir.Unseen()
	if err != nil {
	if err := c.SyncNewMail(dir); err != nil {
		return dir, err
	}
	for _, key := range keys {
		c.uids.GetOrInsert(key)
	}
	return dir, nil
}

@@ -91,6 +102,17 @@ func (c *Container) Dir(name string) maildir.Dir {
	return maildir.Dir(filepath.Join(c.dir, name))
}

// IsRecent returns if a uid has the Recent flag set
func (c *Container) IsRecent(uid uint32) bool {
	_, ok := c.recentUIDS[uid]
	return ok
}

// ClearRecentFlag removes the Recent flag from the message with the given uid
func (c *Container) ClearRecentFlag(uid uint32) {
	delete(c.recentUIDS, uid)
}

// UIDs fetches the unique message identifiers for the maildir
func (c *Container) UIDs(d maildir.Dir) ([]uint32, error) {
	keys, err := d.Keys()
diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index 4a7ae51..cf08e70 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -79,11 +79,12 @@ func (w *Worker) handleFSEvent(ev fsnotify.Event) {
	if w.selected == nil {
		return
	}
	newUnseen, err := w.selected.Unseen()
	err := w.c.SyncNewMail(*w.selected)
	if err != nil {
		w.worker.Logger.Printf("could not move new to cur : %v", err)
		return
	}

	uids, err := w.c.UIDs(*w.selected)
	if err != nil {
		w.worker.Logger.Printf("could not scan UIDs: %v", err)
@@ -98,7 +99,6 @@ func (w *Worker) handleFSEvent(ev fsnotify.Event) {
		Uids: sortedUids,
	}, nil)
	dirInfo := w.getDirectoryInfo(w.selectedName)
	dirInfo.Recent = len(newUnseen)
	w.worker.PostMessage(&types.DirectoryInfo{
		Info: dirInfo,
	}, nil)
@@ -138,12 +138,6 @@ func (w *Worker) getDirectoryInfo(name string) *models.DirectoryInfo {
		return dirInfo
	}

	recent, err := dir.UnseenCount()
	if err != nil {
		w.worker.Logger.Printf("could not get unseen count: %v", err)
	}
	dirInfo.Recent = recent

	for _, uid := range uids {
		message, err := w.c.Message(dir, uid)
		if err != nil {
@@ -164,9 +158,12 @@ func (w *Worker) getDirectoryInfo(name string) *models.DirectoryInfo {
		if !seen {
			dirInfo.Unseen++
		}
		if w.c.IsRecent(uid) {
			dirInfo.Recent++
		}
	}
	dirInfo.Unseen += dirInfo.Recent
	dirInfo.Exists = len(uids) + recent
	dirInfo.Exists = len(uids) + dirInfo.Recent
	return dirInfo
}

@@ -334,12 +331,7 @@ func (w *Worker) sort(uids []uint32, criteria []*types.SortCriterion) ([]uint32,
	}
	var msgInfos []*models.MessageInfo
	for _, uid := range uids {
		m, err := w.c.Message(*w.selected, uid)
		if err != nil {
			w.worker.Logger.Printf("could not get message: %v", err)
			continue
		}
		info, err := m.MessageInfo()
		info, err := w.msgInfoFromUid(uid)
		if err != nil {
			w.worker.Logger.Printf("could not get message info: %v", err)
			continue
@@ -377,13 +369,7 @@ func (w *Worker) handleRemoveDirectory(msg *types.RemoveDirectory) error {
func (w *Worker) handleFetchMessageHeaders(
	msg *types.FetchMessageHeaders) error {
	for _, uid := range msg.Uids {
		m, err := w.c.Message(*w.selected, uid)
		if err != nil {
			w.worker.Logger.Printf("could not get message: %v", err)
			w.err(msg, err)
			continue
		}
		info, err := m.MessageInfo()
		info, err := w.msgInfoFromUid(uid)
		if err != nil {
			w.worker.Logger.Printf("could not get message info: %v", err)
			w.err(msg, err)
@@ -393,6 +379,7 @@ func (w *Worker) handleFetchMessageHeaders(
			Message: types.RespondTo(msg),
			Info:    info,
		}, nil)
		w.c.ClearRecentFlag(uid)
	}
	return nil
}
@@ -590,3 +577,18 @@ func (w *Worker) handleSearchDirectory(msg *types.SearchDirectory) error {
	}, nil)
	return nil
}

func (w *Worker) msgInfoFromUid(uid uint32) (*models.MessageInfo, error) {
	m, err := w.c.Message(*w.selected, uid)
	if err != nil {
		return nil, err
	}
	info, err := m.MessageInfo()
	if err != nil {
		return nil, err
	}
	if w.c.IsRecent(uid) {
		info.Flags = append(info.Flags, models.RecentFlag)
	}
	return info, nil
}
-- 
2.28.0

[PATCH 2/3] maildir: Don't add recents to unread and msg count

Details
Message ID
<20200914051045.1726954-3-reto@labrat.space>
In-Reply-To
<20200914051045.1726954-1-reto@labrat.space> (view parent)
DKIM signature
pass
Download raw message
Patch: +1 -2
From: Galen Abell <galen@galenabell.com>

---
 worker/maildir/worker.go | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index cf08e70..e8c5ca5 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -162,8 +162,7 @@ func (w *Worker) getDirectoryInfo(name string) *models.DirectoryInfo {
			dirInfo.Recent++
		}
	}
	dirInfo.Unseen += dirInfo.Recent
	dirInfo.Exists = len(uids) + dirInfo.Recent
	dirInfo.Exists = len(uids)
	return dirInfo
}

-- 
2.28.0

[PATCH 3/3] maildir: fix updating of recent counts

Details
Message ID
<20200914051045.1726954-4-reto@labrat.space>
In-Reply-To
<20200914051045.1726954-1-reto@labrat.space> (view parent)
DKIM signature
pass
Download raw message
Patch: +24 -10
---
This is what I meant yesterday, essentially what the comment says.
We can't really do that UI side, as we'd need a new event type for that which
doesn't make too much sense in my view if this is just a nuisance of the maildir
worker.

It's a bit on the spammy side for the event loop, but I can't think about a more
efficient version that's somewhat easy to maintain.

The only thing being clearing the recent flag UI side, but that's somewhat wrong
considering that in IMAP you can toggle this flag with other MUAs etc.


 worker/maildir/worker.go | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index e8c5ca5..6a00353 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -367,19 +367,33 @@ func (w *Worker) handleRemoveDirectory(msg *types.RemoveDirectory) error {

func (w *Worker) handleFetchMessageHeaders(
	msg *types.FetchMessageHeaders) error {
	for _, uid := range msg.Uids {
		info, err := w.msgInfoFromUid(uid)
		if err != nil {
			w.worker.Logger.Printf("could not get message info: %v", err)
			w.err(msg, err)
			continue
	emitMsgInfosForUIDS := func() {
		for _, uid := range msg.Uids {
			info, err := w.msgInfoFromUid(uid)
			if err != nil {
				w.worker.Logger.Printf("could not get message info: %v", err)
				w.err(msg, err)
				continue
			}
			w.worker.PostMessage(&types.MessageInfo{
				Message: types.RespondTo(msg),
				Info:    info,
			}, nil)
			w.c.ClearRecentFlag(uid)
		}
		w.worker.PostMessage(&types.MessageInfo{
			Message: types.RespondTo(msg),
			Info:    info,
		dirInfo := w.getDirectoryInfo(w.selectedName)
		// update the counts
		w.worker.PostMessage(&types.DirectoryInfo{
			Info: dirInfo,
		}, nil)
		w.c.ClearRecentFlag(uid)
	}
	// this gets done twice due to the recent flag.
	// When a header is first fetched, it has the recent flag associated to it
	// as soon as it's fetched that should trigger the recent commands.
	// If that happens, the message stops being recent, so we need to send
	// the updated flags to the msglist.
	emitMsgInfosForUIDS()
	emitMsgInfosForUIDS()
	return nil
}

-- 
2.28.0
Details
Message ID
<20200914052708.l4ztx7nlxsvtn6qh@feather.localdomain>
In-Reply-To
<20200914051045.1726954-1-reto@labrat.space> (view parent)
DKIM signature
pass
Download raw message
Hm, on a second thought...
When do we actually want to clear the recent flag?
I mean we could do it when switch folders / read the message instead of automatically.

What is the proper use here?

Not sure how many people actually understand the recent flag and how it differs
from the seen flag...
So not auto clearing it might confuse the one or other user.

Opinions?
Details
Message ID
<C5NEED8XJVQV.2FT3HASRJBILM@freya>
In-Reply-To
<20200914052708.l4ztx7nlxsvtn6qh@feather.localdomain> (view parent)
DKIM signature
pass
Download raw message
Hey Reto,

Please feel free to squash my patch into the others, it's not very large
;)

The new patches don't seem to work quite right though; it looks like
when I get a new message it very briefly flashes the recent count and
then immediately clears it. I'm also not sure if we want the recent
trigger to fire when aerc starts up and handles new messages, since it
seems like it should only happen when aerc is running in the background.
That might require some more refactoring though so not a priority.

I do think the recent flag is a bit confusing in its current state, and
I wonder if it even makes sense to show it, especially considering that
we don't refresh folders in the background (so you couldn't have a
recent flag pop up on INBOX if you are in Archive, for instance). If we
do want to keep it I think it would be better to clear it more
explicitly, ie when a message is read or a folder is switched.
Details
Message ID
<20200915043955.tenvpe5j5dzitowx@feather.localdomain>
In-Reply-To
<C5NEED8XJVQV.2FT3HASRJBILM@freya> (view parent)
DKIM signature
pass
Download raw message
On Mon, Sep 14, 2020 at 11:08:58PM +0200, Galen Abell wrote:
> The new patches don't seem to work quite right though; it looks like
> when I get a new message it very briefly flashes the recent count and
> then immediately clears it.

Yup, working as designed actually. Not that I say that this is how we should be
doing it...

>I'm also not sure if we want the recent
> trigger to fire when aerc starts up and handles new messages, since it
> seems like it should only happen when aerc is running in the background.
> That might require some more refactoring though so not a priority.

That's nice in principle, however what's "in the background" is a very hard question
for any program.... What does that mean now days with a multiple monitor setup?

> I do think the recent flag is a bit confusing in its current state, and
> I wonder if it even makes sense to show it, especially considering that
> we don't refresh folders in the background (so you couldn't have a
> recent flag pop up on INBOX if you are in Archive, for instance). If we
> do want to keep it I think it would be better to clear it more
> explicitly, ie when a message is read or a folder is switched.

Well, maildir / imap don't ;)
notmuch refreshes in the background just fine actually.

But yeah... let's not auto clear the flag but do it on folder switch / reading
the message.

So, to simplify, shall we clear the recent flag on fetchbodypart so that we track
the read mails as well as on opendir or whatever the event type is called?

Cheers,
Reto
Details
Message ID
<C5NRB7DLTFBE.30PSUAZFT7KB1@njordr>
In-Reply-To
<20200915043955.tenvpe5j5dzitowx@feather.localdomain> (view parent)
DKIM signature
pass
Download raw message
> That's nice in principle, however what's "in the background" is a very
> hard question
> for any program.... What does that mean now days with a multiple monitor
> setup?

Yeah, I was actually thinking more "while aerc is running" and not "in
the background", because currently we fire a bunch of recent triggers
right after startup if there are any new messages that get moved to cur.
This makes sense if we consider them as being recent to aerc, but they
might be "old" messages timestamp-wise. I think it'd be less annoying to
only trigger notifications after the initial maildir sync.

> So, to simplify, shall we clear the recent flag on fetchbodypart so that
> we track
> the read mails as well as on opendir or whatever the event type is
> called?

That sounds better to me, although we will also end up with all new
messages showing as recent in the sidebar on the initial load (for the
same reason as above).
Review patch Export thread (mbox)