mail/reply: rewrite recipient selection

Thoroughly ensure that To+Cc contains neither our own address (except
when it is wanted) nor any duplicates.

Use structured headers provided by email.headerregistry to simplify
dealing with addr-spec vs. mailbox (display name + <addr-spec>).

4 files changed, 159 insertions, 93 deletions

diff --git a/alot/commands/thread.py b/alot/commands/thread.py
index 5d8006a1..a2a33ef1 100644
--- a/alot/commands/thread.py
+++ b/alot/commands/thread.py
@@ -96,9 +96,10 @@ class ReplyCommand(Command):
                reply.ReplyMode.AUTHOR
 
         to, cc = reply.determine_recipients(message, account, mode)
-        headers[HDR.TO] = to
+        # TODO keep those as lists?
+        headers[HDR.TO] = str(to)
         if cc:
-            headers[HDR.CC] = cc
+            headers[HDR.CC] = str(cc)
 
         # set In-Reply-To + References headers
         headers[HDR.IN_REPLY_TO] = '<%s>' % message.id
@@ -107,9 +108,9 @@ class ReplyCommand(Command):
         # if any of the recipients is a mailinglist that we are subscribed to,
         # set Mail-Followup-To header so that duplicates are avoided
         # to and cc are already cleared of our own address
-        mft = reply.mail_followup_to([to, cc])
+        mft = reply.mail_followup_to(to + cc)
         if mft:
-            headers[HDR.MAIL_FOLLOWUP_TO] = mft
+            headers[HDR.MAIL_FOLLOWUP_TO] = str(mft)
 
         body_text = reply.body_text(message, ui)
 
diff --git a/alot/db/message.py b/alot/db/message.py
index 97ab991f..01d7c7f1 100644
--- a/alot/db/message.py
+++ b/alot/db/message.py
@@ -119,6 +119,20 @@ class _MessageHeaders:
                 return []
             raise
 
+    def pick_addrlist_header(self, *args):
+        """
+        Given a list of "address-list" header names as positional arguments,
+        return the addresses from the first one that is present.
+
+        :returns: addresses from the first existing header, or an empty tuple
+        :rtype: tuple(headerregistry.Address)
+        """
+        for h in args:
+            if h in self:
+                return self.get(h).addresses
+
+        return ()
+
 class _MimeTree:
     _part = None
 
diff --git a/alot/mail/policy.py b/alot/mail/policy.py
index 1b9f07c3..6125f8f7 100644
--- a/alot/mail/policy.py
+++ b/alot/mail/policy.py
@@ -6,13 +6,17 @@ import email.headerregistry as _hdr
 
 from . import headers as HDR
 
+# here we define our custom policy that handles additional headers
+
 _headers = {
-    HDR.X_BEEN_THERE   : _hdr.UniqueAddressHeader,
-    HDR.X_MAILING_LIST : _hdr.UniqueAddressHeader,
+    HDR.X_BEEN_THERE        : _hdr.UniqueAddressHeader,
+    HDR.X_MAILING_LIST      : _hdr.UniqueAddressHeader,
+    HDR.MAIL_FOLLOWUP_TO    : _hdr.UniqueAddressHeader,
+    HDR.MAIL_REPLY_TO       : _hdr.UniqueAddressHeader,
 }
 
-# here we define our custom policy that handles additional headers
-# derive from SMTP
+# derive from SMTP, but create a new instance to get our own
+# instance of header_factory
 p = _p.EmailPolicy() + _p.SMTP
 
 for h, t in _headers.items():
diff --git a/alot/mail/reply.py b/alot/mail/reply.py
index 4a69ecee..43381602 100644
--- a/alot/mail/reply.py
+++ b/alot/mail/reply.py
@@ -2,7 +2,7 @@
 # For further details see the COPYING file
 
 import email.policy
-from   email.utils import getaddresses, parseaddr
+from   email.utils import getaddresses
 from   enum        import Enum, auto
 import logging
 
@@ -75,92 +75,140 @@ class ReplyMode(Enum):
     GROUP  = auto()
     LIST   = auto()
 
-def _ensure_unique_address(recipients):
+class AddrList:
     """
-    clean up a list of name,address pairs so that
-    no address appears multiple times.
+    An ordered set of unique  addresses.
     """
-    res = dict()
-    for name, address in getaddresses(recipients):
-        res[address] = name
-    logging.debug(res)
-    urecipients = [formataddr((n, a)) for a, n in res.items()]
-    return sorted(urecipients)
-
-def _clear_my_address(my_account, value):
-    """return recipient header without the addresses in my_account
-
-    :param my_account: my account
-    :type my_account: :class:`Account`
-    :param value: a list of recipient or sender strings (with or without
-        real names as taken from email headers)
-    :type value: list(str)
-    :returns: a new, potentially shortend list
-    :rtype: list(str)
-    """
-    new_value = []
-    for name, address in getaddresses(value):
-        if not my_account.matches_address(address):
-            new_value.append(formataddr((name, address)))
-    return new_value
 
-def determine_recipients(message, account, mode):
-    # set To
-    sender = (message.headers.get(HDR.REPLY_TO) or
-              message.headers.get(HDR.FROM) or '')
-    sender_address = parseaddr(sender)[1]
-    cc = []
-
-    # check if reply is to self sent message
-    if account.matches_address(sender_address):
-        recipients = message.headers.get_all(HDR.TO)
-        emsg = 'Replying to own message, set recipients to: %s' \
-            % recipients
-        logging.debug(emsg)
+    # list of Address objects
+    _addrs      = None
+    # mapping of addr-spec -> Address
+    _addr_specs = None
+
+    def __init__(self, addresses):
+        self._addrs      = []
+        self._addr_specs = {}
+
+        for addr in addresses:
+            self.add(addr)
+
+    def __str__(self):
+        return ', '.join((str(a) for a in self._addrs))
+
+    def __len__(self):
+        return len(self._addrs)
+
+    def __iter__(self):
+        return iter(self._addrs)
+
+    def _addr_to_addr_spec(self, addr):
+        if hasattr(addr, 'addr_spec'):
+            # headerregistry.Address
+            return addr.addr_spec
+
+        # account.Address
+        return str(addr)
+
+    def __contains__(self, addr):
+        return self._addr_to_addr_spec(addr) in self._addr_specs
+
+    def __delitem__(self, addr):
+        addr_spec = self._addr_to_addr_spec(addr)
+        idx       = self._addrs.index(addr_spec)
+        del self._addrs[idx]
+        del self._addr_specs[addr_spec]
+
+    def __add__(self, other):
+        ret = self.__class__(self)
+        for a in other:
+            ret.add(a)
+        return ret
+
+    def add(self, addr):
+        if not addr.addr_spec:
+            raise ValueError('Missing addr-spec')
+
+        if addr in self:
+            idx = self._addrs.index(self._addr_specs[addr.addr_spec])
+
+            # if the address we already have is missing display name,
+            # replace it with the new one
+            if not self._addrs[idx].display_name and addr.display_name:
+                self._addrs[idx]                 = addr
+                self._addr_specs[addr.addr_spec] = addr
+
+            return
+
+        self._addrs.append(addr)
+        self._addr_specs[addr.addr_spec] = addr
+
+def _reply_list(message):
+    # To choose the target of the reply --list
+    # Reply-To is standart reply target RFC 2822:, RFC 1036: 2.2.1
+    # X-BeenThere is needed by sourceforge ML also winehq
+    # X-Mailing-List is also standart and is used by git-send-mail
+    # Some mail server (gmail) will not resend you own mail, so we
+    # fall back to 'To' as last resort
+    to = message.headers.pick_addrlist_header(
+            HDR.X_BEEN_THERE, HDR.X_MAILING_LIST, HDR.REPLY_TO, HDR.TO)
+
+    # copy original Cc
+    cc = message.headers.pick_addrlist_header(HDR.CC)
+
+    return to, cc
+
+def _reply_group(message, senders):
+    # make sure that our own address is not included
+    # if the message was self-sent, then our address is not included
+    MFT = message.headers.pick_addrlist_header(HDR.MAIL_FOLLOWUP_TO)
+    if MFT and settings.get('honor_followup_to'):
+        logging.debug('honor followup to: %s', MFT)
+        to = MFT
+        cc = ()
     else:
-        recipients = [sender]
-
-    if mode == ReplyMode.GROUP:
-        # make sure that our own address is not included
-        # if the message was self-sent, then our address is not included
-        MFT = message.headers.get_all(HDR.MAIL_FOLLOWUP_TO)
-        followupto = _clear_my_address(account, MFT)
-        if followupto and settings.get('honor_followup_to'):
-            logging.debug('honor followup to: %s', ', '.join(followupto))
-            recipients = followupto
-            # since Mail-Followup-To was set, ignore the Cc header
-        else:
-            if sender != message.headers.get(HDR.FROM):
-                recipients.append(message.headers.get(HDR.FROM))
-
-            # append To addresses if not replying to self sent message
-            if not account.matches_address(sender_address):
-                cleared = _clear_my_address(account,
-                                           message.headers.get_all(HDR.TO))
-                recipients.extend(cleared)
-
-            # copy cc for group-replies
-            if HDR.CC in message.headers:
-                cc = _clear_my_address(account, message.headers.get_all(HDR.CC))
-
-    to = ', '.join(_ensure_unique_address(recipients))
-    cc = ', '.join(cc)
-    logging.debug('reply to: %s', to)
+        to = message.headers.pick_addrlist_header(HDR.TO) + senders
+        cc = message.headers.pick_addrlist_header(HDR.CC)
+
+    return to, cc
+
+def determine_recipients(message, account, mode):
+    # TODO: detect reply-to munging by ML here?
+    senders = message.headers.pick_addrlist_header(HDR.MAIL_REPLY_TO,
+                HDR.REPLY_TO, HDR.FROM)
+
+    self_reply = len(senders) == 1 and account.matches_address(senders[0].addr_spec)
+    if self_reply:
+        senders = message.headers.get(HDR.TO)
+        senders = senders.addresses if senders else ()
+        logging.debug('Replying to own message, invert senders to: %s',
+                      senders)
 
     if mode == ReplyMode.LIST:
-        # To choose the target of the reply --list
-        # Reply-To is standart reply target RFC 2822:, RFC 1036: 2.2.1
-        # X-BeenThere is needed by sourceforge ML also winehq
-        # X-Mailing-List is also standart and is used by git-send-mail
-        to = (message.headers.get(HDR.REPLY_TO)     or
-              message.headers.get(HDR.X_BEEN_THERE) or
-              message.headers.get(HDR.X_MAILING_LIST))
-
-        # Some mail server (gmail) will not resend you own mail, so you
-        # have to deal with the one in sent
-        if to is None:
-            to = message.headers[HDR.TO]
-        logging.debug('mail list reply to: %s', to)
+        to, cc = _reply_list(message)
+    elif mode == ReplyMode.GROUP:
+        to, cc = _reply_group(message, senders)
+    else: # mode == ReplyMode.AUTHOR
+        to = senders
+        cc = ()
+
+    to = AddrList(to)
+    cc = AddrList(cc)
+
+    # remove duplicates across to/cc
+    for a in to:
+        if a in cc:
+            del cc[a]
+
+    # remove sending account from the address lists
+    for a in [account.address] + account.aliases:
+        # keep own address if it's the only recipient
+        # i.e. the user deliberately wants to self-send the reply
+        if a in to and len(to) > 1:
+            del to[a]
+        if a in cc:
+            del cc[a]
+
+    logging.debug('%s reply to: %s %s', mode, str(to), str(cc))
 
     return to, cc
 
@@ -217,7 +265,6 @@ def mail_followup_to(recipients):
     lists = settings.get('mailinglists')
 
     # check if any recipient address matches a known mailing list
-    if any(addr in lists for n, addr in getaddresses(recipients)):
-        followupto = ', '.join(recipients)
-        logging.debug('mail followup to: %s', followupto)
-        return followupto
+    if any(a.addr_spec in lists for a in recipients):
+        logging.debug('mail followup to: %s', recipients)
+        return recipients