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

Proposed by Barry Warsaw on 2009-06-03
Status: Merged
Approved by: Aaron Bentley on 2009-06-04
Approved revision: 4404
Merged at revision: not available
Proposed branch: lp:~barry/bzr/claws
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 130 lines
To merge this branch: bzr merge lp:~barry/bzr/claws
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve on 2009-06-04
John A Meinel 2009-06-03 Approve on 2009-06-03
Review via email: mp+7038@code.launchpad.net
To post a comment you must log in.
Barry Warsaw (barry) wrote :

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) wrote :

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

review: Approve
Barry Warsaw (barry) 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. I know the code in the branch works for Claws though. What do you think of Aaron's suggestions?

Aaron Bentley (abentley) wrote :

-----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) 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.

Aaron Bentley (abentley) wrote :

-----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
lp:~barry/bzr/claws updated on 2009-06-04
4403. By Barry Warsaw on 2009-06-04

Much simpler approach to support From: in Claws, after discussion with
abentley. This does not try to add the support to every mail client.

Barry Warsaw (barry) wrote :
Download full text (4.4 KiB)

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 @@
...

Read more...

Barry Warsaw (barry) 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.

=== 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):

lp:~barry/bzr/claws updated on 2009-06-04
4404. By Barry Warsaw on 2009-06-04

Add test for including body text for Claws.

Aaron Bentley (abentley) wrote :

-----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) wrote :

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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
155 extension, **kwargs)155 extension, **kwargs)
156156
157 def _compose(self, prompt, to, subject, attach_path, mime_subtype,157 def _compose(self, prompt, to, subject, attach_path, mime_subtype,
158 extension, body=None):158 extension, body=None, from_=None):
159 """Invoke a mail client as a commandline process.159 """Invoke a mail client as a commandline process.
160160
161 Overridden by MAPIClient.161 Overridden by MAPIClient.
@@ -166,6 +166,8 @@
166 "text", but the precise subtype can be specified here166 "text", but the precise subtype can be specified here
167 :param extension: A file extension (including period) associated with167 :param extension: A file extension (including period) associated with
168 the attachment type.168 the attachment type.
169 :param body: Optional body text.
170 :param from_: Optional From: header.
169 """171 """
170 for name in self._get_client_commands():172 for name in self._get_client_commands():
171 cmdline = [self._encode_path(name, 'executable')]173 cmdline = [self._encode_path(name, 'executable')]
@@ -173,6 +175,8 @@
173 kwargs = {'body': body}175 kwargs = {'body': body}
174 else:176 else:
175 kwargs = {}177 kwargs = {}
178 if from_ is not None:
179 kwargs['from_'] = from_
176 cmdline.extend(self._get_compose_commandline(to, subject,180 cmdline.extend(self._get_compose_commandline(to, subject,
177 attach_path,181 attach_path,
178 **kwargs))182 **kwargs))
@@ -331,25 +335,45 @@
331class Claws(ExternalMailClient):335class Claws(ExternalMailClient):
332 """Claws mail client."""336 """Claws mail client."""
333337
338 supports_body = True
339
334 _client_commands = ['claws-mail']340 _client_commands = ['claws-mail']
335341
336 def _get_compose_commandline(self, to, subject, attach_path):342 def _get_compose_commandline(self, to, subject, attach_path, body=None,
343 from_=None):
337 """See ExternalMailClient._get_compose_commandline"""344 """See ExternalMailClient._get_compose_commandline"""
338 compose_url = ['mailto:']345 compose_url = []
339 if to is not None:346 if from_ is not None:
340 compose_url.append(self._encode_safe(to))347 compose_url.append('from=' + urllib.quote(from_))
341 compose_url.append('?')
342 if subject is not None:348 if subject is not None:
343 # Don't use urllib.quote_plus because Claws doesn't seem349 # Don't use urllib.quote_plus because Claws doesn't seem
344 # to recognise spaces encoded as "+".350 # to recognise spaces encoded as "+".
345 compose_url.append(351 compose_url.append(
346 'subject=%s' % urllib.quote(self._encode_safe(subject)))352 'subject=' + urllib.quote(self._encode_safe(subject)))
353 if body is not None:
354 compose_url.append(
355 'body=' + urllib.quote(self._encode_safe(body)))
356 # to must be supplied for the claws-mail --compose syntax to work.
357 if to is None:
358 raise errors.NoMailAddressSpecified()
359 compose_url = 'mailto:%s?%s' % (
360 self._encode_safe(to), '&'.join(compose_url))
347 # Collect command-line options.361 # Collect command-line options.
348 message_options = ['--compose', ''.join(compose_url)]362 message_options = ['--compose', compose_url]
349 if attach_path is not None:363 if attach_path is not None:
350 message_options.extend(364 message_options.extend(
351 ['--attach', self._encode_path(attach_path, 'attachment')])365 ['--attach', self._encode_path(attach_path, 'attachment')])
352 return message_options366 return message_options
367
368 def _compose(self, prompt, to, subject, attach_path, mime_subtype,
369 extension, body=None, from_=None):
370 """See ExternalMailClient._compose"""
371 if from_ is None:
372 from_ = self.config.get_user_option('email')
373 super(Claws, self)._compose(prompt, to, subject, attach_path,
374 mime_subtype, extension, body, from_)
375
376
353mail_client_registry.register('claws', Claws,377mail_client_registry.register('claws', Claws,
354 help=Claws.__doc__)378 help=Claws.__doc__)
355379
356380
=== 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 @@
191 def test_commandline(self):191 def test_commandline(self):
192 claws = mail_client.Claws(None)192 claws = mail_client.Claws(None)
193 commandline = claws._get_compose_commandline(193 commandline = claws._get_compose_commandline(
194 None, None, 'file%')194 'jrandom@example.org', None, 'file%')
195 self.assertEqual(195 self.assertEqual(
196 ['--compose', 'mailto:?', '--attach', 'file%'], commandline)196 ['--compose', 'mailto:jrandom@example.org?', '--attach', 'file%'],
197 commandline)
197 commandline = claws._get_compose_commandline(198 commandline = claws._get_compose_commandline(
198 'jrandom@example.org', 'Hi there!', None)199 'jrandom@example.org', 'Hi there!', None)
199 self.assertEqual(200 self.assertEqual(
@@ -217,6 +218,30 @@
217 self.assertFalse(isinstance(item, unicode),218 self.assertFalse(isinstance(item, unicode),
218 'Command-line item %r is unicode!' % item)219 'Command-line item %r is unicode!' % item)
219220
221 def test_with_from(self):
222 claws = mail_client.Claws(None)
223 cmdline = claws._get_compose_commandline(
224 u'jrandom@example.org', None, None, None, u'qrandom@example.com')
225 self.assertEqual(
226 ['--compose',
227 'mailto:jrandom@example.org?from=qrandom%40example.com'],
228 cmdline)
229
230 def test_to_required(self):
231 claws = mail_client.Claws(None)
232 self.assertRaises(errors.NoMailAddressSpecified,
233 claws._get_compose_commandline,
234 None, None, 'file%')
235
236 def test_with_body(self):
237 claws = mail_client.Claws(None)
238 cmdline = claws._get_compose_commandline(
239 u'jrandom@example.org', None, None, 'This is some body text')
240 self.assertEqual(
241 ['--compose',
242 'mailto:jrandom@example.org?body=This%20is%20some%20body%20text'],
243 cmdline)
244
220245
221class TestEditor(tests.TestCase):246class TestEditor(tests.TestCase):
222247