Re: [Evolution-hackers] New patches for Inline PGP support

[Not Zed <notzed ximian com>]

Looking good matt,

I have a few comments, naturally :)

First, I think you have too much of the decoding logic in the format
handlers.  Really, you should be able to pass a whole part to the
cipher, and get a fully decoded part back - that is why it takes a
single mime part and returns a single mime part.  You definitely
shouldn't have to do any stream processing on it to extract components
out of it - this isn't the text/plain decoder which doesn't know if it
really has text/plain, this is the application/x-inline-pgp-signed
decider which should already know what it has.  It really shoudln't be
any different from the logic of the other formatters for encrypted
parts, just dump the part to the right camel-cipher, and then format the
result - i.e. 15 lines of code max.  The cipher is where all the ugly
code needs to go, particularly for decrypting.

You definitely DEFINITELY must not pass a data-wrapper to the
cipher_decrypt function.  mimepart subclasses data-wrapper, not the
other way around!  The cipher interface is so that it can do whatever
extra parsing/required internally, not leave it to the application.
Just have the cipher fill out the mimepart with the content of the final
output and set the mime-type to application/octet-stream.  I know this
means text/plain attachments get an attachment bar - but that can
probably be fixed with some hackery, so don't worry about that for now.

Finally, decode_to_stream() should already handle any quoted-printable
stuff, you dont need to add your own qp decoder/etc.  You shouldn't need
to worry about transfer-encoding at that level at all.  The code which
checks the transfer encoding looks like a very complicated sequence of
code which just copies the content, with no actual affect, except
perhaps corrupting it since it doesn't set the output encoding to
binary!  I dont think this is step required at all, not even in the
cipher (and if it was, the cipher can just add a filter, without having
to create whole copies of temporary parts :-).

The signed handler is a bit different - i guess it still needs to strip
out the armouring and create a new part to pass to format_secure, but
that is all, it shouldn't need to prepare content for the cipher at all.

Anyway, I think it is just a matter of a little re-arranging of the
code, most of the parts look good otherwise :)  Oh, remember to clean up
properly in the failure cases, i.e. the if (!valid) code doesn't do any.

I'm not sure about the text/plain formatting changes; I see why you're
doing it, but it will affect the formatting, and add weird extra boxes
in certain cases I think.  I guess i'll have to see how it looks.

Thanks,
 Michael

PS I think it is satisfying from my point of view to see that it isn't
actually all that much work in the end ... and is clean and isolated
work too.

On Tue, 2005-05-31 at 01:02 +1200, Matt Brown wrote:
> Get the patches at
> http://www.mattb.net.nz/patches/evolution/evo-inline-pgp.patch
> http://www.mattb.net.nz/patches/evolution/eds-inline-pgp.patch
> 
> evo-inline-pgp.patch
> 
> Adds support to em-inline-filter for breaking out inline signed and
> inline encrypted messages into two fake mime types
> 
> Adds two formatters to display these mime types (after sending things
> off to various camel_gpg_ functions in eds and the appropriate
> filters). 
> 
> eds-inline-pgp.patch
> 
> Modifies camel-gpg-context decrypt and verify routines to support the
> fake mime types used for inline pgp. 
> 
> Adds a new filter (camel-mime-filter-pgp) to strip ascii armor and
> dash
> decode pgp clearsigned messages.
> 
> Major changes from previous patches
> - No longer using plugins - this prevents needless duplication of the
> em-inline-filter architecture
> - Now properly handles encrypted content by calling
> em_utils_snoop_part
> to determine the mime type of the decrypted block - this seems to work
> really well for images / text, all handled ok. 
> - All text output is passed back through the standard formatting
> functions to nested inline pgp (or other inline content) is handled
> ok. 
> 
> Outstanding work
> - Several places where things aren't as efficient as they could be
> - inline PGP messages often have trailing whitespace leading to funny
> looking blank text parts after the signature row. Need to investigate
> adding support to em-inline-filter to suppress empty plain text
> parts. 
> 
> Testing / Code Review of the patches would be much appreciated. I've
> tried to incorporate all the suggestions / code style pointers that
> Not
> Zed and Jeffrey commented on after the last patches. 
>