Re: [evolution-patches] Unified Attachment Bar Patch

[Not Zed <notzed ximian com>]

Sigh.  Just this one bit of code below is - far less efficient than it
need be, and leaks memory like a sieve.

I dont really know what i'm supposed to do to review code like this,
tell you every thing you forgot, ask you to resubmit a more finished
patch, or just ignore it and hope it'll go away?

Lets see:
 the mstream is not needed, and you leak it.
 the copy of the mstream 'buffer' is not needed,and you leak that too
 you leak the 'safe filename'
 yes, you do need to close the fd (you dont even need that either).

You can just create a file stream and write directly to it, you dont
need any intermediate memory objects, or even redundant copies of same.

Other than that - the attachment bar and attachment object methods seem
a bit odd.  there seems to be a strange mixture of funtionality spread
across the two objects.

I suggest you simplify the attachment-bar greatly, and only let it
add/remove e-attachment objects, and get the selection.  I dont tihnk
you need any of the add attachment type helpers.  The e-attachment
object should contain all of the remote-downloading code too - it is the
one which is managing a specific attachment.  It can contain all of the
editing code too.

That should simplify things a bit overall, even if you might need to
change more of the using code (although only in small ways).

 Michael

On Thu, 2005-07-07 at 22:20 +0530, Srinivasa Ragavan wrote:
> +               CamelDataWrapper *wrapper;
> +               CamelStreamMem *mstream;
> +               unsigned char *buffer = NULL;
> +               int fd;
> +               char *attach_file_url;
> +       
> +               wrapper = camel_medium_get_content_object
> (CAMEL_MEDIUM (p->data));
> +               mstream = (CamelStreamMem *) camel_stream_mem_new ();
> +                       
> +               camel_data_wrapper_decode_to_stream (wrapper,
> (CamelStream *) mstream);
> +               buffer = g_memdup (mstream->buffer->data,
> mstream->buffer->len);
> +
> +
> +               /* Extract the content from the stream and write it
> down
> +                * as a mime part file into the directory denoting the
> +                * calendar source */
> +               attach_file_url = g_strconcat (local_store,
> +                                              comp_uid,  "-",
> +
> camel_file_util_safe_filename
> +
> (camel_mime_part_get_filename ((CamelMimePart *)p->data)), NULL); 
> +
> +               fd = open (attach_file_url+7, O_RDWR|O_CREAT|O_TRUNC,
> 0600);
> +               if (fd == -1) {
> +                       /* TODO handle error conditions */
> +                       g_message ("DEBUG: could not open the file
> descriptor\n");
> +                       continue;
> +               }
> +
> +               /* write the camel mime part  (attachment->body) into
> the file*/
> +               if (camel_write (fd, buffer, mstream->buffer->len) ==
> -1) {
> +                       /* TODO handle error condition */
> +                       g_message ("DEBUG: camel write failed.\n");
> +                       continue;
> +               }
> +               list = g_slist_append (list, g_strdup
> (attach_file_url));
> +               /* do i need to close the fd */
> +               g_free (attach_file_url);
> +       }
> +