~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
2 2

[PATCH aerc v1] tag: allow to toggle tags in notmuch

Details
Message ID
<20250226112643.542037-2-inwit@sindominio.net>
Sender timestamp
1740572634
DKIM signature
pass
Download raw message
Patch: +44 -21
So far, the :tag/:modify-labels command in the notmuch worker allows
adding a tag (by prefixing it with '+') and removing it (by prefixing it
with '-'). Add a new functionality to this command, allowing it to
toggle a tag by prefixing it with '!'.

Fixes: https://todo.sr.ht/~rjarry/aerc/292
Changelog-added: It is now possible to toggle notmuch tags.
Signed-off-by: inwit <inwit@sindominio.net>

---
I haven't looked into JMAP, sorry, since I don't have access to one such
account. And, as usual, I'm not confident in go, so a careful review
would be needed.

 app/account.go                 |  3 ++-
 commands/msg/modify-labels.go  |  8 +++++---
 doc/aerc.1.scd                 |  9 +++++----
 lib/hooks/tag-modified.go      |  2 ++
 lib/msgstore.go                |  9 +++++----
 worker/notmuch/lib/database.go | 19 +++++++++++++++++--
 worker/notmuch/message.go      |  8 ++++----
 worker/notmuch/worker.go       |  6 +++---
 worker/types/messages.go       |  1 +
 9 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/app/account.go b/app/account.go
index 262d5f38..7e7229cc 100644
--- a/app/account.go
+++ b/app/account.go
@@ -315,12 +315,13 @@ func (acct *AccountView) newStore(name string) *lib.MessageStore {
				msg := fmt.Sprintf("mail-added hook: %s", err)
				PushError(msg)
			}
		}, func(add []string, remove []string) {
		}, func(add []string, remove []string, toggle []string) {
			err := hooks.RunHook(&hooks.TagModified{
				Account: acct.Name(),
				Backend: backend,
				Add:     add,
				Remove:  remove,
				Toggle:  toggle,
			})
			if err != nil {
				msg := fmt.Sprintf("tag-modified hook: %s", err)
diff --git a/commands/msg/modify-labels.go b/commands/msg/modify-labels.go
index f89d84a6..769c2972 100644
--- a/commands/msg/modify-labels.go
+++ b/commands/msg/modify-labels.go
@@ -9,7 +9,7 @@ import (
)

type ModifyLabels struct {
	Labels []string `opt:"..." metavar:"[+-]<label>" complete:"CompleteLabels" desc:"Message label."`
	Labels []string `opt:"..." metavar:"[+-!]<label>" complete:"CompleteLabels" desc:"Message label."`
}

func init() {
@@ -43,19 +43,21 @@ func (m ModifyLabels) Execute(args []string) error {
		return err
	}

	var add, remove []string
	var add, remove, toggle []string
	for _, l := range m.Labels {
		switch l[0] {
		case '+':
			add = append(add, l[1:])
		case '-':
			remove = append(remove, l[1:])
		case '!':
			toggle = append(toggle, l[1:])
		default:
			// if no operand is given assume add
			add = append(add, l)
		}
	}
	store.ModifyLabels(uids, add, remove, func(
	store.ModifyLabels(uids, add, remove, toggle, func(
		msg types.WorkerMessage,
	) {
		switch msg := msg.(type) {
diff --git a/doc/aerc.1.scd b/doc/aerc.1.scd
index b01dee75..3895223b 100644
--- a/doc/aerc.1.scd
+++ b/doc/aerc.1.scd
@@ -520,11 +520,12 @@ message list, the message in the message viewer, etc).
*:unflag* [*-t*] _<flag>_
	Operates exactly like *:flag*, defaulting to unsetting (disabling) flags.

*:modify-labels* [_+_|_-_]_<label>_...++
*:tag* [_+_|_-_]_<label>_...
*:modify-labels* [_+_|_-_|_!_]_<label>_...++
*:tag* [_+_|_-_|_!_]_<label>_...
	Modify message labels (e.g. notmuch tags). Labels prefixed with a *+* are
	added, those prefixed with a *-* removed. As a convenience, labels without
	either operand add the specified label.
	added, those prefixed with a *-* are removed and those prefixed with a *!*
	are toggled. As a convenience, labels without either operand add the
	specified label.

	Example: add _inbox_ and _unread_ labels, remove _spam_ label.

diff --git a/lib/hooks/tag-modified.go b/lib/hooks/tag-modified.go
index 21852803..e1b76639 100644
--- a/lib/hooks/tag-modified.go
+++ b/lib/hooks/tag-modified.go
@@ -11,6 +11,7 @@ type TagModified struct {
	Backend string
	Add     []string
	Remove  []string
	Toggle  []string
}

func (m *TagModified) Cmd() string {
@@ -22,6 +23,7 @@ func (m *TagModified) Env() []string {
		fmt.Sprintf("AERC_ACCOUNT=%s", m.Account),
		fmt.Sprintf("AERC_TAG_ADDED=%v", m.Add),
		fmt.Sprintf("AERC_TAG_REMOVED=%v", m.Remove),
		fmt.Sprintf("AERC_TAG_TOGGLED=%v", m.Toggle),
	}

	return env
diff --git a/lib/msgstore.go b/lib/msgstore.go
index 13015acd..2b1ee2c3 100644
--- a/lib/msgstore.go
+++ b/lib/msgstore.go
@@ -74,7 +74,7 @@ type MessageStore struct {
	triggerDirectoryChange func()
	triggerMailDeleted     func()
	triggerMailAdded       func(string)
	triggerTagModified     func([]string, []string)
	triggerTagModified     func([]string, []string, []string)
	triggerFlagChanged     func(string)

	threadBuilderDebounce *time.Timer
@@ -93,7 +93,7 @@ func NewMessageStore(worker *types.Worker, name string,
	ui func() *config.UIConfig,
	triggerNewEmail func(*models.MessageInfo),
	triggerDirectoryChange func(), triggerMailDeleted func(),
	triggerMailAdded func(string), triggerTagModified func([]string, []string),
	triggerMailAdded func(string), triggerTagModified func([]string, []string, []string),
	triggerFlagChanged func(string),
	onSelect func(*models.MessageInfo),
) *MessageStore {
@@ -939,16 +939,17 @@ func (store *MessageStore) PrevResult() {
	store.nextPrevResult(-1)
}

func (store *MessageStore) ModifyLabels(uids []models.UID, add, remove []string,
func (store *MessageStore) ModifyLabels(uids []models.UID, add, remove, toggle []string,
	cb func(msg types.WorkerMessage),
) {
	store.worker.PostAction(&types.ModifyLabels{
		Uids:   uids,
		Add:    add,
		Remove: remove,
		Toggle: toggle,
	}, func(msg types.WorkerMessage) {
		if _, ok := msg.(*types.Done); ok {
			store.triggerTagModified(add, remove)
			store.triggerTagModified(add, remove, toggle)
		}
		cb(msg)
	})
diff --git a/worker/notmuch/lib/database.go b/worker/notmuch/lib/database.go
index b4643a56..a4f885ed 100644
--- a/worker/notmuch/lib/database.go
+++ b/worker/notmuch/lib/database.go
@@ -7,6 +7,7 @@ import (
	"context"
	"errors"
	"fmt"
	"slices"

	"git.sr.ht/~rjarry/aerc/lib/log"
	"git.sr.ht/~rjarry/aerc/lib/notmuch"
@@ -273,7 +274,7 @@ func (db *DB) IndexFile(filename string) (string, error) {
	return msg.ID(), nil
}

func (db *DB) MsgModifyTags(key string, add, remove []string) error {
func (db *DB) MsgModifyTags(key string, add, remove, toggle []string) error {
	err := db.db.Reopen(notmuch.MODE_READ_WRITE)
	if err != nil {
		return err
@@ -296,6 +297,7 @@ func (db *DB) MsgModifyTags(key string, add, remove []string) error {
	if err != nil {
		return err
	}
	tags := msg.Tags()
	defer msg.Close()
	for _, tag := range add {
		err := msg.AddTag(tag)
@@ -306,7 +308,20 @@ func (db *DB) MsgModifyTags(key string, add, remove []string) error {
	for _, tag := range remove {
		err := msg.RemoveTag(tag)
		if err != nil {
			log.Warnf("failed to add tag: %v", err)
			log.Warnf("failed to remove tag: %v", err)
		}
	}
	for _, tag := range toggle {
		if -1 == slices.IndexFunc(tags, func(s string) bool { return s == tag }) {
			err := msg.AddTag(tag)
			if err != nil {
				log.Warnf("failed to toggle tag: %v", err)
			}
		} else {
			err := msg.RemoveTag(tag)
			if err != nil {
				log.Warnf("failed to toggle tag: %v", err)
			}
		}
	}
	return msg.SyncTagsToMaildirFlags()
diff --git a/worker/notmuch/message.go b/worker/notmuch/message.go
index 81a4da54..f8844afe 100644
--- a/worker/notmuch/message.go
+++ b/worker/notmuch/message.go
@@ -164,18 +164,18 @@ func (m *Message) Filename() (string, error) {
// Consider using *Message.ModifyTags for multiple additions / removals
// instead of looping over a tag array
func (m *Message) AddTag(tag string) error {
	return m.ModifyTags([]string{tag}, nil)
	return m.ModifyTags([]string{tag}, nil, nil)
}

// RemoveTag removes a single tag.
// Consider using *Message.ModifyTags for multiple additions / removals
// instead of looping over a tag array
func (m *Message) RemoveTag(tag string) error {
	return m.ModifyTags(nil, []string{tag})
	return m.ModifyTags(nil, []string{tag}, nil)
}

func (m *Message) ModifyTags(add, remove []string) error {
	return m.db.MsgModifyTags(m.key, add, remove)
func (m *Message) ModifyTags(add, remove, toggle []string) error {
	return m.db.MsgModifyTags(m.key, add, remove, toggle)
}

func (m *Message) Remove(curDir maildir.Dir, mfs types.MultiFileStrategy) error {
diff --git a/worker/notmuch/worker.go b/worker/notmuch/worker.go
index 8c954a61..e9620736 100644
--- a/worker/notmuch/worker.go
+++ b/worker/notmuch/worker.go
@@ -609,7 +609,7 @@ func (w *worker) handleModifyLabels(msg *types.ModifyLabels) error {
		if err != nil {
			return fmt.Errorf("could not get message from uid %s: %w", uid, err)
		}
		err = m.ModifyTags(msg.Add, msg.Remove)
		err = m.ModifyTags(msg.Add, msg.Remove, msg.Toggle)
		if err != nil {
			return fmt.Errorf("could not modify message tags: %w", err)
		}
@@ -1021,7 +1021,7 @@ func (w *worker) processNewMaildirFiles(dir string) error {
			continue
		}
		// Force message to move from new/ to cur/
		err = w.db.MsgModifyTags(key, nil, nil)
		err = w.db.MsgModifyTags(key, nil, nil, nil)
		if err != nil {
			w.w.Errorf("MsgModifyTags failed: %v", err)
		}
@@ -1045,5 +1045,5 @@ func (w *worker) addFlags(id string, flags models.Flags) error {
		}
	}

	return w.db.MsgModifyTags(id, addTags, removeTags)
	return w.db.MsgModifyTags(id, addTags, removeTags, nil)
}
diff --git a/worker/types/messages.go b/worker/types/messages.go
index 0174b544..9c540f6e 100644
--- a/worker/types/messages.go
+++ b/worker/types/messages.go
@@ -290,6 +290,7 @@ type ModifyLabels struct {
	Uids   []models.UID
	Add    []string
	Remove []string
	Toggle []string
}

type LabelList struct {
-- 
2.47.2
Details
Message ID
<D82ERTGO96A1.3DNE244Y20YPT@ringo>
In-Reply-To
<20250226112643.542037-2-inwit@sindominio.net> (view parent)
Sender timestamp
1740579812
DKIM signature
pass
Download raw message
inwit, Feb 26, 2025 at 12:32:
> So far, the :tag/:modify-labels command in the notmuch worker allows
> adding a tag (by prefixing it with '+') and removing it (by prefixing it
> with '-'). Add a new functionality to this command, allowing it to
> toggle a tag by prefixing it with '!'.
>
> Fixes: https://todo.sr.ht/~rjarry/aerc/292
> Changelog-added: It is now possible to toggle notmuch tags.
> Signed-off-by: inwit <inwit@sindominio.net>
>
> ---
> I haven't looked into JMAP, sorry, since I don't have access to one such
> account. And, as usual, I'm not confident in go, so a careful review
> would be needed.

Hi inwit,

this is looking good overall. It would be nice if you could address JMAP
in the same patch.

The code should be pretty simple and I can validate it if you don't have
access to a JMAP provider.

Here is what it could look like (not tested):

diff --git worker/jmap/set.go worker/jmap/set.go
index b6bae383fbc8..41cb9574f54f 100644
--- worker/jmap/set.go
+++ worker/jmap/set.go
@@ -167,26 +167,43 @@ func (w *JMAPWorker) rolePatch(role mailbox.Role) string {
 
 func (w *JMAPWorker) handleModifyLabels(msg *types.ModifyLabels) error {
        var req jmap.Request
-       patch := jmap.Patch{}
-
-       for _, a := range msg.Add {
-               mboxId, ok := w.dir2mbox[a]
-               if !ok {
-                       return fmt.Errorf("unknown label: %q", a)
-               }
-               patch[w.mboxPatch(mboxId)] = true
-       }
-       for _, r := range msg.Remove {
-               mboxId, ok := w.dir2mbox[r]
-               if !ok {
-                       return fmt.Errorf("unknown label: %q", r)
-               }
-               patch[w.mboxPatch(mboxId)] = nil
-       }
 
        patches := make(map[jmap.ID]jmap.Patch)
 
        for _, uid := range msg.Uids {
+               email, err := w.cache.GetEmail(jmap.ID(uid))
+               if err != nil {
+                       return fmt.Errorf("email not in cache: %w", err)
+               }
+
+               patch := jmap.Patch{}
+
+               for _, a := range msg.Add {
+                       mboxId, ok := w.dir2mbox[a]
+                       if !ok {
+                               return fmt.Errorf("unknown label: %q", a)
+                       }
+                       patch[w.mboxPatch(mboxId)] = true
+               }
+               for _, r := range msg.Remove {
+                       mboxId, ok := w.dir2mbox[r]
+                       if !ok {
+                               return fmt.Errorf("unknown label: %q", r)
+                       }
+                       patch[w.mboxPatch(mboxId)] = nil
+               }
+               for _, t := range msg.Toggle {
+                       mboxId, ok := w.dir2mbox[t]
+                       if !ok {
+                               return fmt.Errorf("unknown label: %q", t)
+                       }
+                       if email.MailboxIDs[mboxId] {
+                               patch[w.mboxPatch(mboxId)] = nil
+                       } else {
+                               patch[w.mboxPatch(mboxId)] = true
+                       }
+               }
+
                patches[jmap.ID(uid)] = patch
        }
Details
Message ID
<D82FPHGHTYVQ.IAUORUE2DDTV@sindominio.net>
In-Reply-To
<D82ERTGO96A1.3DNE244Y20YPT@ringo> (view parent)
Sender timestamp
1740582451
DKIM signature
pass
Download raw message
On 26/02/2025, 14:23, Robin Jarry wrote:
> this is looking good overall. It would be nice if you could address JMAP
> in the same patch.
Agreed.

> Here is what it could look like (not tested):
It does compile and looks ok to me. I'll add it up to the patch. Thanks!
Reply to thread Export thread (mbox)