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. > _______________________________________________ evolution-hackers maillist - [email protected] http://lists.ximian.com/mailman/listinfo/evolution-hackers
