Merge lp:~barry/bzr/claws into lp:~bzr/bzr/trunk-old

[Barry Warsaw]

[Barry Warsaw (barry)]

This branch fixes Claws mail support by properly encoding the --compose URL, including body text. It also adds from= support so that the From: field is set correctly.

This has only been tested on Jaunty with Claws 3.7.1.

[John A Meinel (jameinel)]

As long as you've tested it manually, looks good to me.

review: Approve

[Barry Warsaw (barry)]

> As long as you've tested it manually, looks good to me.

Thanks! Aaron suggested in IRC that I pass the from address in the _get_compose_commandline() call instead of grabbing it from the user's config. He also suggested I override ExternalMailClient.compose to "inject the configured from address", and also add tests.

I don't know, nor would be able to test, setting the From header in other clients. I know the code in the branch works for Claws though. What do you think of Aaron's suggestions?

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Barry Warsaw wrote:
>> As long as you've tested it manually, looks good to me.
>
> Thanks! Aaron suggested in IRC that I pass the from address in the _get_compose_commandline() call instead of grabbing it from the user's config. He also suggested I override ExternalMailClient.compose to "inject the configured from address", and also add tests.
>
> I don't know, nor would be able to test, setting the From header in other clients.

Let's assume for the moment that no other client supports it. It's
certainly true that neither Thunderbird nor xdg-email supports it, and
initial tests with Evolution aren't promising.

I'm just sayin' _get_compose_commandline should stay true to its roots,
and just turn its input into a commandline. This also makes testing the
"from" formatting simpler.

> I know the code in the branch works for Claws though. What do you think of Aaron's suggestions?

Anyhow, "bzr selftest Claws" fails, so this branch Needs Fixing.

 review needs-fixing

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkom5p0ACgkQ0F+nu1YWqI03hACeJiE2na0t/3JEsfOInHwfRLJi
VI8An3VgTCyyTOOn0uhOEcSxiE7L0QIH
=mb8c
-----END PGP SIGNATURE-----

review: Needs Fixing

[Barry Warsaw (barry)]

> I'm just sayin' _get_compose_commandline should stay true to its roots,
> and just turn its input into a commandline. This also makes testing the
> "from" formatting simpler.
>
> > I know the code in the branch works for Claws though. What do you think of
> Aaron's suggestions?
>
> Anyhow, "bzr selftest Claws" fails, so this branch Needs Fixing.

I think I've addressed both of these issues. See the latest incremental diff
here:

http://bazaar.launchpad.net/~barry/bzr/claws/revision/4403

This plumbs the from_ argument through more of the interfaces, adds
--mail-from switches to the command line, and adds docstring markup for those
that were missing. As far as I can tell, selftest passes (not just for
Claws).

It also adds some tests for the from_ argument.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Barry Warsaw wrote:
>> I'm just sayin' _get_compose_commandline should stay true to its roots,
>> and just turn its input into a commandline. This also makes testing the
>> "from" formatting simpler.
>>
>>> I know the code in the branch works for Claws though. What do you think of
>> Aaron's suggestions?
>>
>> Anyhow, "bzr selftest Claws" fails, so this branch Needs Fixing.
>
> I think I've addressed both of these issues. See the latest incremental diff
> here:
>
> http://bazaar.launchpad.net/~barry/bzr/claws/revision/4403
>
> This plumbs the from_ argument through more of the interfaces, adds
> --mail-from switches to the command line, and adds docstring markup for those
> that were missing. As far as I can tell, selftest passes (not just for
> Claws).
>
> It also adds some tests for the from_ argument.

This goes a lot farther than I was suggesting. I was trying to describe
a better way of factoring your existing patch, without changing the
big-picture behaviour.

It's good in a way, but it also introduces new problems. If we accept a
"--mail-from" parameter, then we need to notify the user when we can't
honour it, probably by raising an exception.

This means that we need to distinguish between user-supplied --mail-from
and default handling. We could either do default handling in Claws, or
we could provide a flag on MailClient classes indicating whether they
accept a from address, and make the default handling in send conditional
on that flag.

It also seems a bit odd to provide a from_ parameter to Editor that
doesn't do anything, since the Editor client *can* support configuring a
- From address.

If you don't want to do all this, I can understand. Broadening the
scope of the patch also broadens the scope of review. If you'd rather
just get the behaviour for Claws, I think we can get something mergeable
with only small changes to your original patch. I'm happy to discuss it
on IRC.

 review needs-fixing

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkon2H8ACgkQ0F+nu1YWqI25nACfdMGzk4ToKo26ZKftUANPhXh9
NNUAnA6Y5h/MLb06v/KtKKPR9q6JokBA
=mOnN
-----END PGP SIGNATURE-----

review: Needs Fixing

[Barry Warsaw (barry)]

Aaron,

Thanks for setting me on the path to a much smaller and better fix. Here's an
incremental against the original patch, which just adds From: support for
Claws along the lines we discussed. This also adds two new tests for Claws,
and 'bzr selftest Claws' appears to pass for me.

=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py 2009-06-03 19:40:52 +0000
+++ bzrlib/mail_client.py 2009-06-04 19:20:42 +0000
@@ -155,7 +155,7 @@
                       extension, **kwargs)

     def _compose(self, prompt, to, subject, attach_path, mime_subtype,
- extension, body=None):
+ extension, body=None, from_=None):
         """Invoke a mail client as a commandline process.

         Overridden by MAPIClient.
@@ -166,6 +166,8 @@
             "text", but the precise subtype can be specified here
         :param extension: A file extension (including period) associated with
             the attachment type.
+ :param body: Optional body text.
+ :param from_: Optional From: header.
         """
         for name in self._get_client_commands():
             cmdline = [self._encode_path(name, 'executable')]
@@ -173,6 +175,8 @@
                 kwargs = {'body': body}
             else:
                 kwargs = {}
+ if from_ is not None:
+ kwargs['from_'] = from_
             cmdline.extend(self._get_compose_commandline(to, subject,
                                                          attach_path,
                                                          **kwargs))
@@ -335,12 +339,12 @@

     _client_commands = ['claws-mail']

- def _get_compose_commandline(self, to, subject, attach_path, body=None):
+ def _get_compose_commandline(self, to, subject, attach_path, body=None,
+ from_=None):
         """See ExternalMailClient._get_compose_commandline"""
         compose_url = []
- email = self.config.get_user_option('email')
- if email is not None:
- compose_url.append('from=' + urllib.quote(email))
+ if from_ is not None:
+ compose_url.append('from=' + urllib.quote(from_))
         if subject is not None:
             # Don't use urllib.quote_plus because Claws doesn't seem
             # to recognise spaces encoded as "+".
@@ -360,6 +364,16 @@
             message_options.extend(
                 ['--attach', self._encode_path(attach_path, 'attachment')])
         return message_options
+
+ def _compose(self, prompt, to, subject, attach_path, mime_subtype,
+ extension, body=None, from_=None):
+ """See ExternalMailClient._compose"""
+ if from_ is None:
+ from_ = self.config.get_user_option('email')
+ super(Claws, self)._compose(prompt, to, subject, attach_path,
+ mime_subtype, extension, body, from_)
+
+
 mail_client_registry.register('claws', Claws,
                               help=Claws.__doc__)

=== modified file 'bzrlib/tests/test_mail_client.py'
--- bzrlib/tests/test_mail_client.py 2009-05-11 18:35:20 +0000
+++ bzrlib/tests/test_mail_client.py 2009-06-04 19:24:46 +0000
@@ -191,9 +191,10 @@
...

[Barry Warsaw (barry)]

abentley> barry: Your original bzr patch added body support without tests. Do
you mind adding a test for body?

Done, here's the incremental.

=== modified file 'bzrlib/tests/test_mail_client.py'
--- bzrlib/tests/test_mail_client.py 2009-06-04 19:25:38 +0000
+++ bzrlib/tests/test_mail_client.py 2009-06-04 19:54:53 +0000
@@ -233,6 +233,15 @@
                           claws._get_compose_commandline,
                           None, None, 'file%')

+ def test_with_body(self):
+ claws = mail_client.Claws(None)
+ cmdline = claws._get_compose_commandline(
+ <email address hidden>', None, None, 'This is some body text')
+ self.assertEqual(
+ ['--compose',
+ 'mailto:<email address hidden>?body=This%20is%20some%20body%20text'],
+ cmdline)
+

 class TestEditor(tests.TestCase):

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Barry Warsaw wrote:
> abentley> barry: Your original bzr patch added body support without tests. Do
> you mind adding a test for body?
>
> Done, here's the incremental.

Thanks.

 status approved

I'll merge it.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkooKCMACgkQ0F+nu1YWqI0gpACfX/K+gwU7AoLu3EiuzomZpSgd
MLQAniDqyKCJX41hTgOMm+gPWiOvCNRh
=YR+A
-----END PGP SIGNATURE-----

review: Approve

[Barry Warsaw (barry)]

On Jun 4, 2009, at 4:03 PM, Aaron Bentley wrote:

> Barry Warsaw wrote:
>> abentley> barry: Your original bzr patch added body support without
>> tests. Do
>> you mind adding a test for body?
>>
>> Done, here's the incremental.
>
> Thanks.
>
> status approved
>
> I'll merge it.

Thanks!

=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py	2009-06-03 19:05:40 +0000
+++ bzrlib/mail_client.py	2009-06-04 20:35:26 +0000
@@ -155,7 +155,7 @@
                       extension, **kwargs)
 
     def _compose(self, prompt, to, subject, attach_path, mime_subtype,
-                extension, body=None):
+                 extension, body=None, from_=None):
         """Invoke a mail client as a commandline process.
 
         Overridden by MAPIClient.
@@ -166,6 +166,8 @@
             "text", but the precise subtype can be specified here
         :param extension: A file extension (including period) associated with
             the attachment type.
+        :param body: Optional body text.
+        :param from_: Optional From: header.
         """
         for name in self._get_client_commands():
             cmdline = [self._encode_path(name, 'executable')]
@@ -173,6 +175,8 @@
                 kwargs = {'body': body}
             else:
                 kwargs = {}
+            if from_ is not None:
+                kwargs['from_'] = from_
             cmdline.extend(self._get_compose_commandline(to, subject,
                                                          attach_path,
                                                          **kwargs))
@@ -331,25 +335,45 @@
 class Claws(ExternalMailClient):
     """Claws mail client."""
 
+    supports_body = True
+
     _client_commands = ['claws-mail']
 
-    def _get_compose_commandline(self, to, subject, attach_path):
+    def _get_compose_commandline(self, to, subject, attach_path, body=None,
+                                 from_=None):
         """See ExternalMailClient._get_compose_commandline"""
-        compose_url = ['mailto:']
-        if to is not None:
-            compose_url.append(self._encode_safe(to))
-        compose_url.append('?')
+        compose_url = []
+        if from_ is not None:
+            compose_url.append('from=' + urllib.quote(from_))
         if subject is not None:
             # Don't use urllib.quote_plus because Claws doesn't seem
             # to recognise spaces encoded as "+".
             compose_url.append(
-                'subject=%s' % urllib.quote(self._encode_safe(subject)))
+                'subject=' + urllib.quote(self._encode_safe(subject)))
+        if body is not None:
+            compose_url.append(
+                'body=' + urllib.quote(self._encode_safe(body)))
+        # to must be supplied for the claws-mail --compose syntax to work.
+        if to is None:
+            raise errors.NoMailAddressSpecified()
+        compose_url = 'mailto:%s?%s' % (
+            self._encode_safe(to), '&'.join(compose_url))
         # Collect command-line options.
-        message_options = ['--compose', ''.join(compose_url)]
+        message_options = ['--compose', compose_url]
         if attach_path is not None:
             message_options.extend(
                 ['--attach', self._encode_path(attach_path, 'attachment')])
         return message_options
+
+    def _compose(self, prompt, to, subject, attach_path, mime_subtype,
+                 extension, body=None, from_=None):
+        """See ExternalMailClient._compose"""
+        if from_ is None:
+            from_ = self.config.get_user_option('email')
+        super(Claws, self)._compose(prompt, to, subject, attach_path,
+                                    mime_subtype, extension, body, from_)
+
+
 mail_client_registry.register('claws', Claws,
                               help=Claws.__doc__)
 
 
=== modified file 'bzrlib/tests/test_mail_client.py'
--- bzrlib/tests/test_mail_client.py	2009-05-11 18:35:20 +0000
+++ bzrlib/tests/test_mail_client.py	2009-06-04 20:35:26 +0000
@@ -191,9 +191,10 @@
     def test_commandline(self):
         claws = mail_client.Claws(None)
         commandline = claws._get_compose_commandline(
-            None, None, 'file%')
+            '[e-mail]', None, 'file%')
         self.assertEqual(
-            ['--compose', 'mailto:?', '--attach', 'file%'], commandline)
+            ['--compose', 'mailto:[e-mail]?', '--attach', 'file%'],
+            commandline)
         commandline = claws._get_compose_commandline(
             '[e-mail]', 'Hi there!', None)
         self.assertEqual(
@@ -217,6 +218,30 @@
             self.assertFalse(isinstance(item, unicode),
                 'Command-line item %r is unicode!' % item)
 
+    def test_with_from(self):
+        claws = mail_client.Claws(None)
+        cmdline = claws._get_compose_commandline(
+            u'[e-mail]', None, None, None, u'[e-mail]')
+        self.assertEqual(
+            ['--compose',
+             'mailto:[e-mail]?from=qrandom%40example.com'],
+            cmdline)
+
+    def test_to_required(self):
+        claws = mail_client.Claws(None)
+        self.assertRaises(errors.NoMailAddressSpecified,
+                          claws._get_compose_commandline,
+                          None, None, 'file%')
+
+    def test_with_body(self):
+        claws = mail_client.Claws(None)
+        cmdline = claws._get_compose_commandline(
+            u'[e-mail]', None, None, 'This is some body text')
+        self.assertEqual(
+            ['--compose',
+             'mailto:[e-mail]?body=This%20is%20some%20body%20text'],
+            cmdline)
+
 
 class TestEditor(tests.TestCase):