Merge lp:~barry/bzr/claws into lp:~bzr/bzr/trunk-old
- claws
- Merge into trunk-old
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Aaron Bentley | ||||
Approved revision: | no longer in the source branch. | ||||
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
John A Meinel | Approve | ||
Review via email: mp+7038@code.launchpad.net |
Commit message
Description of the change
Barry Warsaw (barry) wrote : | # |
John A Meinel (jameinel) wrote : | # |
As long as you've tested it manually, looks good to me.
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_
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_
>
> 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_
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://
iEYEARECAAYFAko
VI8An3VgTCyyTOO
=mb8c
-----END PGP SIGNATURE-----
Barry Warsaw (barry) wrote : | # |
> I'm just sayin' _get_compose_
> 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://
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_
>> 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://
>
> 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://
iEYEARECAAYFAko
NNUAnA6Y5h/
=mOnN
-----END PGP SIGNATURE-----
Barry Warsaw (barry) wrote : | # |
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/
--- bzrlib/
+++ bzrlib/
@@ -155,7 +155,7 @@
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 @@
: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_
@@ -173,6 +175,8 @@
else:
+ if from_ is not None:
+ kwargs['from_'] = from_
@@ -335,12 +339,12 @@
_client_
- def _get_compose_
+ def _get_compose_
+ from_=None):
"""See ExternalMailCli
- email = self.config.
- if email is not None:
- compose_
+ if from_ is not None:
+ compose_
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 @@
return message_options
+
+ def _compose(self, prompt, to, subject, attach_path, mime_subtype,
+ extension, body=None, from_=None):
+ """See ExternalMailCli
+ if from_ is None:
+ from_ = self.config.
+ super(Claws, self)._
+ mime_subtype, extension, body, from_)
+
+
mail_client_
=== modified file 'bzrlib/
--- bzrlib/
+++ bzrlib/
@@ -191,9 +191,10 @@
...
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/
--- bzrlib/
+++ bzrlib/
@@ -233,6 +233,15 @@
+ def test_with_
+ claws = mail_client.
+ cmdline = claws._
+ <email address hidden>', None, None, 'This is some body text')
+ self.assertEqual(
+ ['--compose',
+ 'mailto:<email address hidden>
+ cmdline)
+
class TestEditor(
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://
iEYEARECAAYFAko
MLQAniDqyKCJX41
=YR+A
-----END PGP SIGNATURE-----
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
1 | === modified file 'bzrlib/mail_client.py' |
2 | --- bzrlib/mail_client.py 2009-06-03 19:05:40 +0000 |
3 | +++ bzrlib/mail_client.py 2009-06-04 20:35:26 +0000 |
4 | @@ -155,7 +155,7 @@ |
5 | extension, **kwargs) |
6 | |
7 | def _compose(self, prompt, to, subject, attach_path, mime_subtype, |
8 | - extension, body=None): |
9 | + extension, body=None, from_=None): |
10 | """Invoke a mail client as a commandline process. |
11 | |
12 | Overridden by MAPIClient. |
13 | @@ -166,6 +166,8 @@ |
14 | "text", but the precise subtype can be specified here |
15 | :param extension: A file extension (including period) associated with |
16 | the attachment type. |
17 | + :param body: Optional body text. |
18 | + :param from_: Optional From: header. |
19 | """ |
20 | for name in self._get_client_commands(): |
21 | cmdline = [self._encode_path(name, 'executable')] |
22 | @@ -173,6 +175,8 @@ |
23 | kwargs = {'body': body} |
24 | else: |
25 | kwargs = {} |
26 | + if from_ is not None: |
27 | + kwargs['from_'] = from_ |
28 | cmdline.extend(self._get_compose_commandline(to, subject, |
29 | attach_path, |
30 | **kwargs)) |
31 | @@ -331,25 +335,45 @@ |
32 | class Claws(ExternalMailClient): |
33 | """Claws mail client.""" |
34 | |
35 | + supports_body = True |
36 | + |
37 | _client_commands = ['claws-mail'] |
38 | |
39 | - def _get_compose_commandline(self, to, subject, attach_path): |
40 | + def _get_compose_commandline(self, to, subject, attach_path, body=None, |
41 | + from_=None): |
42 | """See ExternalMailClient._get_compose_commandline""" |
43 | - compose_url = ['mailto:'] |
44 | - if to is not None: |
45 | - compose_url.append(self._encode_safe(to)) |
46 | - compose_url.append('?') |
47 | + compose_url = [] |
48 | + if from_ is not None: |
49 | + compose_url.append('from=' + urllib.quote(from_)) |
50 | if subject is not None: |
51 | # Don't use urllib.quote_plus because Claws doesn't seem |
52 | # to recognise spaces encoded as "+". |
53 | compose_url.append( |
54 | - 'subject=%s' % urllib.quote(self._encode_safe(subject))) |
55 | + 'subject=' + urllib.quote(self._encode_safe(subject))) |
56 | + if body is not None: |
57 | + compose_url.append( |
58 | + 'body=' + urllib.quote(self._encode_safe(body))) |
59 | + # to must be supplied for the claws-mail --compose syntax to work. |
60 | + if to is None: |
61 | + raise errors.NoMailAddressSpecified() |
62 | + compose_url = 'mailto:%s?%s' % ( |
63 | + self._encode_safe(to), '&'.join(compose_url)) |
64 | # Collect command-line options. |
65 | - message_options = ['--compose', ''.join(compose_url)] |
66 | + message_options = ['--compose', compose_url] |
67 | if attach_path is not None: |
68 | message_options.extend( |
69 | ['--attach', self._encode_path(attach_path, 'attachment')]) |
70 | return message_options |
71 | + |
72 | + def _compose(self, prompt, to, subject, attach_path, mime_subtype, |
73 | + extension, body=None, from_=None): |
74 | + """See ExternalMailClient._compose""" |
75 | + if from_ is None: |
76 | + from_ = self.config.get_user_option('email') |
77 | + super(Claws, self)._compose(prompt, to, subject, attach_path, |
78 | + mime_subtype, extension, body, from_) |
79 | + |
80 | + |
81 | mail_client_registry.register('claws', Claws, |
82 | help=Claws.__doc__) |
83 | |
84 | |
85 | === modified file 'bzrlib/tests/test_mail_client.py' |
86 | --- bzrlib/tests/test_mail_client.py 2009-05-11 18:35:20 +0000 |
87 | +++ bzrlib/tests/test_mail_client.py 2009-06-04 20:35:26 +0000 |
88 | @@ -191,9 +191,10 @@ |
89 | def test_commandline(self): |
90 | claws = mail_client.Claws(None) |
91 | commandline = claws._get_compose_commandline( |
92 | - None, None, 'file%') |
93 | + 'jrandom@example.org', None, 'file%') |
94 | self.assertEqual( |
95 | - ['--compose', 'mailto:?', '--attach', 'file%'], commandline) |
96 | + ['--compose', 'mailto:jrandom@example.org?', '--attach', 'file%'], |
97 | + commandline) |
98 | commandline = claws._get_compose_commandline( |
99 | 'jrandom@example.org', 'Hi there!', None) |
100 | self.assertEqual( |
101 | @@ -217,6 +218,30 @@ |
102 | self.assertFalse(isinstance(item, unicode), |
103 | 'Command-line item %r is unicode!' % item) |
104 | |
105 | + def test_with_from(self): |
106 | + claws = mail_client.Claws(None) |
107 | + cmdline = claws._get_compose_commandline( |
108 | + u'jrandom@example.org', None, None, None, u'qrandom@example.com') |
109 | + self.assertEqual( |
110 | + ['--compose', |
111 | + 'mailto:jrandom@example.org?from=qrandom%40example.com'], |
112 | + cmdline) |
113 | + |
114 | + def test_to_required(self): |
115 | + claws = mail_client.Claws(None) |
116 | + self.assertRaises(errors.NoMailAddressSpecified, |
117 | + claws._get_compose_commandline, |
118 | + None, None, 'file%') |
119 | + |
120 | + def test_with_body(self): |
121 | + claws = mail_client.Claws(None) |
122 | + cmdline = claws._get_compose_commandline( |
123 | + u'jrandom@example.org', None, None, 'This is some body text') |
124 | + self.assertEqual( |
125 | + ['--compose', |
126 | + 'mailto:jrandom@example.org?body=This%20is%20some%20body%20text'], |
127 | + cmdline) |
128 | + |
129 | |
130 | class TestEditor(tests.TestCase): |
131 |
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.