Ben Burwell: 2 Teach the reply command about mailing lists Add docs for reply -T 7 files changed, 162 insertions(+), 69 deletions(-)
> Would it make sense to fallback to List-ID and try to parse it as a > mail address? Probably not RFC approved, but may be useful for some > random list that doesn't set the List-Post header? I would rather not add features that deviate from RFCs until we need them for a sort of "quirks mode." > I don't think that we need to (nor should) set all possible > combinations as keyboard shortcuts. [...] People should set up those
I am in agreement with this. I think that replying to the list-id is fine, generally, but must be opt-in so we don't run into a Reply-To header sort of issue.I don't understand what you mean? Ben just mentioned that we probably should not use the List-Id. The whole topic is an explicit flag the users set to make aerc ignore the reply-to header and to just write to the list, denoted in the List-Post header. It's in no way done automatically just because the header is present. What has thej3s
> more "special" ones themselves. Fair enough -- I was sort of on the fence about including the shortcuts; I've removed them from v2. > I thought it contained helpers to cope with actual mailing lists, not > helpers to parse mail headers. Well, it is to cope with mailing lists, specifically, their special headers ;) Enjoy your vacation!
Hi all! Mail list developer in-training here. On Mon Jan 6, 2020 at 10:28 AM, Ben Burwell wrote:
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~sircmpwn/aerc/patches/9462/mbox | git am -3Learn more about email & git
Mailing lists can add a "List-Post" header containing a mailto URL specifying where mail should be sent to post to the list. Add a -l option to the reply command which will send the reply to the mailing list found in the List-Post header, if present.
\o/ Yey, cool
This header is specified in RFC 2369, the same RFC which specifies the use of the List-Unsubscribe header with the same format. Thus, extract and re-use the parsing logic from unsubscribe.go. Additionally, perform cleanup of lingering references to when reply and forward were implemented in the same file. --- commands/msg/reply.go | 145 +++++++++++++----- commands/msg/unsubscribe.go | 28 +--- config/binds.conf | 4 + doc/aerc.1.scd | 7 +- lib/mailing_list.go | 34 ++++ .../mailing_list_test.go | 9 +- 6 files changed, 159 insertions(+), 68 deletions(-) create mode 100644 lib/mailing_list.go rename commands/msg/unsubscribe_test.go => lib/mailing_list_test.go (85%) diff --git a/commands/msg/reply.go b/commands/msg/reply.go index a7379d7..e898196 100644 --- a/commands/msg/reply.go +++ b/commands/msg/reply.go @@ -6,14 +6,22 @@ import ( "fmt" "io" gomail "net/mail" + "net/url" "strings" "git.sr.ht/~sircmpwn/getopt" + "git.sr.ht/~sircmpwn/aerc/lib" "git.sr.ht/~sircmpwn/aerc/models" "git.sr.ht/~sircmpwn/aerc/widgets" ) +const ( + replyTypeSender int = iota + replyTypeAll + replyTypeList +) + type reply struct{} func init() { @@ -29,17 +37,18 @@ func (reply) Complete(aerc *widgets.Aerc, args []string) []string { } func (reply) Execute(aerc *widgets.Aerc, args []string) error { - opts, optind, err := getopt.Getopts(args, "aqT:") + opts, optind, err := getopt.Getopts(args, "aqlT:") if err != nil { return err } if optind != len(args) { - return errors.New("Usage: reply [-aq -T <template>]") + return errors.New("Usage: reply [-aql -T <template>]") } var ( - quote bool - replyAll bool - template string + quote bool + replyAll bool + template string + replyList bool ) for _, opt := range opts { switch opt.Option { @@ -49,9 +58,22 @@ func (reply) Execute(aerc *widgets.Aerc, args []string) error { quote = true case 'T': template = opt.Value + case 'l': + replyList = true } } + replyType := replyTypeSender + + if replyAll && replyList { + return errors.New("reply-all and reply-list are mutually exclusive") + } + if replyAll { + replyType = replyTypeAll + } else if replyList { + replyType = replyTypeList + } + widget := aerc.SelectedTab().(widgets.ProvidesMessage) acct := widget.SelectedAccount() @@ -70,37 +92,9 @@ func (reply) Execute(aerc *widgets.Aerc, args []string) error { } acct.Logger().Println("Replying to email " + msg.Envelope.MessageId) - var ( - to []string - cc []string - toList []*models.Address - ) - if args[0] == "reply" { - if len(msg.Envelope.ReplyTo) != 0 { - toList = msg.Envelope.ReplyTo - } else { - toList = msg.Envelope.From - } - for _, addr := range toList { - if addr.Name != "" { - to = append(to, fmt.Sprintf("%s <%s@%s>", - addr.Name, addr.Mailbox, addr.Host)) - } else { - to = append(to, fmt.Sprintf("<%s@%s>", addr.Mailbox, addr.Host)) - } - } - if replyAll { - for _, addr := range msg.Envelope.Cc { - cc = append(cc, addr.Format()) - } - for _, addr := range msg.Envelope.To { - address := fmt.Sprintf("%s@%s", addr.Mailbox, addr.Host) - if address == us.Address { - continue - } - to = append(to, addr.Format()) - } - } + to, cc, err := getReplyAddresses(msg, replyType, us) + if err != nil { + return err } var subject string @@ -130,9 +124,7 @@ func (reply) Execute(aerc *widgets.Aerc, args []string) error { return err } - if args[0] == "reply" { - composer.FocusTerminal() - } + composer.FocusTerminal() tab := aerc.NewTab(composer, subject) composer.OnHeaderChange("Subject", func(subject string) { @@ -183,3 +175,78 @@ func findPlaintext(bs *models.BodyStructure, return nil, nil } + +// getReplyAddresses returns the list of To: and CC: addresses to use in the +// reply we are composing, depending on the reply type (all, sender, list). +func getReplyAddresses(msg *models.MessageInfo, replyType int, + us *gomail.Address) ([]string, []string, error) {
This could probably use some hints in the return types.
Even though I'm not fond of named return arguments for naked returns, they
are nice to document stuff in the function signature.
And can you please make the replyType an actual type so that my editor knows the
valid values? :)
Pseudo patch:
+ type replyType int
const (
- replyTypeSender int = iota
- replyTypeSender replyType = iota
replyTypeAll
replyTypeList
)
Can we make that something like:
func getReplyAddresses(msg *models.MessageInfo, type replyType,
us *gomail.Address) (to []string, cc []string, error) {
+ if replyType == replyTypeList { + listPost, err := getListReply(msg) + return []string{listPost}, nil, err + } + + var ( + to []string + cc []string + ) + toList := getReplyTo(msg.Envelope) + for _, addr := range toList { + to = append(to, addr.Format()) + } + + if replyType == replyTypeAll { + for _, addr := range msg.Envelope.Cc { + cc = append(cc, addr.Format()) + } + for _, addr := range msg.Envelope.To { + address := fmt.Sprintf("%s@%s", addr.Mailbox, addr.Host) + if address == us.Address { + continue + } + to = append(to, addr.Format()) + } + } + return to, cc, nil +} + +// getReplyTo gets the addresses to reply to: the content of the Reply-To +// header if present, or the From header if not. +func getReplyTo(env *models.Envelope) []*models.Address { + if len(env.ReplyTo) > 0 { + return env.ReplyTo + } + return env.From +} + +// getListReply searches the list-post header for a mailto: URL and returns the +// enclosed address, or an error. +func getListReply(msg *models.MessageInfo) (string, error) { + listPost, err := msg.RFC822Headers.Text("list-post") + if err != nil { + return "", fmt.Errorf("get reply addresses: %w", err) + } + if listPost == "" { + return "", errors.New("no list-post header found")
Would it make sense to fallback to List-ID and try to parse it as a mail address? Probably not RFC approved, but may be useful for some random list that doesn't set the List-Post header? Just throwing out an idea, nothing to do with the patch per se.
+ } + urls, err := lib.ParseURLList(listPost) + if err != nil { + return "", fmt.Errorf("could not parse list-post header: %w", err) + } + addr, err := firstMailtoAddress(urls) + if err != nil { + return "", fmt.Errorf("get reply addresses: %w", err) + } + return addr, nil +} + +// firstMailtoAddress grabs the first mailto: URL in the list and returns the +// address, or an error if none is found. +func firstMailtoAddress(urls []*url.URL) (string, error) { + for _, u := range urls { + if u.Scheme != "mailto" { + continue + } + return u.Opaque, nil + } + return "", errors.New("no mailto address found") +} diff --git a/commands/msg/unsubscribe.go b/commands/msg/unsubscribe.go index 5ffec46..d9fb259 100644 --- a/commands/msg/unsubscribe.go +++ b/commands/msg/unsubscribe.go @@ -1,7 +1,6 @@ package msg import ( - "bufio" "errors" "net/url" "strings" @@ -42,7 +41,10 @@ func (Unsubscribe) Execute(aerc *widgets.Aerc, args []string) error { if !headers.Has("list-unsubscribe") { return errors.New("No List-Unsubscribe header found") } - methods := parseUnsubscribeMethods(headers.Get("list-unsubscribe")) + methods, err := lib.ParseURLList(headers.Get("list-unsubscribe")) + if err != nil { + return err + } aerc.Logger().Printf("found %d unsubscribe methods", len(methods)) for _, method := range methods { aerc.Logger().Printf("trying to unsubscribe using %v", method) @@ -58,28 +60,6 @@ func (Unsubscribe) Execute(aerc *widgets.Aerc, args []string) error { return errors.New("no supported unsubscribe methods found") } -// parseUnsubscribeMethods reads the list-unsubscribe header and parses it as a -// list of angle-bracket <> deliminated URLs. See RFC 2369. -func parseUnsubscribeMethods(header string) (methods []*url.URL) { - r := bufio.NewReader(strings.NewReader(header)) - for { - // discard until < - _, err := r.ReadSlice('<') - if err != nil { - return - } - // read until < - m, err := r.ReadSlice('>') - if err != nil { - return - } - m = m[:len(m)-1] - if u, err := url.Parse(string(m)); err == nil { - methods = append(methods, u) - } - } -} - func unsubscribeMailto(aerc *widgets.Aerc, u *url.URL) error { widget := aerc.SelectedTab().(widgets.ProvidesMessage) acct := widget.SelectedAccount() diff --git a/config/binds.conf b/config/binds.conf index 760fae1..29722e1 100644 --- a/config/binds.conf +++ b/config/binds.conf @@ -39,6 +39,8 @@ rr = :reply -a<Enter> rq = :reply -aq<Enter> Rr = :reply<Enter> Rq = :reply -q<Enter> +rll = :reply -l<Enter> +rlq = :reply -lq<Enter> c = :cf<space> $ = :term<space> @@ -62,6 +64,8 @@ rr = :reply -a<Enter> rq = :reply -aq<Enter> Rr = :reply<Enter> Rq = :reply -q<Enter> +rll = :reply -l<Enter> +rlq = :reply -lq<Enter>
I don't think that we need to (nor should) set all possible combinations as keyboard shortcuts. I'd be in favor of having common functionality there, but making a shortcut for every single combination which is valid seems excessive. People should set up those more "special" ones themselves. Reply / reply all is sensible, but I don't think we should set list reply etc. Thoughts?
H = :toggle-headers<Enter> <C-k> = :prev-part<Enter> diff --git a/doc/aerc.1.scd b/doc/aerc.1.scd index 73a4d83..70d1bdb 100644 --- a/doc/aerc.1.scd +++ b/doc/aerc.1.scd @@ -116,13 +116,16 @@ message list, the message in the message viewer, etc). *-p*: Pipe just the selected message part, if applicable -*reply* [-aq] +*reply* [-aql] Opens the composer to reply to the selected message. - *-a*: Reply all + *-a*: Reply all (mutually exclusive with *-l*) *-q*: Insert a quoted version of the selected message into the reply editor + *-l*: Reply just to the address specified in the List-Post header (mutually + exclusive with *-a*) + *read* Marks the marked or selected messages as read. diff --git a/lib/mailing_list.go b/lib/mailing_list.go new file mode 100644 index 0000000..3046fcd --- /dev/null +++ b/lib/mailing_list.go
That name is... confusing. I thought it contained helpers to cope with actual mailing lists, not helpers to parse mail headers. But nope, I don't have a better idea...
@@ -0,0 +1,34 @@ +package lib + +import ( + "bufio" + "errors" + "io" + "net/url" + "strings" +) + +// ParseURLList parses a list of URLs from a header string as specified in RFC +// 2369, such as the "List-Unsubscribe" header. +func ParseURLList(s string) ([]*url.URL, error) { + urls := []*url.URL{} + r := bufio.NewReader(strings.NewReader(s)) + for { + // discard until < + _, err := r.ReadSlice('<') + if err == io.EOF { + return urls, nil + } else if err != nil { + return nil, errors.New("parse URL list: did not find expected <") + } + // read until <
Copy / paste mistake, read until '>'.
But I don't think the comment is needed anyhow. ReadSlice('>') should do the trick.
+ m, err := r.ReadSlice('>') + if err != nil { + return nil, errors.New("parse URL list: did not find expected >") + } + m = m[:len(m)-1] + if u, err := url.Parse(string(m)); err == nil { + urls = append(urls, u) + } + } +} diff --git a/commands/msg/unsubscribe_test.go b/lib/mailing_list_test.go similarity index 85% rename from commands/msg/unsubscribe_test.go rename to lib/mailing_list_test.go index e4e6f25..08c0b58 100644 --- a/commands/msg/unsubscribe_test.go +++ b/lib/mailing_list_test.go @@ -1,10 +1,10 @@ -package msg +package lib import ( "testing" ) -func TestParseUnsubscribe(t *testing.T) { +func TestParseURLList(t *testing.T) { type tc struct { hdr string expected []string @@ -27,7 +27,10 @@ func TestParseUnsubscribe(t *testing.T) { }}, } for _, c := range cases { - result := parseUnsubscribeMethods(c.hdr) + result, err := ParseURLList(c.hdr) + if err != nil { + t.Errorf("error parsing URL list: %v", err) + } if len(result) != len(c.expected) { t.Errorf("expected %d methods but got %d", len(c.expected), len(result)) continue -- 2.24.1
Hi Ben, I'll be off on vacation this week, hence only a short glance over. Here are a few comments: On Sun, Jan 05, 2020 at 11:04:45PM -0500, Ben Burwell wrote:
--- doc/aerc.1.scd | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/aerc.1.scd b/doc/aerc.1.scd index 70d1bdb..6dbb7b5 100644 --- a/doc/aerc.1.scd +++ b/doc/aerc.1.scd @@ -116,7 +116,7 @@ message list, the message in the message viewer, etc). *-p*: Pipe just the selected message part, if applicable -*reply* [-aql] +*reply* [-aql] [-T <template-file>] Opens the composer to reply to the selected message. *-a*: Reply all (mutually exclusive with *-l*) @@ -126,6 +126,8 @@ message list, the message in the message viewer, etc). *-l*: Reply just to the address specified in the List-Post header (mutually exclusive with *-a*) + *-T*: Use the specified template file for creating the initial message body + *read* Marks the marked or selected messages as read. -- 2.24.1